user32: Fix error handling in {Begin,End,}DeferWindowPos() to match Windows behavior (resend)
Sebastian Lackner
sebastian at fds-team.de
Tue Aug 25 09:31:06 CDT 2015
I also think moving the tests to an existing test file would be better, but here a few
additional comments:
On 25.08.2015 15:08, Rodrigo Rivas wrote:
> diff --git a/dlls/user32/tests/Makefile.in b/dlls/user32/tests/Makefile.in
> index 7149dc8..982b0fe 100644
> --- a/dlls/user32/tests/Makefile.in
> +++ b/dlls/user32/tests/Makefile.in
> @@ -9,6 +9,7 @@ C_SRCS = \
> cursoricon.c \
> dce.c \
> dde.c \
> + defer.c \
> dialog.c \
> edit.c \
> generated.c \
> diff --git a/dlls/user32/tests/defer.c b/dlls/user32/tests/defer.c
> new file mode 100644
> index 0000000..eb63384
> --- /dev/null
> +++ b/dlls/user32/tests/defer.c
> @@ -0,0 +1,168 @@
> +/*
> + * Unit tests for DeferWindowPos
> + *
> + * Copyright 2015 Rodrigo Rivas Costa
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#include "windows.h"
> +
> +#include "wine/test.h"
> +
> +
> +#define WIDTH_CHILDREN 50
> +#define NCHILDREN 3
> +#define IDX_FAIL 1
> +
> +static HINSTANCE g_hInstance;
This is not a DLL, so theoretically no need to use a static variable for that.
The overhead of calling GetModuleHandle multiple times should be very small.
> +static HWND hChildren[NCHILDREN];
> +
> +static void ExpectY(HWND hwnd, int y)
> +{
> + RECT r;
> + POINT p;
> + GetWindowRect(hwnd, &r);
> + p.x = r.left;
> + p.y = r.top;
> + MapWindowPoints(NULL, GetParent(hwnd), &p, 1);
> + ok(p.y == y, "Window has not been properly moved: %d != %d\n", (int)p.y, y);
I didn't test it, but there should be no need to explicitly cast here.
> +}
Such a helper function will make it complicated to track down test failures.
In this case the code doesn't look too long, so you could directly duplicate it into
the individual test functions. The alternative would be to pass the line number to
the function, and then use ok_(...) instead. You can take a look at existing tests how
to do that.
> +
> +static void TestDeferWindowPos(int posY)
You could split each test function into a separate patch (failing tests marked with
"todo_wine"), and then send the fix as a forth patch at the end. It is not strictly
necessary, but it would probably be easier for reviewing.
> +{
> + int i;
> + BOOL res;
> + HDWP hdwp;
> +
> + hdwp = BeginDeferWindowPos(1);
> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + {
> + HWND h = hChildren[i];
> + hdwp = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
> + ok(hdwp != NULL, "DeferWindowPos failed\n");
> + }
> + res = EndDeferWindowPos(hdwp);
> + ok(res, "EndDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + ExpectY(hChildren[i], posY);
> +}
> +
> +static void TestBogusDeferWindowPos(int posY)
> +{
> + int i;
> + BOOL res;
> + HDWP hdwp;
> +
> + hdwp = BeginDeferWindowPos(1);
> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + {
> + HDWP hnext;
> + HWND h = hChildren[i];
> + if (i==IDX_FAIL)
The style (missing spaces inbetween) is a bit inconsistent here, and also at a couple of places below.
> + h = (HWND)0; //Insert an error. This is not a valid HWND.
C++ Style comments are not allowed in Wine source. Also, you can use NULL instead of casting 0 to a HWND.
> + hnext = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
> + if (i==IDX_FAIL)
> + {
> + DWORD err = GetLastError();
> + ok(hnext == NULL, "DeferWindowPos accepted a bogus HWND\n");
> + ok(err == ERROR_INVALID_WINDOW_HANDLE, "Bogus HWND should return ERROR_INVALID_WINDOW_HANDLE, got %d\n", (int)err);
Also no need to explicitly cast here.
> + }
> + else
> + {
> + ok(hnext != NULL, "DeferWindowPos failed\n");
> + hdwp = hnext;
> + }
> + }
> + res = EndDeferWindowPos(hdwp);
> + ok(res, "EndDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + {
> + if (i != IDX_FAIL)
> + ExpectY(hChildren[i], posY);
> + }
> +}
> +
> +static void TestDestroyDeferWindowPos(int posY)
> +{
> + int i;
> + BOOL res;
> + HDWP hdwp;
> +
> + hdwp = BeginDeferWindowPos(1);
> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + {
> + HWND h = hChildren[i];
> + hdwp = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
> + ok(hdwp != NULL, "DeferWindowPos failed\n");
> + /* Destroying a window after DeferWindowPos but before EndDeferWindowPos should be silently ignored */
> + if (i == IDX_FAIL)
> + DestroyWindow(h);
> + }
> + res = EndDeferWindowPos(hdwp);
> + ok(res, "EndDeferWindowPos failed\n");
> + for (i = 0; i < NCHILDREN; ++i)
> + {
> + if (i != IDX_FAIL)
> + ExpectY(hChildren[i], posY);
> + }
> +}
> +
> +static LRESULT CALLBACK TestWndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
> +{
> + switch (msg)
> + {
> + case WM_CREATE:
> + {
> + int i;
> + int posY = 0;
> + for (i=0; i < NCHILDREN; ++i)
> + {
> + hChildren[i] = CreateWindowExA(0, "Button", NULL, WS_CHILD|WS_VISIBLE,
> + i*WIDTH_CHILDREN, posY, WIDTH_CHILDREN, WIDTH_CHILDREN,
> + hWnd, NULL, g_hInstance, NULL);
> + }
> +
> + TestDeferWindowPos(++posY);
> + TestBogusDeferWindowPos(++posY);
> + TestDestroyDeferWindowPos(++posY);
If possible you should move those calls outside of the TestWndProc.
> + }
> + break;
> + }
> + return DefWindowProcA(hWnd, msg, wParam, lParam);
> +}
> +
> +START_TEST(defer)
> +{
> + WNDCLASSEXA cls;
> + ATOM acls;
> +
> + g_hInstance = GetModuleHandleA(NULL);
> + memset(&cls, 0, sizeof(cls));
> + cls.cbSize = sizeof(cls);
> + cls.lpfnWndProc = TestWndProc;
> + cls.hInstance = g_hInstance;
> + cls.lpszClassName = "TestWnd";
> +
> + acls = RegisterClassExA(&cls);
> +
> + HWND hWnd = CreateWindowExA(0, MAKEINTRESOURCEA(acls), "TestWnd", WS_OVERLAPPEDWINDOW,
You can't declare a variable in the middle of a function (should give you a warning while compiling).
> + CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
> + NULL, NULL, g_hInstance, NULL);
> + ok(hWnd != NULL, "CreateWindowEx failed\n");
> +}
> diff --git a/dlls/user32/winpos.c b/dlls/user32/winpos.c
> index f92a3dc..7cb3a4f 100644
> --- a/dlls/user32/winpos.c
> +++ b/dlls/user32/winpos.c
> @@ -2349,6 +2349,11 @@ HDWP WINAPI DeferWindowPos( HDWP hdwp, HWND hwnd, HWND hwndAfter,
>
> hwnd = WIN_GetFullHandle( hwnd );
> if (is_desktop_window( hwnd )) return 0;
> + if (!IsWindow( hwnd ))
> + {
> + SetLastError( ERROR_INVALID_WINDOW_HANDLE );
> + return 0;
> + }
>
> if (!(pDWP = get_user_handle_ptr( hdwp, USER_DWP ))) return 0;
> if (pDWP == OBJ_OTHER_PROCESS)
> @@ -2418,7 +2423,6 @@ BOOL WINAPI EndDeferWindowPos( HDWP hdwp )
> {
> DWP *pDWP;
> WINDOWPOS *winpos;
> - BOOL res = TRUE;
> int i;
>
> TRACE("%p\n", hdwp);
> @@ -2430,20 +2434,20 @@ BOOL WINAPI EndDeferWindowPos( HDWP hdwp )
> return FALSE;
> }
>
> - for (i = 0, winpos = pDWP->winPos; res && i < pDWP->actualCount; i++, winpos++)
> + for (i = 0, winpos = pDWP->winPos; i < pDWP->actualCount; i++, winpos++)
> {
> TRACE("hwnd %p, after %p, %d,%d (%dx%d), flags %08x\n",
> winpos->hwnd, winpos->hwndInsertAfter, winpos->x, winpos->y,
> winpos->cx, winpos->cy, winpos->flags);
>
> if (WIN_IsCurrentThread( winpos->hwnd ))
> - res = USER_SetWindowPos( winpos );
> + USER_SetWindowPos( winpos );
> else
> - res = SendMessageW( winpos->hwnd, WM_WINE_SETWINDOWPOS, 0, (LPARAM)winpos );
> + SendMessageW( winpos->hwnd, WM_WINE_SETWINDOWPOS, 0, (LPARAM)winpos );
> }
> HeapFree( GetProcessHeap(), 0, pDWP->winPos );
> HeapFree( GetProcessHeap(), 0, pDWP );
> - return res;
> + return TRUE;
> }
More information about the wine-devel
mailing list