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