Hello everyone,
I'd like to port my re-implementation of task dialogs (http://www.korosoft.net/projects/tdemu/) to WINE, actually I have posted a small patch yesterday (merely rearranges some source around, preparatory work) which sadly did not seem to get accepted.
I'm going to take my code and clean it to WINE's source code standards, however I do not see how I could split it in many patches as everything is dependant on everything else.
As this is quite a big amount of work, I would like to have some guidance (what must I do to be sure it is accepted) so that I do not end up doing all that for nothing - starting by knowing why my patch from yesterday was not accepted (what did I do wrong).
Thanks for your time and help.
I have been looking at this and my advice is start simple and establish the basic framework ie. display a frame with a title. Then add functions and text items or buttons types one by one. Add complexity to the functions after the basic form is accepted.
On 09/06/11 17:00, Patrick Gauthier wrote:
Hello everyone,
I'd like to port my re-implementation of task dialogs (http://www.korosoft.net/projects/tdemu/) to WINE, actually I have posted a small patch yesterday (merely rearranges some source around, preparatory work) which sadly did not seem to get accepted.
I'm going to take my code and clean it to WINE's source code standards, however I do not see how I could split it in many patches as everything is dependant on everything else.
As this is quite a big amount of work, I would like to have some guidance (what must I do to be sure it is accepted) so that I do not end up doing all that for nothing - starting by knowing why my patch from yesterday was not accepted (what did I do wrong).
Thanks for your time and help.
Hi Patrick,
As this is quite a big amount of work, I would like to have some guidance (what must I do to be sure it is accepted) so that I do not end up doing all that for nothing - starting by knowing why my patch from yesterday was not accepted (what did I do wrong).
The patch was marked as "Pending." That usually means Alexandre's waiting for something, e.g. for you to fix something obvious, or to see what else you're planning to do. Since you say the patch is preparatory work, it doesn't make much sense to go in unless the remaining patches are also to go in, so I'd suggest sending at least another patch to show where you're going with it.
Regarding splitting, sometimes it's useful to introduce code that will only be removed later. I know this isn't the usual advice, but if you introduce a new implementation of something, sometimes a stub implementation can appear first. E.g., if it's an interface you're implementing, introduce an implementation that does nothing but return E_NOTIMPL from each method. Then one by one introduce implementation for each function.
Hope that helps, --Juan
+ if (pnButton) *pnButton = IDYES; + if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton; + if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE; + return S_OK;
I don't think it's appropriate to make selections for the user like this. If you can't present all of the choices to the user, you should probably return E_NOTIMPL.
Writing tests first would also help.
I'm going to take my code and clean it to WINE's source code standards, however I do not see how I could split it in many patches as everything is dependant on everything else.
I think Lats' advice here is sound. You might start with a dialog with only an OK button (that only displays if the dialog offers no choices to the user), then add features one at a time.
As this is quite a big amount of work, I would like to have some guidance (what must I do to be sure it is accepted) so that I do not end up doing all that for nothing - starting by knowing why my patch from yesterday was not accepted (what did I do wrong).
Well, you shouldn't write it all at once anyway, as you'd likely have to change all of your later work.
(Sorry for the late reply. BSD dev machine had some hard disk issues.)
Thanks for all your input. Here are some comments:
On 06/09/11 10:51, Juan Lang wrote:
The patch was marked as "Pending." That usually means Alexandre's waiting for something, e.g. for you to fix something obvious, or to see what else you're planning to do. Since you say the patch is preparatory work, it doesn't make much sense to go in unless the remaining patches are also to go in, so I'd suggest sending at least another patch to show where you're going with it.
Alright, I will follow-up with more first then.
Regarding splitting, sometimes it's useful to introduce code that will only be removed later. I know this isn't the usual advice, but if you introduce a new implementation of something, sometimes a stub implementation can appear first. E.g., if it's an interface you're implementing, introduce an implementation that does nothing but return E_NOTIMPL from each method. Then one by one introduce implementation for each function.
Thing is, however, that I already got the full implementation written "as a block", short of "replaying my coding process", I don't see how I could efficiently do that. I could always make different patches for different parts, like, dialog creation, then layout, then message handling, etc, but you would not get a working TaskDialog until the last patch was integrated.
On 06/09/11 10:55, Vincent Povirk wrote:
- if (pnButton) *pnButton = IDYES;
- if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton;
- if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE;
- return S_OK;
I don't think it's appropriate to make selections for the user like this. If you can't present all of the choices to the user, you should probably return E_NOTIMPL.
Yes, I agree, if I had made that stub myself I would have returned E_NOTIMPL, but I merely moved it as-is from commctrl.c, I assume it was put there that way quickly to make some application work, I did not want to break anything.
I think Lats' advice here is sound. You might start with a dialog with only an OK button (that only displays if the dialog offers no choices to the user), then add features one at a time.
Well, you shouldn't write it all at once anyway, as you'd likely have to change all of your later work.
Thing is, as I mentioned earlier, the code is already all written and working. (You can download the DLL from my website and see for yourself)
On 06/09/11 11:15, Dan Kegel wrote:
An iota of guidance was provided at http://source.winehq.org/patches/ Scroll down to your patch, you can see it's in "Pending" state. Scroll down further, you'll see that "Pending" means "The patch is not obviously correct at first glance. Making a more convincing argument, preferably in the form of a test case, may help." See also http://wiki.winehq.org/SubmittingPatches for a generic checklist.
Thanks for those two sites, I did not know / forgot they existed. They have been bookmarked so I will check them first before asking.
Your patch seemed to do three things:
- moving old stub into new file
- adding a TaskDialog function
- adding ordinals
Those three things could all be separate patches, and some of them could have tests.
I would like some guidance on what constitutes an adequate patch size then. According to this, I shoud make a commit into my local git every time I do something simple. I can (and will) split this patch as you mention, but I fail to see how I could split my patches as I get into the meat of my implementation (as I mentioned earlier). Input in this area would be appreciated.
Also how do I write a test that tests the ordinals? By checking that GetProcAddress(hMod, "TaskDialogIndirect") == GetProcAddress(hMod, "#345") ?
I'd suggest sending just one of the three first (probably not the one that moves code to a new file), and including a test with the first patch.
- Dan
Alright, will do.
What I seem to get from your input and reading SubmittingPatches, is basically "tests, tests, tests". I thought a while about "how does one automate tests on GUI stuff?" and "how can I ensure that my dialog renders right in a test, short of using a flaky thing like GetPixel()?", etc.
Then it came to me. What I will do is first change the implementation to return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx messages and tests that the callback procedures receives all the right TDN_xxx notifications, at least. It won't be able to test that the dialog _displays_ right, but at least that it functions right from an API point of view. As long as I can close the dialog when done testing, the tests should not require any kind of user interaction.
Of course any input is appreciated, I am still a newbie at this.
- Patrick
On 06/09/11 10:55, Vincent Povirk wrote:
- if (pnButton) *pnButton = IDYES;
- if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton;
- if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE;
- return S_OK;
I don't think it's appropriate to make selections for the user like this. If you can't present all of the choices to the user, you should probably return E_NOTIMPL.
Yes, I agree, if I had made that stub myself I would have returned E_NOTIMPL, but I merely moved it as-is from commctrl.c, I assume it was put there that way quickly to make some application work, I did not want to break anything.
Ah, you're right, I didn't notice that.
Thing is, as I mentioned earlier, the code is already all written and working. (You can download the DLL from my website and see for yourself)
Yeah, splitting an existing implementation can be tough.
On the other hand, the various features (progress bars, radio buttons, that dialog has a lot of stuff going on..) should be written so that they're independent from the other features and you can split them out in a sensible way (though it's still a hassle to actually do it).
I guess many of the elements could be shown or hidden, and each one affects the dialog size and the placement of everything else. But if you do this cleanly, it should still be easy to add each element.
I was hoping to read your existing code and see if anything struck me, but it seems you haven't provided it.
I would like some guidance on what constitutes an adequate patch size then. According to this, I shoud make a commit into my local git every time I do something simple. I can (and will) split this patch as you mention, but I fail to see how I could split my patches as I get into the meat of my implementation (as I mentioned earlier). Input in this area would be appreciated.
As I see it, the following guidelines apply: * The code must compile without warnings and pass all the tests after each patch is applied. Any non-passing tests must be marked as todo_wine. (As a consequence of this, if you fix Wine so that a failing test passes, you must remove the todo_wine as part of that same commit.) * No patch should introduce a memory leak, regression, or other bad thing. If you absolutely cannot avoid doing this (I think it's happened to me once), make a note that you've done this so that your badness doesn't get accepted without the later fix. * Try to avoid introducing unused code, resources, etc. That means adding internal functions as you begin to use them. (However, things like dialog or menu resources can be added as a whole, even if individual controls/menus are unused/unimplemented.) I've had to break this rule once or twice as well. * Each patch should contain a single logical change. * If you can split your patch into multiple logical changes without breaking anything in between, you probably should. * In some cases, you can get away with having two logical changes in a patch, if one of the changes is sufficiently small and related to the other. An example of a small change might be adding a function that only forwards to another that you implement (like TaskDialog/TaskDialogIndirect), or adding a small test that you also fix in Wine. But it's always OK to send multiple patches instead.
There's not really a guideline in terms of lines of code. It's more about how much logic you have in each patch, and how much it relates to the other logic. If it takes a long time for a reader (who we assume is already familiar with the code you're modifying) to understand all the logic you've introduced and how it all relates to each other, that's a sign that your patch is too big.
I would like to point out that your first patch was marked "Pending", not "Needs Splitting" (and also not "Needs tests"). That means Alexandre didn't necessarily object to the number of different things you changed in the patch, but he was uncomfortable with something about one of those changes, and now we don't know which it was. Anything you hear from anyone other than Alexandre is, of course, speculation. (I see "Pending" as meaning "This makes me nervous" while "Rejected" means "I know for sure this is wrong".)
Also how do I write a test that tests the ordinals? By checking that GetProcAddress(hMod, "TaskDialogIndirect") == GetProcAddress(hMod, "#345") ?
No, that would be silly. The most you should need to do there is explain in more detail why you think the ordinal is important and unlikely to change. But it seems to me you already explained that well enough (though I personally wouldn't know how to verify it), and so I really don't think the ordinals are the sticking point. (Come to think of it, there's a way to specify in the .spec file that programs should link by ordinal, and if MS does it then we probably should too. I think it's -ordinal.)
Now that I understand you're not making the existing stub worse, and given that your patch doesn't do very much, the "Pending" status is baffling to me. It makes me think I missed something obvious.
Maybe the problem is that your TaskDialog function doesn't set dwFlags? That would likely result in other invalid fields being used when they shouldn't. From MSDN, it appears many of the other fields should be initialized as well. (But that should have resulted in a "Rejected" status, so maybe he didn't notice that. Hmm.)
What I seem to get from your input and reading SubmittingPatches, is basically "tests, tests, tests". I thought a while about "how does one automate tests on GUI stuff?" and "how can I ensure that my dialog renders right in a test, short of using a flaky thing like GetPixel()?", etc.
Then it came to me. What I will do is first change the implementation to return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx messages and tests that the callback procedures receives all the right TDN_xxx notifications, at least. It won't be able to test that the dialog _displays_ right, but at least that it functions right from an API point of view. As long as I can close the dialog when done testing, the tests should not require any kind of user interaction.
Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;)
I definitely think testing the notifications is the right idea, and making sure the display and interaction are correct is not as worthwhile. It's still possible to test display and interaction by accessing individual controls by id and using messages to interact with them. But I don't think it's worth doing that unless real programs are known to do that (they often do this with other common dialogs). I also don't know to what extent it's worth making your dialog template compatible with windows, or how to avoid potential copyright infringement when doing that. I hope someone else can chime in on this issue.
I think we may overemphasize the importance of tests. They are not always necessary for getting code into Wine, as our current spotty test coverage proves. Still, they are really nice to have.
On Fri, 2011-06-10 at 23:29 -0500, Vincent Povirk wrote:
Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;)
I added it for the Internet Explorer 9 installer but it has been broken already when someone changed it to return IDYES instead of IDOK in pnButton. I don't know which app that was for.
Am 11.06.2011 09:45, schrieb Hans Leidekker:
On Fri, 2011-06-10 at 23:29 -0500, Vincent Povirk wrote:
Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;)
I added it for the Internet Explorer 9 installer but it has been broken already when someone changed it to return IDYES instead of IDOK in pnButton. I don't know which app that was for.
Yes IE9 installer matters here and it's actually broken, so a working GUI implementation would be awesome.
Am 11.06.2011 01:37, schrieb Patrick Gauthier:
What I seem to get from your input and reading SubmittingPatches, is basically "tests, tests, tests". I thought a while about "how does one automate tests on GUI stuff?" and "how can I ensure that my dialog renders right in a test, short of using a flaky thing like GetPixel()?", etc.
Then it came to me. What I will do is first change the implementation to return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx messages and tests that the callback procedures receives all the right TDN_xxx notifications, at least. It won't be able to test that the dialog _displays_ right, but at least that it functions right from an API point of view. As long as I can close the dialog when done testing, the tests should not require any kind of user interaction.
Right, you can send messages and maybe also send keys but not GetPixel(). One interessting part is the returnvalue for various "bad" inputs like NULL for pointers and so on. You can have a look at the user32 tests, maybe the file dlls/user32/tests/dialog.c is good to get inspired for some special things.
Thing is, however, that I already got the full implementation written "as a block", short of "replaying my coding process", I don't see how I could efficiently do that. I could always make different patches for different parts, like, dialog creation, then layout, then message handling, etc, but you would not get a working TaskDialog until the last patch was integrated.
It should work as your existing implementation after all patches are committed. Between the patches only the rules apply that Vincent just sent on that topic.
In short it means if you send e.g. 10 patches and progressbar patch is patch 6, then Wine shouldn't suffer after patch 1-5 applied like compilation errors or at runtime crashes and the tests should still apply. But of course at this point a request to TaskDialog to display a progressbar won't show one, so it maybe should return E_NOTIMPL or something and print a FIXME.
On 11/06/11 17:45, Hans Leidekker wrote:
On Fri, 2011-06-10 at 23:29 -0500, Vincent Povirk wrote:
Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;)
I added it for the Internet Explorer 9 installer but it has been broken already when someone changed it to return IDYES instead of IDOK in pnButton. I don't know which app that was for.
I changed it to work with uTorrent so that exit would work and you could delete torrents.
-
André Hentschel -
Hans Leidekker -
Jeff Latimer -
Juan Lang -
Lats
-
Patrick Gauthier -
Vincent Povirk