(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
