TaskDialog implementation

[Patrick Gauthier]

(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