[PATCH 0/1] MR6617: comctl32: Use correct clipping region when handling themed WM_NCPAINT in listview.
Same calculation is used in the DefWindowProc() handler. This also fixes handling of region == 1: GetDCEx(hwnd, 1, DCX_INTERSECTRGN) won't work. Probably same fix is needed for other comctl32 themed controls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Same calculation is used in the DefWindowProc() handler. This also fixes handling of region == 1: GetDCEx(hwnd, 1, DCX_INTERSECTRGN) won't work. Probably same fix is needed for other comctl32 themed controls. Spotted during testing of the CS_PARENTDC patches. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/comctl32/listview.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 13d1bd092f2..cedc44e5b6b 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -10706,14 +10706,22 @@ static LRESULT LISTVIEW_NCPaint(const LISTVIEW_INFO *infoPtr, HRGN region) if (!theme || !(exstyle & WS_EX_CLIENTEDGE)) return DefWindowProcW (infoPtr->hwndSelf, WM_NCPAINT, (WPARAM)region, 0); - GetWindowRect(infoPtr->hwndSelf, &r); + GetClientRect(infoPtr->hwndSelf, &r); + cliprgn = CreateRectRgn(r.left, r.top, r.right, r.bottom); + if (region > (HRGN)1) + { + CombineRgn(cliprgn, region, cliprgn, RGN_DIFF); + dc = GetDCEx(infoPtr->hwndSelf, cliprgn, DCX_USESTYLE | DCX_WINDOW | DCX_INTERSECTRGN); + } + else + dc = GetDCEx(infoPtr->hwndSelf, cliprgn, DCX_USESTYLE | DCX_WINDOW | DCX_EXCLUDERGN); + GetWindowRect(infoPtr->hwndSelf, &r); cliprgn = CreateRectRgn (r.left + cxEdge, r.top + cyEdge, r.right - cxEdge, r.bottom - cyEdge); if (region != (HRGN)1) CombineRgn (cliprgn, cliprgn, region, RGN_AND); - dc = GetDCEx(infoPtr->hwndSelf, region, DCX_WINDOW | DCX_INTERSECTRGN); if (infoPtr->hwndHeader && LISTVIEW_IsHeaderEnabled(infoPtr)) { GetWindowRect(infoPtr->hwndHeader, &window_rect); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6617
What does this fix visually? I guess the question is does this happen in any situation already, or some specific use case triggers this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84075
On Fri Oct 4 15:00:28 2024 +0000, Nikolay Sivov wrote:
What does this fix visually? I guess the question is does this happen in any situation already, or some specific use case triggers this. Visually listview erases parts of other windows of the same parent in winecfg.
As described in the commit message this was spotted during testing of the CS_PARENTDC patches. It looks like this bug was overlooked probably because current painting code in Wine doesn't trigger it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84076
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/listview.c:
if (!theme || !(exstyle & WS_EX_CLIENTEDGE)) return DefWindowProcW (infoPtr->hwndSelf, WM_NCPAINT, (WPARAM)region, 0);
- GetWindowRect(infoPtr->hwndSelf, &r); + GetClientRect(infoPtr->hwndSelf, &r); + cliprgn = CreateRectRgn(r.left, r.top, r.right, r.bottom); + if (region > (HRGN)1) + { + CombineRgn(cliprgn, region, cliprgn, RGN_DIFF); + dc = GetDCEx(infoPtr->hwndSelf, cliprgn, DCX_USESTYLE | DCX_WINDOW | DCX_INTERSECTRGN); + } + else + dc = GetDCEx(infoPtr->hwndSelf, cliprgn, DCX_USESTYLE | DCX_WINDOW | DCX_EXCLUDERGN);
Could you add some tests? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84314
On Tue Oct 8 09:43:17 2024 +0000, Zhiyi Zhang wrote:
Could you add some tests? What kind of tests would you like to see? Are you questioning correctness of the DefWindowProc(WM_NCPAINT) handler?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84316
What kind of tests would you like to see?
That listview draws correctly after this fix.
Are you questioning correctness of the DefWindowProc(WM_NCPAINT) handler?
No. The MR looks fine at first glance. I just think it would be better to have some tests. It shouldn't be too difficult to test each comctl32 control's WM_NCPAINT handler. I've seen many strange behaviors in comctl32 so I wouldn't be surprised if they don't do what DefWindowProc(WM_NCPAINT) does. I also don't want to block this. So feel free to remove me from the reviewers if you want. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84317
On Tue Oct 8 10:04:54 2024 +0000, Zhiyi Zhang wrote:
What kind of tests would you like to see? That listview draws correctly after this fix. Are you questioning correctness of the DefWindowProc(WM_NCPAINT) handler? No. The MR looks fine at first glance. I just think it would be better to have some tests. It shouldn't be too difficult to test each comctl32 control's WM_NCPAINT handler. I've seen many strange behaviors in comctl32 so I wouldn't be surprised if they don't do what DefWindowProc(WM_NCPAINT) does. I also don't want to block this. So feel free to remove me from the reviewers if you want. I don't see off hand how easily the correntness of listview painting could be tested. Suggestions are welcome.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84318
On Tue Oct 8 10:22:54 2024 +0000, Dmitry Timoshkov wrote:
I don't see off hand how easily the correntness of listview painting could be tested. Suggestions are welcome. Regardless of what the DefWindowProc(WM_NCPAINT) handler does comctl32 controls currently can't work correctly if region == 1 is passed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84320
Visually listview erases parts of other windows of the same parent in winecfg.
I don't see off hand how easily the correntness of listview painting could be tested. Suggestions are welcome.
Can it be done by checking the pixels for other windows are not erased? For example, use GetPixel(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84322
On Tue Oct 8 10:42:09 2024 +0000, Zhiyi Zhang wrote:
Visually listview erases parts of other windows of the same parent in winecfg. I don't see off hand how easily the correntness of listview painting could be tested. Suggestions are welcome. Can it be done by checking the pixels for other windows are not erased? For example, use GetPixel(). Are you suggesting checking all the pixels or just some? This sounds like a fragile approach.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84323
On Tue Oct 8 10:51:34 2024 +0000, Dmitry Timoshkov wrote:
Are you suggesting checking all the pixels or just some? This sounds like a fragile approach. For what it's worth using winecfg as an interactive test worked pretty well for me since its uses listview in many places.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84325
Are you suggesting checking all the pixels or just some?
One pixel is probably enough. test_themed_background() is such an example of a visual test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84326
On Tue Oct 8 12:33:36 2024 +0000, Zhiyi Zhang wrote:
Are you suggesting checking all the pixels or just some? One pixel is probably enough. test_themed_background() is such an example of a visual test. The problem is that I don't know exact placement of sibling windows that get corrupted. As I mentioned I'm using winecfg (where this bug was noticed originally), and replicating what winecfg doesn't seem reasonable. Why not use winecfg as an interactive test to check the correctness of listview repainting?
Besides, the patch fixes an obvious bug, user32 painting mechanisms are unversal and comctl32 controls are just normal user32 windows that must follow the user32 painting rules. So, I don't get where the speculation about comctl32 strange behaviour comes from. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84331
The problem is that I don't know exact placement of sibling windows that get corrupted. As I mentioned I'm using winecfg (where this bug was noticed originally), and replicating what winecfg doesn't seem reasonable. Why not use winecfg as an interactive test to check the correctness of listview repainting?
Because interactive tests are often not run. Besides, this is not something that can't be tested without human interaction.
and replicating what winecfg doesn't seem reasonable.
Having a test replicating the behavior that can reproduce the bug seems reasonable to me.
Besides, the patch fixes an obvious bug, user32 painting mechanisms are unversal and comctl32 controls are just normal user32 windows that must follow the user32 painting rules. So, I don't get where the speculation about comctl32 strange behaviour comes from.
It might seem obvious and it's probably fine to commit this MR. But I would feel more comfortable approving this MR with a test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84340
On Tue Oct 8 14:31:02 2024 +0000, Zhiyi Zhang wrote:
The problem is that I don't know exact placement of sibling windows that get corrupted. As I mentioned I'm using winecfg (where this bug was noticed originally), and replicating what winecfg doesn't seem reasonable. Why not use winecfg as an interactive test to check the correctness of listview repainting? Because interactive tests are often not run. Besides, this is not something that can't be tested without human interaction. and replicating what winecfg doesn't seem reasonable. Having a test replicating the behavior that can reproduce the bug seems reasonable to me. Besides, the patch fixes an obvious bug, user32 painting mechanisms are unversal and comctl32 controls are just normal user32 windows that must follow the user32 painting rules. So, I don't get where the speculation about comctl32 strange behaviour comes from. It might seem obvious and it's probably fine to commit this MR. But I would feel more comfortable approving this MR with a test. Could you show two screenshots comparing the corrupted window area of winecfg before and after this MR?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84341
On Tue Oct 8 14:34:49 2024 +0000, Zhiyi Zhang wrote:
Could you show two screenshots comparing the corrupted window area of winecfg before and after this MR? I feel highly uncomfortable spending time on adding tests for an obvious bugfix that simply replicates existing DefWindowProc() code that has no questions about its correctness.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84343
This merge request was closed by Dmitry Timoshkov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617
On Tue Oct 8 14:40:01 2024 +0000, Dmitry Timoshkov wrote:
I feel highly uncomfortable spending time on adding tests for an obvious bugfix that simply replicates existing DefWindowProc() code that has no questions about its correctness. As I said, feel free to keep this MR open without my approval. I am not that familiar with this part of the code to know it's obviously correct.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6617#note_84346
participants (4)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Nikolay Sivov (＠nsivov)
-
Zhiyi Zhang (＠zhiyi)