(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