Re: TaskDialog implementation

[Jeff Latimer]

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.

Re: TaskDialog implementation

[André Hentschel]

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.

-- 

Best Regards, André Hentschel

Re: TaskDialog implementation

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

Re: 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

Re: 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

re: TaskDialog implementation

[Dan Kegel]

Patrick wrote:
> I'd like to port my re-implementation of task dialogs
> (http://www.korosoft.net/projects/tdemu/) to WINE

Awesome!

> I would like to have some guidance ... [Why was my patch from
> yesterday not accepted?]

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.

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

Re: TaskDialog implementation

[Vincent Povirk]

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

Re: TaskDialog implementation

[Juan Lang]

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

Re: TaskDialog implementation

[Lats]

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