[PATCH 2/3] user32: Added more dialog owner tests.

[Dmitry Timoshkov]

Jacek Caban <jacek at codeweavers.com> wrote:

> >>>>>>>>> Please don't create fake embedded blocks.
> >>>>>>>> I need this declaration here, otherwise I wouldn't be able to use
> >>>>>>>> handles for testing lparam value.
> >>>>>>> wparam/lparam could be tested directly in the window procedure.
> >>>>>> It would require some global variables and new window procedure. It's
> >>>>>> cleaner to do it using existing tools IMO.
> >>>>> Writing the tests for user32 requires a bit of efforts, yes.
> >>>> I'd prefer to respond to comments that intend to improve the patch
> >>>> instead of comments that intend to increase efforts.
> >>> I already described the ways how to improve the patch. Improving a patch
> >>> and writing good tests usually increases the efforts, not avoids them.
> >> Yes, and I addressed all of your comments on wine-devel.
> >>
> >> - SetParent needs to be used. If we used CreateWindowEx without WS_CHILD 
> >> (which the test needs), you'd have an owned window, not child window. Do 
> >> you need more explanation?
> >> - As I explained you, I already added GetParent()/GetWindow() tests and 
> >> fixes for this case to Wine and those are unrelated to the problem that 
> >> this series fixes. I even showed that to you before you asked me to add 
> >> them.
> >> - My tests do test that messages are sent to child (there is parent flag 
> >> in message sequence which is used for that). That's what you wanted to 
> >> be tested, but it's there in the patch already.
> >>
> >> Did I miss anything?
> > Since SetParent() apparently makes a difference it would be interesting to
> > see the tests without it (with parent specified in the CreateWindowEx call).
> 
> Then there is nothing new. Parent will be the owner same as in other
> already existing tests.

But there is no an existing test that checks the lparam the way you seem
to be mostly interested.

> > And if you really need to patch the message sequnce with custom lparam
> > please make the message array a global variable, or (if you prefer a local
> > one) declare it at the beginning of the function.
> 
> I don't know values that I need at the beginning of the function. lparam
> is filled with handles to windows that need to be created first.

I really don't like ugly fake embedded block and would like to avoid introducing
such bad programming practices in Wine code. I see a couple of less ugly solutions:
1) patch the message sequence using predefined indexes (should be slightly less ugly
than an embedded block) 2) move the lparam checks into dedicated window procedure,
and pass the values to check against either via global variables or a CreateWindow
extra parameter.

-- 
Dmitry.