[PATCH v2 0/1] MR6737: comctl32/toolbar: forward unhandled WM_NOTIFY

[Alanas (＠BZZZZ)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57178

This makes [7-zip file manager](https://www.7-zip.org/) pressing enter while editing path work:
* before:
  * [Icon near path becomes dark (7-zip file manager doesn't go to path).](https://old.reddit.com/r/winehq/comments/16ah3ze/7zip_cant_enter_path_from_p...)
* after:
  * 7-zip file manager goes to path.
  * Path input displays incorrect path (shouldn't happen, bug in wine's comboboxex unrelated to toolbar).

--
  v2: comctl32/toolbar: forward unhandled WM_NOTIFY
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737

[PATCH v2 1/1] comctl32/toolbar: forward unhandled WM_NOTIFY

[Alanas]

From: Alanas <alanas.00(a)mail.ru>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57178
---
 dlls/comctl32/tests/toolbar.c | 46 +++++++++++++++++++++++++++++++++--
 dlls/comctl32/toolbar.c       |  2 +-
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/dlls/comctl32/tests/toolbar.c b/dlls/comctl32/tests/toolbar.c
index 7c886e1f88f..e86852d30bb 100644
--- a/dlls/comctl32/tests/toolbar.c
+++ b/dlls/comctl32/tests/toolbar.c
@@ -179,7 +179,24 @@ static BOOL equal_dc(HDC hdc1, HDC hdc2, int width, int height)
 
 static void *alloced_str;
 
-static LRESULT parent_wnd_notify(LPARAM lParam)
+#ifdef _WIN64
+#define forward_test_idFrom 0x466e20495bc54f0d
+#define forward_test_return 0xdef293ec10fc0b87
+#else
+#define forward_test_idFrom 0xe7f4c2cc
+#define forward_test_return 0xa66b3c5a
+#endif
+#define forward_test_code 0x61a59ebb
+static _Bool forward_test;
+static const NMHDR forward_test_nmhdr = {
+    // .hwndFrom should not be accessed
+    .hwndFrom = NULL,
+    // .idFrom and .code are random and don't mean anything
+    .idFrom = forward_test_idFrom,
+    .code = forward_test_code
+};
+
+static LRESULT parent_wnd_notify(WPARAM wParam, LPARAM lParam)
 {
     NMHDR *hdr = (NMHDR *)lParam;
     NMTBHOTITEM *nmhi;
@@ -371,6 +388,12 @@ static LRESULT parent_wnd_notify(LPARAM lParam)
 
             return 0;
         }
+        case forward_test_code:
+            ok(hdr == &forward_test_nmhdr, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && (NMHDR*)lParam != &forward_test_nmhdr\n");
+            ok(forward_test, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && !forward_test\n");
+            ok(wParam == forward_test_idFrom, "wParam == 0x%Ix\n", wParam);
+            forward_test = FALSE;
+            return forward_test_return;
     }
     return 0;
 }
@@ -428,7 +451,7 @@ static LRESULT CALLBACK parent_wnd_proc(HWND hWnd, UINT message, WPARAM wParam,
     switch (message)
     {
         case WM_NOTIFY:
-            return parent_wnd_notify(lParam);
+            return parent_wnd_notify(wParam, lParam);
     }
 
     defwndproc_counter++;
@@ -2833,6 +2856,24 @@ static void test_BTNS_SEP(void)
     DestroyWindow(hwnd);
 }
 
+static void test_WM_NOTIFY_forwarding_to_parent(void)
+{
+    HWND toolbar = NULL;
+    LRESULT ret;
+#ifdef _WIN64
+    const WPARAM unused_wParam = 0x9c15b293ef1d1a25;
+#else
+    const WPARAM unused_wParam = 0x5d0b6232;
+#endif
+    rebuild_toolbar(&toolbar);
+    forward_test = TRUE;
+    ret = SendMessageA(toolbar, WM_NOTIFY, unused_wParam, (LPARAM)&forward_test_nmhdr);
+    ok(ret == forward_test_return, "SendMessageA returned 0x%Ix\n", ret);
+    ok(!forward_test, "toolbar didn't forward WM_NOTIFY to parent\n");
+    forward_test = FALSE;
+    DestroyWindow(toolbar);
+}
+
 START_TEST(toolbar)
 {
     ULONG_PTR ctx_cookie;
@@ -2884,6 +2925,7 @@ START_TEST(toolbar)
     test_drawtext_flags();
     test_imagelist();
     test_BTNS_SEP();
+    test_WM_NOTIFY_forwarding_to_parent();
 
     if (!load_v6_module(&ctx_cookie, &ctx))
         return;
diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
index 690a02db6ee..d12cd25715c 100644
--- a/dlls/comctl32/toolbar.c
+++ b/dlls/comctl32/toolbar.c
@@ -6360,7 +6360,7 @@ TOOLBAR_Notify (TOOLBAR_INFO *infoPtr, LPNMHDR lpnmh)
         return 0;
 
     default:
-        return 0;
+        return SendMessageW(infoPtr->hwndNotify, WM_NOTIFY, lpnmh->idFrom, (LPARAM)lpnmh);
     }
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/6737

[PATCH v2 1/1] comctl32/toolbar: forward unhandled WM_NOTIFY

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=149343

Your paranoid android.


=== debian11b (64 bit WoW report) ===

Report validation errors:
winmm:mci crashed (c0000008)

[Alanas (＠BZZZZ)]

On Sun Oct 27 18:19:09 2024 +0000, Nikolay Sivov wrote:
> That's from test_BTNS_SEP().
> I added `test_WM_NOTIFY_forwarding_to_parent` (tested in wine x86-64 and windows xp professional x64 edition service pack 2).

I didn't fix `test_BTNS_SEP`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86211

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> -static LRESULT parent_wnd_notify(LPARAM lParam)
> +#ifdef _WIN64
> +#define forward_test_idFrom 0x466e20495bc54f0d
> +#define forward_test_return 0xdef293ec10fc0b87
> +#else
> +#define forward_test_idFrom 0xe7f4c2cc
> +#define forward_test_return 0xa66b3c5a
> +#endif
> +#define forward_test_code 0x61a59ebb
> +static _Bool forward_test;
> +static const NMHDR forward_test_nmhdr = {
> +    // .hwndFrom should not be accessed
> +    .hwndFrom = NULL,
> +    // .idFrom and .code are random and don't mean anything
> +    .idFrom = forward_test_idFrom,
> +    .code = forward_test_code
> forward_test_code is not really used anywhere.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86278

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> static void *alloced_str;
>
> -static LRESULT parent_wnd_notify(LPARAM lParam)
> +#ifdef _WIN64
> +#define forward_test_idFrom 0x466e20495bc54f0d
> +#define forward_test_return 0xdef293ec10fc0b87
> +#else
> +#define forward_test_idFrom 0xe7f4c2cc
> +#define forward_test_return 0xa66b3c5a
> +#endif
> +#define forward_test_code 0x61a59ebb
> +static _Bool forward_test;
> +static const NMHDR forward_test_nmhdr = {
> +    // .hwndFrom should not be accessed

Please use /**/ for comments/

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86279

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> return 0;
>          }
> +        case forward_test_code:
> +            ok(hdr == &forward_test_nmhdr, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && (NMHDR*)lParam != &forward_test_nmhdr\n");
> +            ok(forward_test, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && !forward_test\n");

Let do these checks when forward_test == TRUE, instead of putting the check in a ok().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86281

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> static void *alloced_str;
>
> -static LRESULT parent_wnd_notify(LPARAM lParam)
> +#ifdef _WIN64
> +#define forward_test_idFrom 0x466e20495bc54f0d

I would prefer using something like 0xdeadbeef and use the same value for 32bit and 64bit.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86280

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> DestroyWindow(hwnd);
>  }
>
> +static void test_WM_NOTIFY_forwarding_to_parent(void)
> +{
> +    HWND toolbar = NULL;
> +    LRESULT ret;
> +#ifdef _WIN64
> +    const WPARAM unused_wParam = 0x9c15b293ef1d1a25;
> +#else
> +    const WPARAM unused_wParam = 0x5d0b6232;

wparam should be from GetDlgCtrlID() or setting it to 1 or 0 is enough because the you're only expecting it to be different.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86282

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> test_drawtext_flags();
>      test_imagelist();
>      test_BTNS_SEP();
> +    test_WM_NOTIFY_forwarding_to_parent();

test_WM_NOTIFY() is enough.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86283

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/toolbar.c:
> test_drawtext_flags();
>      test_imagelist();
>      test_BTNS_SEP();

Please add the tests first in a separate patch. Use todo_wine to mark the failure and then remove the todo_wine after you fix the bug.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86284

[Zhiyi Zhang (＠zhiyi)]

On Tue Oct 29 10:07:54 2024 +0000, Zhiyi Zhang wrote:
> I would prefer using something like 0xdeadbeef and use the same value
> for 32bit and 64bit.
> Same for other values. These values are too random and it's making the tests a bit messy.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86285

[Alanas (＠BZZZZ)]

On Tue Oct 29 10:02:53 2024 +0000, Zhiyi Zhang wrote:
> forward_test_code is not really used anywhere.
> `forward_test_code` is used!

search `case forward_test_code:`

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86286

[Alanas (＠BZZZZ)]

> use the same value for 32bit and 64bit

32 most significant bits will be not tested

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86287

[Alanas (＠BZZZZ)]

> Use todo_wine to mark the failure

Which failure?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86288

[Alanas (＠BZZZZ)]

On Tue Oct 29 10:07:54 2024 +0000, Zhiyi Zhang wrote:
> Let do these checks when forward_test == TRUE, instead of putting the
> check in a ok().
> like this?
if (forward_test) {
    ok(hdr == &forward_test_nmhdr, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && (NMHDR*)lParam != &forward_test_nmhdr\n");
    ok(wParam == forward_test_idFrom, "wParam == 0x%Ix\n", wParam);
} else {
    ok(FALSE, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code == forward_test_code && !forward_test\n");
}

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86289

[Alanas (＠BZZZZ)]

> Same for other values.

Do you want to use same number for everything?

This is not good for testing (test will not fail with buggy code that reads wrong field):
```c
static const NMHDR forward_test_nmhdr = {
    .hwndFrom = (HWND)0xdeadbeef,
    .idFrom = 0xdeadbeef,
    .code = 0xdeadbeef
};
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86293

[Zhiyi Zhang (＠zhiyi)]

On Tue Oct 29 13:42:14 2024 +0000, Alanas wrote:
> `forward_test_code` is used!
> search `case forward_test_code:`
> I see.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86294

[Zhiyi Zhang (＠zhiyi)]

On Tue Oct 29 11:34:29 2024 +0000, Alanas wrote:
> > Use todo_wine to mark the failure
> ~Which failure?~
> > Please add the tests **first** in a separate patch.
> Do you mean:
> 1. add `test_WM_NOTIFY` that fails in wine but successful in windows
> 2. edit `dlls/comctl32/toolbar.c` to make test successful in wine
Yes, when you add test_WM_NOTIFY() before your fix, there will be a test failure on Wine.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86298

[Zhiyi Zhang (＠zhiyi)]

> 32 most significant bits will be not tested

I don't think it's important in this case.

Something like the following seems better.
```c
static const NMHDR forward_test_nmhdr = {
    .hwndFrom = (HWND)0xabcd0001,
    .idFrom = 0xabcd0002,
    .code = 0xabcd0003
};
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86299

[Zhiyi Zhang (＠zhiyi)]

On Tue Oct 29 10:44:55 2024 +0000, Alanas wrote:
> like this?
> ```c
> if (forward_test) {
>     ok(hdr == &forward_test_nmhdr, "didn't expect WM_NOTIFY message
> ((NMHDR*)lParam)->code == forward_test_code && (NMHDR*)lParam != &forward_test_nmhdr\n");
>     ok(wParam == forward_test_idFrom, "wParam == 0x%Ix\n", wParam);
> } else {
>     ok(FALSE, "didn't expect WM_NOTIFY message ((NMHDR*)lParam)->code ==
> forward_test_code && !forward_test\n");
> }
> ```
> Sort of, use the following code.
if (forward_test)
{
    ok(hdr == &forward_test_nmhdr, "Got unexpected header.\n");
    ok(wParam == forward_test_idFrom, "Got unexpected wParam 0x%Ix.\n", wParam);
}
else
{
    ok(FALSE, "Got unexpected WM_NOTIFY.\n");
}

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6737#note_86300