[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.
> 
> I wasn't mostly interested in them, but they are a nice feature. I was
> making my new tests stricter when I was experimenting with them and I
> think it's a good thing, so I left it in the patch. It doesn't mean we
> should rewrite existing tests to do the same.

I'm not asking to rewrite existing tests, I'm asking to add another test
which does exactly the same as your new test but specifies parent in the
CreateWindowEx instead of using SetParent.

> >>> 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.
> 
> What you propose is way uglier for my taste. We're getting into coding
> style discussion here, which I'd like to avoid. Would that make you
> happy if I pretended to handle error with
> 
> if (hdlg)
> {
> }
> 
> block and declare variable inside this block?

Not really, it's not any better. What do you think about moving it into
a separate helper function that takes the hwnd to check for?

-- 
Dmitry.