Re: user32/tests: Add some tests for dialog owner disabled state. (v3)

[Jacek Caban]

Hi Dmitry,

On 04/06/16 13:15, Dmitry Timoshkov wrote:
> Signed-off-by: Dmitry Timoshkov
>
> dlls/user32/tests/win.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c
> index 957e8a1d..528669e 100644
> --- a/dlls/user32/tests/win.c
> +++ b/dlls/user32/tests/win.c
> @@ -4258,6 +4258,78 @@ static INT_PTR WINAPI empty_dlg_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM
>      return 0;
>  }
>
> +static INT_PTR WINAPI empty_dlg_proc3(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
> +{
> - if (msg == WM_INITDIALOG)
> -
>    EndDialog(hwnd, 0);
> -
> - return 0;
>
> +}
> -
>
> +struct dialog_param
> +{
> - HWND parent, grand_parent;
> - DLGTEMPLATE *dlg_data;
>
> +};
> -
>
> +static INT_PTR WINAPI empty_dlg_proc2(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
> +{
> - if (msg == WM_INITDIALOG)
> - {
> -
>    DWORD style = GetWindowLongA(hwnd, GWL_STYLE);
> -
>    struct dialog_param *param = (struct dialog_param *)lparam;
> -
>    BOOL parent_is_child;
> -
>    HWND disabled_hwnd;
> -
> -
>    parent_is_child = (GetWindowLongA(param->parent, GWL_STYLE) & (WS_POPUP | WS_CHILD)) == WS_CHILD;
> -
> -
>    ok(IsWindowEnabled(hwnd), "wrong state for %p\n", hwnd);
> -
>    if (parent_is_child)
> -
>    {
> -
>        ok(IsWindowEnabled(param->parent), "wrong state for %08x\n", style);
> -
>        disabled_hwnd = param->grand_parent;
> -
>    }
> -
>    else
> -
>    {
> -
>        ok(!IsWindowEnabled(param->parent), "wrong state for %08x\n", style);
> -
>        disabled_hwnd = param->parent;
> -
>    }
> -
> -
>    if (param->grand_parent)
> -
>    {
> -
>        if (parent_is_child)
> -
>            ok(!IsWindowEnabled(param->grand_parent), "wrong state for %08x\n", style);
> -
>        else
> -
>            ok(IsWindowEnabled(param->grand_parent), "wrong state for %08x\n", style);
> -
>    }
> -
> -
>    DialogBoxIndirectParamA(GetModuleHandleA(NULL), param->dlg_data, disabled_hwnd, empty_dlg_proc3, 0);
> -
>    todo_wine_if ((style & (WS_CHILD|WS_POPUP)) == WS_CHILD)
> -
>    ok(IsWindowEnabled(disabled_hwnd), "wrong state for %08x\n", style);

If you ensured that disabled_hwnd is enabled here, then testing it
later, after dialog is done, has very little value. Maybe you'd disable
it by EnableWindow() call here?
> -
> -
>    ok(IsWindowEnabled(hwnd), "wrong state for %p\n", hwnd);
> -
>    ok(IsWindowEnabled(param->parent), "wrong state for %p\n", param->parent);
> -
>    if (param->grand_parent)
> -
>    todo_wine_if ((style & (WS_CHILD|WS_POPUP)) == WS_CHILD)
> -
>        ok(IsWindowEnabled(param->grand_parent), "wrong state for %p (%08x)\n", param->grand_parent, style);
> -
> -
>    DialogBoxIndirectParamA(GetModuleHandleA(NULL), param->dlg_data, hwnd, empty_dlg_proc3, 0);
> -
>    ok(IsWindowEnabled(hwnd), "wrong state for %p\n", hwnd);
> -
>    ok(IsWindowEnabled(param->parent), "wrong state for %p\n", param->parent);
> -
>    if (param->grand_parent)
> -
>    todo_wine_if ((style & (WS_CHILD|WS_POPUP)) == WS_CHILD)
> -
>        ok(IsWindowEnabled(param->grand_parent), "wrong state for %p (%08x)\n", param->grand_parent, style);

Why do you put those tests inside WM_INITDIALOG? Wouldn't separated
tests be cleaner?
> -
>    param->dlg_data->style |= WS_CHILD;
> -
>    DialogBoxIndirectParamA(GetModuleHandleA(NULL), param->dlg_data, hwnd, empty_dlg_proc3, 0);
> -
>    todo_wine_if (!(style & (WS_CHILD|WS_POPUP)))
> -
>    ok(IsWindowEnabled(hwnd), "wrong state for %p (%08x)\n", hwnd, style);
> -
> -
>    EndDialog(hwnd, 0);

Testing that EndDialog enables window would be nice (given that it's
disabled as I said above).
> - }
> - return 0;
>
> +}
> -
>
> static void check_dialog_style(DWORD style_in, DWORD ex_style_in, DWORD style_out, DWORD ex_style_out)
>  {
>      struct
> @@ -4269,11 +4341,19 @@ static void check_dialog_style(DWORD style_in, DWORD ex_style_in, DWORD style_ou
>          WCHAR caption[1];
>      } dlg_data;
>      DWORD style, ex_style;
> - HWND hwnd, parent = 0;
> -
>
> HWND hwnd, grand_parent = 0, parent = 0;
> -
>
> struct dialog_param param;
>
> if (style_in & WS_CHILD)
> -
>    parent = CreateWindowExA(0, "static", NULL, WS_OVERLAPPEDWINDOW,
> - {
> -
>    grand_parent = CreateWindowExA(0, "static", NULL, WS_OVERLAPPEDWINDOW,
>                            0, 0, 0, 0, NULL, NULL, NULL, NULL);
> -
>    ok(grand_parent != 0, "grand_parent creation failed\n");
> - }
> -
> - parent = CreateWindowExA(0, "static", NULL, style_in,
> -
>                         0, 0, 0, 0, grand_parent, NULL, NULL, NULL);
> - ok(parent != 0, "parent creation failed, style %#x\n", style_in);

Always creating window here changes existing tests. It's not a big deal,
I wouldn't expect it to change much, but still, the case that's
currently tested will no longer be tested.

Jacek

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Dmitry Timoshkov]

Jacek Caban  wrote:
> >  if (style_in & WS_CHILD)
> > -
> >    parent = CreateWindowExA(0, "static", NULL, WS_OVERLAPPEDWINDOW,
> > - {
> > -
> >    grand_parent = CreateWindowExA(0, "static", NULL, WS_OVERLAPPEDWINDOW,
> >                            0, 0, 0, 0, NULL, NULL, NULL, NULL);
> > -
> >    ok(grand_parent != 0, "grand_parent creation failed\n");
> > - }
> > -
> > - parent = CreateWindowExA(0, "static", NULL, style_in,
> > -
> >                         0, 0, 0, 0, grand_parent, NULL, NULL, NULL);
> > - ok(parent != 0, "parent creation failed, style %#x\n", style_in);
>
> Always creating window here changes existing tests. It's not a big deal,
> I wouldn't expect it to change much, but still, the case that's
> currently tested will no longer be tested.

The thing which currently is being tested are the dialog styles that
don't depend on the dialog parent.

Regarding any possible additional tests or moving some logic around:
as the patch subject says these tests don't pretend to be perfect or
exhaustive set of tests, they simply test some things I was interested
in, and as always with every test there are things that could be added
or changed.

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Regarding any possible additional tests or moving some logic around:
> as the patch subject says these tests don't pretend to be perfect or
> exhaustive set of tests, they simply test some things I was interested
> in, and as always with every test there are things that could be added
> or changed.

Of course, but could you please fix the few things that Jacek mentioned,
and update the todos that succeed now?  Thanks.

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > Regarding any possible additional tests or moving some logic around:
> > as the patch subject says these tests don't pretend to be perfect or
> > exhaustive set of tests, they simply test some things I was interested
> > in, and as always with every test there are things that could be added
> > or changed.
>
> Of course, but could you please fix the few things that Jacek mentioned,
> and update the todos that succeed now?  Thanks.

I can resend with just todos removed if that's ok, since changing the things
that Jacek pointed out (adding the tests after EndDialog, moving the tests
into another place or removing a test that  makes sure that window state has
not really changed) has no influence on the things being actually tested.

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > > Regarding any possible additional tests or moving some logic around:
> > > as the patch subject says these tests don't pretend to be perfect or
> > > exhaustive set of tests, they simply test some things I was interested
> > > in, and as always with every test there are things that could be added
> > > or changed.
> >
> > Of course, but could you please fix the few things that Jacek mentioned,
> > and update the todos that succeed now?  Thanks.
>
> I can resend with just todos removed if that's ok, since changing the things
> that Jacek pointed out (adding the tests after EndDialog, moving the tests
> into another place or removing a test that  makes sure that window state has
> not really changed) has no influence on the things being actually tested.

Maybe it would still be testing the same thing, but moving the tests out
of WM_INITDIALOG would make things clearer. Avoiding changing the window
creation of existing tests would be a good thing too.

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > > > Regarding any possible additional tests or moving some logic around:
> > > > as the patch subject says these tests don't pretend to be perfect or
> > > > exhaustive set of tests, they simply test some things I was interested
> > > > in, and as always with every test there are things that could be added
> > > > or changed.
> > >
> > > Of course, but could you please fix the few things that Jacek mentioned,
> > > and update the todos that succeed now?  Thanks.
> >
> > I can resend with just todos removed if that's ok, since changing the things
> > that Jacek pointed out (adding the tests after EndDialog, moving the tests
> > into another place or removing a test that  makes sure that window state has
> > not really changed) has no influence on the things being actually tested.
>
> Maybe it would still be testing the same thing, but moving the tests out
> of WM_INITDIALOG would make things clearer.

That would complicate things for no reason and wouldn't chage anything.
For instance moving the tests out of WM_INITDIALOG to WM_ENTERIDLE would
make the tests actually test different behaviour than it was originally
intended.
> Avoiding changing the window
> creation of existing tests would be a good thing too.

Since I am an original author of existing tests I don't think that adding
another hoop to jump trough (make the tests cope with NULL parent) would
change anything for existing tests since this wouldn't change the dialog
styles.

user32/tests: Add some tests for dialog owner disabled state. (v3)

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > Maybe it would still be testing the same thing, but moving the tests out
> > of WM_INITDIALOG would make things clearer.
>
> That would complicate things for no reason and wouldn't chage anything.
> For instance moving the tests out of WM_INITDIALOG to WM_ENTERIDLE would
> make the tests actually test different behaviour than it was originally
> intended.
> > Avoiding changing the window
> > creation of existing tests would be a good thing too.
>
> Since I am an original author of existing tests I don't think that adding
> another hoop to jump trough (make the tests cope with NULL parent) would
> change anything for existing tests since this wouldn't change the dialog
> styles.

Never mind then.

Could you please resubmit at least with the todos fixed?