TaskDialog implementation

[Vincent Povirk]

> 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.