[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.
From: Dmitry Timoshkov dmitry@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@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);
What does this fix visually? I guess the question is does this happen in any situation already, or some specific use case triggers this.
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.
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?
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?
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.
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.
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.
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().
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.
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.
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.
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.
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.
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?
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.
This merge request was closed by Dmitry Timoshkov.
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.
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Nikolay Sivov (＠nsivov)
-
Zhiyi Zhang (＠zhiyi)