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

[Dmitry Timoshkov]

Jacek Caban <jacek at codeweavers.com> 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.

-- 
Dmitry.