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

[Dmitry Timoshkov]

Hi Jacek,

Jacek Caban  wrote:
> - parent = CreateWindowExA(0, "TestParentClass", "Test parent",
> -
>                         WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> -
>                         100, 100, 200, 200, 0, 0, 0, NULL);
> - ok (parent != 0, "Failed to create parent window\n");
> -
> - child = CreateWindowExA(0, "TestWindowClass", "Test child",
> -
>                        WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> -
>                        100, 100, 200, 200, 0, 0, 0, NULL);
> - ok (child != 0, "Failed to create child window\n");
> -
> - child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2",
> -
>                         WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD,
> -
>                         100, 100, 200, 200, child, 0, 0, NULL);
> - ok (child2 != 0, "Failed to create child window\n");
> -
> - SetParent(child, parent);
> - SetFocus(child);

Is that really required to use SetParent() instead of specifying correct
parent in the CreateWindowEx call?
> - flush_sequence();
> - DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 );
> - ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE);

Is this test supposed to show that actual dialog owner is parent window?
If yes, then this theory is not tested at all.
> - SetForegroundWindow(hdlg);
> -
> - {
> -
>    const struct message WmEndDialogSeq[] = {
> -
>        { WM_ENABLE, sent },
> -
>        { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE },
> -
>        { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild },
> -
>        { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> -
>        { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> -
>        /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */
> -
>        { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE },
> -
>        { WM_QUERYNEWPALETTE, sent|optional },
> -
>        { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> -
>        { WM_GETTEXT, sent|optional|defwinproc },
> -
>        { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> -
>        { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild },
> -
>        { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild },
> -
>        { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg },
> -
>        { 0 }
> -
>    };
> -
> -
>    flush_sequence();
> -
>    EndDialog(hdlg, 0);
> -
>    ok_sequence(WmEndDialogSeq, "EndDialog", FALSE);
> - }

Please don't create fake embedded blocks.

Also it would be really nice to send the tests first so it would be more clear
when a todo_wine is removed with a subsequent patch in order to see whether
the patch is actually related to the previously sent tests.

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

[Jacek Caban]

Hi Dmitry,

On 02/29/16 17:16, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Jacek Caban  wrote:
> > - parent = CreateWindowExA(0, "TestParentClass", "Test parent",
> > -
> >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > -
> >                         100, 100, 200, 200, 0, 0, 0, NULL);
> > - ok (parent != 0, "Failed to create parent window\n");
> > -
> > - child = CreateWindowExA(0, "TestWindowClass", "Test child",
> > -
> >                        WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > -
> >                        100, 100, 200, 200, 0, 0, 0, NULL);
> > - ok (child != 0, "Failed to create child window\n");
> > -
> > - child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2",
> > -
> >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD,
> > -
> >                         100, 100, 200, 200, child, 0, 0, NULL);
> > - ok (child2 != 0, "Failed to create child window\n");
> > -
> > - SetParent(child, parent);
> > - SetFocus(child);
>
> Is that really required to use SetParent() instead of specifying correct
> parent in the CreateWindowEx call?

Yes, SetParent and CreateWindowEx's parent are not the same thing. See
for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the
difference.
> > - flush_sequence();
> > - DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 );
> > - ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE);
>
> Is this test supposed to show that actual dialog owner is parent window?
> If yes, then this theory is not tested at all.

No, it's actually supposed to show that owner is not the parent. We know
that owner is child in this case, but current code will use parent
anyway, which is not right. My tests show that messages are sent to
child, not parent.
> > - SetForegroundWindow(hdlg);
> > -
> > - {
> > -
> >    const struct message WmEndDialogSeq[] = {
> > -
> >        { WM_ENABLE, sent },
> > -
> >        { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE },
> > -
> >        { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild },
> > -
> >        { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > -
> >        { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > -
> >        /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */
> > -
> >        { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE },
> > -
> >        { WM_QUERYNEWPALETTE, sent|optional },
> > -
> >        { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > -
> >        { WM_GETTEXT, sent|optional|defwinproc },
> > -
> >        { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > -
> >        { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild },
> > -
> >        { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild },
> > -
> >        { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg },
> > -
> >        { 0 }
> > -
> >    };
> > -
> > -
> >    flush_sequence();
> > -
> >    EndDialog(hdlg, 0);
> > -
> >    ok_sequence(WmEndDialogSeq, "EndDialog", FALSE);
> > - }
>
> 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.
> Also it would be really nice to send the tests first so it would be more clear
> when a todo_wine is removed with a subsequent patch in order to see whether
> the patch is actually related to the previously sent tests.

I'm not sure I agree.

Thanks for the review,
Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > - parent = CreateWindowExA(0, "TestParentClass", "Test parent",
> > > -
> > >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > > -
> > >                         100, 100, 200, 200, 0, 0, 0, NULL);
> > > - ok (parent != 0, "Failed to create parent window\n");
> > > -
> > > - child = CreateWindowExA(0, "TestWindowClass", "Test child",
> > > -
> > >                        WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > > -
> > >                        100, 100, 200, 200, 0, 0, 0, NULL);
> > > - ok (child != 0, "Failed to create child window\n");
> > > -
> > > - child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2",
> > > -
> > >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD,
> > > -
> > >                         100, 100, 200, 200, child, 0, 0, NULL);
> > > - ok (child2 != 0, "Failed to create child window\n");
> > > -
> > > - SetParent(child, parent);
> > > - SetFocus(child);
> >
> > Is that really required to use SetParent() instead of specifying correct
> > parent in the CreateWindowEx call?
>
> Yes, SetParent and CreateWindowEx's parent are not the same thing. See
> for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the
> difference.

For child windows it shouldn't matter.
> > > - flush_sequence();
> > > - DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 );
> > > - ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE);
> >
> > Is this test supposed to show that actual dialog owner is parent window?
> > If yes, then this theory is not tested at all.
>
> No, it's actually supposed to show that owner is not the parent. We know
> that owner is child in this case, but current code will use parent
> anyway, which is not right. My tests show that messages are sent to
> child, not parent.

Adding actual checks with GetParent()/GetWindow(GW_OWNER)/GetAncestor()
would be preferrable IMO instead of not obvious message sequences.
> > > - SetForegroundWindow(hdlg);
> > > -
> > > - {
> > > -
> > >    const struct message WmEndDialogSeq[] = {
> > > -
> > >        { WM_ENABLE, sent },
> > > -
> > >        { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE },
> > > -
> > >        { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild },
> > > -
> > >        { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > > -
> > >        { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > > -
> > >        /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */
> > > -
> > >        { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE },
> > > -
> > >        { WM_QUERYNEWPALETTE, sent|optional },
> > > -
> > >        { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > > -
> > >        { WM_GETTEXT, sent|optional|defwinproc },
> > > -
> > >        { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > > -
> > >        { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild },
> > > -
> > >        { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild },
> > > -
> > >        { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg },
> > > -
> > >        { 0 }
> > > -
> > >    };
> > > -
> > > -
> > >    flush_sequence();
> > > -
> > >    EndDialog(hdlg, 0);
> > > -
> > >    ok_sequence(WmEndDialogSeq, "EndDialog", FALSE);
> > > - }
> >
> > 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.
> > Also it would be really nice to send the tests first so it would be more clear
> > when a todo_wine is removed with a subsequent patch in order to see whether
> > the patch is actually related to the previously sent tests.
>
> I'm not sure I agree.

Please don't get me wrong, but when sending tests after the fixes the result
can't be easily verified, and therefore this reduces the trust and value of
patches.

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

[Jacek Caban]

Hi Dmitry,

On 02/29/16 17:40, Dmitry Timoshkov wrote:
> Jacek Caban  wrote:
> > > > - parent = CreateWindowExA(0, "TestParentClass", "Test parent",
> > > > -
> > > >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > > > -
> > > >                         100, 100, 200, 200, 0, 0, 0, NULL);
> > > > - ok (parent != 0, "Failed to create parent window\n");
> > > > -
> > > > - child = CreateWindowExA(0, "TestWindowClass", "Test child",
> > > > -
> > > >                        WS_OVERLAPPEDWINDOW | WS_VISIBLE,
> > > > -
> > > >                        100, 100, 200, 200, 0, 0, 0, NULL);
> > > > - ok (child != 0, "Failed to create child window\n");
> > > > -
> > > > - child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2",
> > > > -
> > > >                         WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD,
> > > > -
> > > >                         100, 100, 200, 200, child, 0, 0, NULL);
> > > > - ok (child2 != 0, "Failed to create child window\n");
> > > > -
> > > > - SetParent(child, parent);
> > > > - SetFocus(child);
> > >
> > > Is that really required to use SetParent() instead of specifying correct
> > > parent in the CreateWindowEx call?
> >
> > Yes, SetParent and CreateWindowEx's parent are not the same thing. See
> > for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the
> > difference.
>
> For child windows it shouldn't matter.

It does, otherwise parent would be the owner.
> > > > - flush_sequence();
> > > > - DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 );
> > > > - ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE);
> > >
> > > Is this test supposed to show that actual dialog owner is parent window?
> > > If yes, then this theory is not tested at all.
> >
> > No, it's actually supposed to show that owner is not the parent. We know
> > that owner is child in this case, but current code will use parent
> > anyway, which is not right. My tests show that messages are sent to
> > child, not parent.
>
> Adding actual checks with GetParent()/GetWindow(GW_OWNER)/GetAncestor()
> would be preferrable IMO instead of not obvious message sequences.

If I just wanted to test the owner, then yes. But we already have tests
for that (the one I pointed you) and this part already works fine. I
want to test that messages are sent to the right window, which is what
this series fixes.
> > > > - SetForegroundWindow(hdlg);
> > > > -
> > > > - {
> > > > -
> > > >    const struct message WmEndDialogSeq[] = {
> > > > -
> > > >        { WM_ENABLE, sent },
> > > > -
> > > >        { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE },
> > > > -
> > > >        { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild },
> > > > -
> > > >        { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > > > -
> > > >        { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
> > > > -
> > > >        /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */
> > > > -
> > > >        { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE },
> > > > -
> > > >        { WM_QUERYNEWPALETTE, sent|optional },
> > > > -
> > > >        { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > > > -
> > > >        { WM_GETTEXT, sent|optional|defwinproc },
> > > > -
> > > >        { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
> > > > -
> > > >        { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild },
> > > > -
> > > >        { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild },
> > > > -
> > > >        { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg },
> > > > -
> > > >        { 0 }
> > > > -
> > > >    };
> > > > -
> > > > -
> > > >    flush_sequence();
> > > > -
> > > >    EndDialog(hdlg, 0);
> > > > -
> > > >    ok_sequence(WmEndDialogSeq, "EndDialog", FALSE);
> > > > - }
> > >
> > > 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.

Cheers,
Jacek

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

[Dmitry Timoshkov]

Jacek Caban  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. But since
you are new to this area I think it's justified to ask for more tests
to show that you well understand the code and can proof that the patch
is 100% correct.

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

[Jacek Caban]

On 3/1/16 12:25 AM, Dmitry Timoshkov wrote:
> Jacek Caban  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.
> But since
> you are new to this area I think it's justified to ask for more tests
> to show that you well understand the code and can proof that the patch
> is 100% correct.

Given quality of your comments so far (and I mean technical ones, not 
just this one) I think it's justified to ask you to go back and try to 
understand my patch before questioning my abilities.

Jacek

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

[Dmitry Timoshkov]

Hi Jacek,

Jacek Caban  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.
> > But since
> > you are new to this area I think it's justified to ask for more tests
> > to show that you well understand the code and can proof that the patch
> > is 100% correct.
>
> Given quality of your comments so far (and I mean technical ones, not 
> just this one) I think it's justified to ask you to go back and try to 
> understand my patch before questioning my abilities.

I'm not qustioning your abilities, just pointing out to an obvious fact.
Since I'm the person who spent a lot of time working on user32 stuff and
who has written many of the exisitng user32 tests I think that asking
for a more convincing test shouldn't lead to so a much exagerrated
reaction.

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

[Jacek Caban]

Hi Dmitry,

On 3/1/16 4:31 AM, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Jacek Caban  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?
> > > But since
> > > you are new to this area I think it's justified to ask for more tests
> > > to show that you well understand the code and can proof that the patch
> > > is 100% correct.
> >
> > Given quality of your comments so far (and I mean technical ones, not
> > just this one) I think it's justified to ask you to go back and try to
> > understand my patch before questioning my abilities.
>
> I'm not qustioning your abilities, just pointing out to an obvious fact.
> Since I'm the person who spent a lot of time working on user32 stuff and
> who has written many of the exisitng user32 tests I think that asking
> for a more convincing test shouldn't lead to so a much exagerrated
> reaction.

See above. After giving a few comments that were clearly wrong, all you 
had to say is that I should put more efforts with no constructive 
comment. If you have valid technical comments, I'd like to hear them.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  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).
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.

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

[Jacek Caban]

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

Jacek

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

[Dmitry Timoshkov]

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

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

[Jacek Caban]

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

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

[Dmitry Timoshkov]

Jacek Caban  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:
> > - 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?

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

[Jacek Caban]

On 03/01/16 15:19, Dmitry Timoshkov wrote:
> Jacek Caban  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.

I already explained to you that it would not test anything that's not
already tested. We have owner tests for various CreateWindowEx and
SetParent combinations in win.c and we have modal dialog tests that have
regular (as in not reparented) top level owner in msg.c.
> > > > > 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:
> > > - 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?

I don't see how it's better, but sure, if Alexandre shares your opinion,
I will change it.

Jacek

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

[Alexandre Julliard]

Jacek Caban  writes:
> On 03/01/16 15:19, Dmitry Timoshkov wrote:
> > 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?
>
> I don't see how it's better, but sure, if Alexandre shares your opinion,
> I will change it.

Not a big deal, but yes, a helper function containing the message
sequence would probably look cleaner.

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

[Dmitry Timoshkov]

Jacek Caban  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.
>
> I already explained to you that it would not test anything that's not
> already tested. We have owner tests for various CreateWindowEx and
> SetParent combinations in win.c and we have modal dialog tests that have
> regular (as in not reparented) top level owner in msg.c.

I don't understand why you're so much persistent about adding another test
that basically replicates what your new tests do, but with different parent.
Such a test clearly doesn't exist, and it would extend the test coverage for
your fixes, it's not apparent at all that this new test won't break due to
them.

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

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Jacek Caban  wrote:
> > I'm not sure I agree.
>
> Please don't get me wrong, but when sending tests after the fixes the result
> can't be easily verified, and therefore this reduces the trust and value of
> patches.

There are many cases where adding todos and then removing them creates
more work, so it's perfectly acceptable to send a test to apply after
the fix to demonstrate that it is correct. We do that all the time.

If you don't trust that the test is testing the right thing, it's easy
enough to verify by reverting the fix and confirming that the test
fails. I do that frequently myself.

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

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > Jacek Caban  wrote:
> > > I'm not sure I agree.
> >
> > Please don't get me wrong, but when sending tests after the fixes the result
> > can't be easily verified, and therefore this reduces the trust and value of
> > patches.
>
> There are many cases where adding todos and then removing them creates
> more work, so it's perfectly acceptable to send a test to apply after
> the fix to demonstrate that it is correct. We do that all the time.

In this case just adding and then removing TRUE in ok_sequence() call
and adding and then removing appropriate todo_wine statements shouldn't
create too much additional work, after all this work should be done
anyway in order to verify that the patch actually works.
> If you don't trust that the test is testing the right thing, it's easy
> enough to verify by reverting the fix and confirming that the test
> fails. I do that frequently myself.

As I have said, the patches that remove todo_wine statements are easier
recognizable as the fixes.