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

[Jacek Caban]

On 03/01/16 14:41, Dmitry Timoshkov wrote:
> 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.

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

Jacek