[PATCH v2] user32: Do not call SendMessage() to show a window that is already visible.

[Alexey Prokhin]

Based on a patch made by Kimmo Myllyvirta.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=39731
Signed-off-by: Alexey Prokhin <alexey(a)prokhin.ru>
---
v2: handle all ShowWindow commands, add a better test case. Supersede 166154
---
 dlls/user32/tests/msg.c | 116 ++++++++++++++++++++++++++++++----------
 dlls/user32/winpos.c    |  64 +++++++++++++++++++---
 2 files changed, 146 insertions(+), 34 deletions(-)

diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c
index a5bc1c5518..8aa00d5ba0 100644
--- a/dlls/user32/tests/msg.c
+++ b/dlls/user32/tests/msg.c
@@ -5041,16 +5041,6 @@ static void test_WM_DEVICECHANGE(HWND hwnd)
     }
 }
 
-static DWORD CALLBACK show_window_thread(LPVOID arg)
-{
-   HWND hwnd = arg;
-
-   /* function will not return if ShowWindow(SW_HIDE) calls SendMessage() */
-   ok(ShowWindow(hwnd, SW_HIDE) == FALSE, "ShowWindow(SW_HIDE) expected FALSE\n");
-
-   return 0;
-}
-
 /* Helper function to easier test SetWindowPos messages */
 #define test_msg_setpos( expected_list, flags, todo ) \
         test_msg_setpos_( (expected_list), (flags), (todo), __FILE__, __LINE__)
@@ -5071,8 +5061,6 @@ static void test_msg_setpos_(const struct message *expected_list, UINT flags, BO
 /* test if we receive the right sequence of messages */
 static void test_messages(void)
 {
-    DWORD tid;
-    HANDLE hthread;
     HWND hwnd, hparent, hchild;
     HWND hchild2, hbutton;
     HMENU hmenu;
@@ -5111,14 +5099,6 @@ static void test_messages(void)
     flush_events();
     ok_sequence(WmEmptySeq, "ShowWindow(SW_HIDE):overlapped", FALSE);
 
-    /* test ShowWindow(SW_HIDE) on a hidden window -  multi-threaded */
-    hthread = CreateThread(NULL, 0, show_window_thread, hwnd, 0, &tid);
-    ok(hthread != NULL, "CreateThread failed, error %d\n", GetLastError());
-    ok(WaitForSingleObject(hthread, INFINITE) == WAIT_OBJECT_0, "WaitForSingleObject failed\n");
-    CloseHandle(hthread);
-    flush_events();
-    ok_sequence(WmEmptySeq, "ShowWindow(SW_HIDE):overlapped", FALSE);
-
     ShowWindow(hwnd, SW_SHOW);
     flush_events();
     ok_sequence(WmShowOverlappedSeq, "ShowWindow(SW_SHOW):overlapped", TRUE);
@@ -13180,6 +13160,14 @@ static const struct message WmShowMaximized_3[] = {
     { 0 }
 };
 
+static const char * const sw_cmd_name[13] =
+{
+    "SW_HIDE", "SW_SHOWNORMAL", "SW_SHOWMINIMIZED", "SW_SHOWMAXIMIZED",
+    "SW_SHOWNOACTIVATE", "SW_SHOW", "SW_MINIMIZE", "SW_SHOWMINNOACTIVE",
+    "SW_SHOWNA", "SW_RESTORE", "SW_SHOWDEFAULT", "SW_FORCEMINIMIZE",
+    "SW_NORMALNA" /* 0xCC */
+};
+
 static void test_ShowWindow(void)
 {
     /* ShowWindow commands in random order */
@@ -13366,13 +13354,6 @@ static void test_ShowWindow(void)
 
     for (i = 0; i < ARRAY_SIZE(sw); i++)
     {
-        static const char * const sw_cmd_name[13] =
-        {
-            "SW_HIDE", "SW_SHOWNORMAL", "SW_SHOWMINIMIZED", "SW_SHOWMAXIMIZED",
-            "SW_SHOWNOACTIVATE", "SW_SHOW", "SW_MINIMIZE", "SW_SHOWMINNOACTIVE",
-            "SW_SHOWNA", "SW_RESTORE", "SW_SHOWDEFAULT", "SW_FORCEMINIMIZE",
-            "SW_NORMALNA" /* 0xCC */
-        };
         char comment[64];
         INT idx; /* index into the above array of names */
 
@@ -13421,6 +13402,86 @@ if (0) /* FIXME: Wine behaves completely different here */
     flush_events();
 }
 
+struct show_window_thread_data
+{
+    const char *comment;
+    HWND hwnd;
+    int cmd;
+    BOOL ret;
+};
+
+static DWORD CALLBACK show_window_thread(LPVOID arg)
+{
+   struct show_window_thread_data *d = arg;
+   BOOL ret;
+
+   /* function will not return if ShowWindow() calls SendMessage() */
+   ret = ShowWindow(d->hwnd, d->cmd);
+   ok(!ret == !d->ret, "%s: expected ret %u, got %u\n", d->comment, d->ret, ret);
+
+   return 0;
+}
+
+static void test_ShowWindow_multithreaded(void)
+{
+    static const struct
+    {
+        INT initial_cmd; /* ShowWindow command to apply before the test */
+        INT cmd; /* ShowWindow command to apply in a thread */
+        BOOL ret; /* ShowWindow return value */
+    } sw[] = {
+        { SW_HIDE, SW_HIDE, FALSE },
+        { SW_RESTORE, SW_SHOW, TRUE },
+        { SW_RESTORE, SW_SHOWNOACTIVATE, TRUE },
+        { SW_RESTORE, SW_RESTORE, TRUE },
+        { SW_RESTORE, SW_SHOWNORMAL, TRUE },
+        { SW_RESTORE, SW_SHOWDEFAULT, TRUE },
+        { SW_MINIMIZE, SW_SHOW, TRUE },
+        { SW_MINIMIZE, SW_SHOWMINNOACTIVE, TRUE },
+        { SW_MINIMIZE, SW_MINIMIZE, TRUE },
+        { SW_MINIMIZE, SW_SHOWMINIMIZED, TRUE },
+        { SW_MINIMIZE, SW_FORCEMINIMIZE, TRUE },
+        { SW_SHOWMAXIMIZED, SW_SHOW, TRUE },
+        { SW_SHOWMAXIMIZED, SW_SHOWMAXIMIZED, TRUE },
+    };
+
+    DWORD tid;
+    HANDLE hthread;
+    HWND hwnd;
+    int i;
+    struct show_window_thread_data swtd;
+
+    hwnd = CreateWindowExA(0, "ShowWindowClass", NULL, WS_OVERLAPPEDWINDOW,
+                          120, 120, 90, 90,
+                          0, 0, 0, NULL);
+    assert(hwnd);
+
+    for (i = 0; i < ARRAY_SIZE(sw); ++i)
+    {
+        char comment[64];
+        sprintf(comment, "%d: ShowWindow(%s)", i+1, sw_cmd_name[sw[i].cmd]);
+
+        ShowWindow(hwnd, sw[i].initial_cmd);
+        flush_events();
+        flush_sequence();
+
+        swtd.comment = comment;
+        swtd.hwnd = hwnd;
+        swtd.cmd = sw[i].cmd;
+        swtd.ret = sw[i].ret;
+        hthread = CreateThread(NULL, 0, show_window_thread, &swtd, 0, &tid);
+        ok(hthread != NULL, "%d: CreateThread failed, error %d\n", i, GetLastError());
+        ok(WaitForSingleObject(hthread, 3000) == WAIT_OBJECT_0, "%d: WaitForSingleObject failed\n", i);
+        CloseHandle(hthread);
+
+        flush_events();
+        ok_sequence(WmOptionalPaint, comment, FALSE);
+    }
+
+    DestroyWindow(hwnd);
+    flush_events();
+}
+
 static INT_PTR WINAPI test_dlg_proc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
 {
     struct recvd_message msg;
@@ -17748,6 +17809,7 @@ START_TEST(msg)
     test_PostMessage();
     test_broadcast();
     test_ShowWindow();
+    test_ShowWindow_multithreaded();
     test_PeekMessage();
     test_PeekMessage2();
     test_PeekMessage3();
diff --git a/dlls/user32/winpos.c b/dlls/user32/winpos.c
index 5837c179e4..96f86c4aeb 100644
--- a/dlls/user32/winpos.c
+++ b/dlls/user32/winpos.c
@@ -1032,6 +1032,51 @@ UINT WINPOS_MinMaximize( HWND hwnd, UINT cmd, LPRECT rect )
     return swpFlags;
 }
 
+/***********************************************************************
+ *          is_window_state_changed
+ *
+ * A helper function for ShowWindow and ShowWindowAsync for checking if
+ * a window already has the requested style.
+ */
+static BOOL is_window_state_changed( LONG style, INT cmd )
+{
+    BOOL is_visible = (style & WS_VISIBLE) != 0;
+
+    switch (cmd) {
+    case SW_HIDE:
+        if (!is_visible)
+            return FALSE;
+        break;
+    case SW_SHOW:
+        if (is_visible)
+            return FALSE;
+        break;
+    case SW_SHOWMINNOACTIVE:
+    case SW_MINIMIZE:
+    case SW_FORCEMINIMIZE:
+    case SW_SHOWMINIMIZED:
+        if (is_visible && (style & WS_MINIMIZE))
+            return FALSE;
+        break;
+    case SW_SHOWMAXIMIZED: /* same as SW_MAXIMIZE */
+        if (is_visible && (style & WS_MAXIMIZE))
+            return FALSE;
+        break;
+    case SW_SHOWNA:
+        break;
+    case SW_SHOWNOACTIVATE:
+    case SW_RESTORE:
+    case SW_SHOWNORMAL: /* same as SW_NORMAL: */
+    case SW_SHOWDEFAULT:
+        if (is_visible && !(style & (WS_MINIMIZE | WS_MAXIMIZE)))
+            return FALSE;
+        break;
+    default:
+        return FALSE;
+    }
+
+    return TRUE;
+}
 
 /***********************************************************************
  *              show_window
@@ -1056,7 +1101,6 @@ static BOOL show_window( HWND hwnd, INT cmd )
     switch(cmd)
     {
         case SW_HIDE:
-            if (!wasVisible) goto done;
             showFlag = FALSE;
             swp |= SWP_HIDEWINDOW | SWP_NOSIZE | SWP_NOMOVE;
             if (style & WS_CHILD) swp |= SWP_NOACTIVATE | SWP_NOZORDER;
@@ -1070,14 +1114,12 @@ static BOOL show_window( HWND hwnd, INT cmd )
 	case SW_SHOWMINIMIZED:
             swp |= SWP_SHOWWINDOW | SWP_FRAMECHANGED;
             swp |= WINPOS_MinMaximize( hwnd, cmd, &newPos );
-            if ((style & WS_MINIMIZE) && wasVisible) goto done;
 	    break;
 
 	case SW_SHOWMAXIMIZED: /* same as SW_MAXIMIZE */
             if (!wasVisible) swp |= SWP_SHOWWINDOW;
             swp |= SWP_FRAMECHANGED;
             swp |= WINPOS_MinMaximize( hwnd, SW_MAXIMIZE, &newPos );
-            if ((style & WS_MAXIMIZE) && wasVisible) goto done;
             break;
 
 	case SW_SHOWNA:
@@ -1085,7 +1127,6 @@ static BOOL show_window( HWND hwnd, INT cmd )
             if (style & WS_CHILD) swp |= SWP_NOZORDER;
             break;
 	case SW_SHOW:
-            if (wasVisible) goto done;
 	    swp |= SWP_SHOWWINDOW | SWP_NOSIZE | SWP_NOMOVE;
             if (style & WS_CHILD) swp |= SWP_NOACTIVATE | SWP_NOZORDER;
 	    break;
@@ -1105,7 +1146,6 @@ static BOOL show_window( HWND hwnd, INT cmd )
             }
             else
             {
-                if (wasVisible) goto done;
                 swp |= SWP_NOSIZE | SWP_NOMOVE;
             }
             if (style & WS_CHILD && !(swp & SWP_STATECHANGED)) swp |= SWP_NOACTIVATE | SWP_NOZORDER;
@@ -1114,6 +1154,9 @@ static BOOL show_window( HWND hwnd, INT cmd )
             goto done;
     }
 
+    if (!is_window_state_changed( style, cmd ))
+        goto done;
+
     if ((showFlag != wasVisible || cmd == SW_SHOWNA) && cmd != SW_SHOWMAXIMIZED && !(swp & SWP_STATECHANGED))
     {
         SendMessageW( hwnd, WM_SHOWWINDOW, showFlag, 0 );
@@ -1199,6 +1242,7 @@ done:
 BOOL WINAPI ShowWindowAsync( HWND hwnd, INT cmd )
 {
     HWND full_handle;
+    LONG style;
 
     if (is_broadcast(hwnd))
     {
@@ -1209,6 +1253,10 @@ BOOL WINAPI ShowWindowAsync( HWND hwnd, INT cmd )
     if ((full_handle = WIN_IsCurrentThread( hwnd )))
         return show_window( full_handle, cmd );
 
+    style = GetWindowLongW( hwnd, GWL_STYLE );
+    if (!is_window_state_changed( style, cmd ))
+        return (style & WS_VISIBLE) != 0;
+
     return SendNotifyMessageW( hwnd, WM_WINE_SHOWWINDOW, cmd, 0 );
 }
 
@@ -1219,6 +1267,7 @@ BOOL WINAPI ShowWindowAsync( HWND hwnd, INT cmd )
 BOOL WINAPI ShowWindow( HWND hwnd, INT cmd )
 {
     HWND full_handle;
+    LONG style;
 
     if (is_broadcast(hwnd))
     {
@@ -1228,8 +1277,9 @@ BOOL WINAPI ShowWindow( HWND hwnd, INT cmd )
     if ((full_handle = WIN_IsCurrentThread( hwnd )))
         return show_window( full_handle, cmd );
 
-    if ((cmd == SW_HIDE) && !(GetWindowLongW( hwnd, GWL_STYLE ) & WS_VISIBLE))
-        return FALSE;
+    style = GetWindowLongW( hwnd, GWL_STYLE );
+    if (!is_window_state_changed( style, cmd ))
+        return (style & WS_VISIBLE) != 0;
 
     return SendMessageW( hwnd, WM_WINE_SHOWWINDOW, cmd, 0 );
 }
-- 
2.17.2 (Apple Git-113)

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=54297

Your paranoid android.


=== w1064v1809 (32 bit report) ===

user32:
msg.c:9314: Test failed: Shift+MouseButton press/release: 7: the msg 0x0202 was expected, but got msg 0x0007 instead
msg.c:9314: Test failed: Shift+MouseButton press/release: 8: the msg 0x0202 was expected, but got msg 0x0101 instead
msg.c:9314: Test failed: Shift+MouseButton press/release: 10: the msg 0x0101 should have been sent
msg.c:9314: Test failed: Shift+MouseButton press/release: 11: the msg 0x0101 was expected, but got msg 0x0202 instead
msg.c:9314: Test failed: Shift+MouseButton press/release: 12: the msg sequence is not complete: expected 0000 - actual 0202

=== debian9 (32 bit report) ===

user32:
msg.c:8693: Test failed: WaitForSingleObject failed 102
msg.c:8699: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
msg.c:8699: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
msg.c:8699: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000