[PATCH v3 0/8] MR5665: For a CS_PARENTDC child use parent for visible region calculations.
These patches fix painting in an application that draws outside of a CS_PARENTDC child client area in its WM_PAINT handler. Comments and suggestions are welcome. -- v3: win32u: Use parent rectangle for visible region calculations of a CS_PARENTDC child. win32u: Don't clip update region to the window client rectangle. win32u: GetUpdateRect() should clip update rectangle to the window client area. win32u: GetUpdateRgn() should clip update region to the window client area. win32u: Clip PAINTSTRUCT.rcPaint to the window client area. server: For a CS_PARENTDC child use parent for visible region calculations. server: If the being validated region covers whole window then validate everything. user32/tests: Add a message test for listbox redrawing after LB_SETCOUNT. https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/user32/tests/msg.c | 71 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 5b070255303..6d3db92c891 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2309,6 +2309,7 @@ static BOOL after_end_dialog, test_def_id, paint_loop_done; static int sequence_cnt, sequence_size; static struct recvd_message* sequence; static int log_all_parent_messages; +static int log_painting_messages; static CRITICAL_SECTION sequence_cs; /* user32 functions */ @@ -16900,6 +16901,25 @@ static const struct message wm_lb_dblclick_0[] = { WM_LBUTTONUP, sent|wparam|lparam, 0, 0 }, { 0 } }; +static const struct message wm_lb_setcount[] = +{ + { LB_SETCOUNT, sent|wparam|lparam, 100, 0 }, + { WM_WINDOWPOSCHANGING, sent|wparam|defwinproc, SWP_NOACTIVATE|SWP_FRAMECHANGED|SWP_NOSIZE|SWP_NOMOVE }, + { WM_NCCALCSIZE, sent|wparam|defwinproc, 1 }, + { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|msg_todo, 0, 0 }, + { WM_ERASEBKGND, sent|parent }, + { WM_WINDOWPOSCHANGED, sent|wparam|defwinproc, SWP_NOACTIVATE|SWP_FRAMECHANGED|SWP_NOSIZE|SWP_NOMOVE|SWP_NOCLIENTMOVE }, + { WM_SIZE, sent|defwinproc }, + { EVENT_OBJECT_VALUECHANGE, winevent_hook|wparam|lparam|msg_todo, OBJID_VSCROLL, 0 }, + { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|msg_todo, OBJID_WINDOW, 0 }, + { EVENT_OBJECT_VALUECHANGE, winevent_hook|wparam|lparam|msg_todo, OBJID_VSCROLL, 0 }, + { WM_USER, sent|wparam|lparam, 0, 0 }, + { WM_NCPAINT, sent|wparam|lparam, 1, 0 }, + { WM_ERASEBKGND, sent }, + { WM_CTLCOLORLISTBOX, sent|parent }, + { WM_USER+1, sent|wparam|lparam, 0, 0 }, + { 0 } +}; #define check_lb_state(a1, a2, a3, a4, a5) check_lb_state_dbg(a1, a2, a3, a4, a5, __LINE__) @@ -16912,10 +16932,11 @@ static LRESULT WINAPI listbox_hook_proc(HWND hwnd, UINT message, WPARAM wp, LPAR struct recvd_message msg; /* do not log painting messages */ - if (message != WM_PAINT && + if ((log_painting_messages || + (message != WM_PAINT && message != WM_NCPAINT && message != WM_SYNCPAINT && - message != WM_ERASEBKGND && + message != WM_ERASEBKGND)) && message != WM_NCHITTEST && message != WM_GETTEXT && !ignore_message( message )) @@ -16960,11 +16981,57 @@ static void check_lb_state_dbg(HWND listbox, int count, int cur_sel, static void test_listbox_messages(void) { + PAINTSTRUCT ps; + RECT rc, rc1; HWND parent, listbox; LRESULT ret; parent = CreateWindowExA(0, "TestParentClass", NULL, WS_OVERLAPPEDWINDOW | WS_VISIBLE, 100, 100, 200, 200, 0, 0, 0, NULL); + + /* test listbox redrawing after LB_SETCOUNT */ + listbox = CreateWindowExA(WS_EX_NOPARENTNOTIFY, "ListBox", NULL, + LBS_OWNERDRAWFIXED | LBS_NODATA | WS_CHILD | WS_VSCROLL | WS_VISIBLE, + 10, 10, 80, 80, parent, (HMENU)ID_LISTBOX, 0, NULL); + listbox_orig_proc = (WNDPROC)SetWindowLongPtrA(listbox, GWLP_WNDPROC, (ULONG_PTR)listbox_hook_proc); + + UpdateWindow(listbox); + + check_lb_state(listbox, 0, LB_ERR, 0, 0); + + flush_sequence(); + + log_all_parent_messages++; + log_painting_messages++; + + ret = GetWindowLongA(listbox, GWL_STYLE); + ok((ret & (WS_VSCROLL | WS_HSCROLL)) == 0, "Listbox should not have scroll bars\n"); + + ret = SendMessageA(listbox, LB_SETCOUNT, 100, 0); + ok(ret == 0, "got %Id\n", ret); + ret = GetWindowLongA(listbox, GWL_STYLE); + ok((ret & (WS_VSCROLL | WS_HSCROLL)) == WS_VSCROLL, "Listbox should have vertical scroll bar\n"); + + SendMessageA(listbox, WM_USER, 0, 0); /* Mark */ + BeginPaint(listbox, &ps); + GetClientRect(parent, &rc1); + MapWindowPoints(parent, listbox, (POINT *)&rc1, 2); + GetClipBox(ps.hdc, &rc); + todo_wine + ok(EqualRect(&rc, &rc1), "hdc clipbox %s != parent client rect %s\n", wine_dbgstr_rect(&rc), wine_dbgstr_rect(&rc1)); + GetClientRect(listbox, &rc); + ok(EqualRect(&ps.rcPaint, &rc), "rcPaint %s != listbox client rect %s\n", wine_dbgstr_rect(&ps.rcPaint), wine_dbgstr_rect(&rc)); + EndPaint(listbox, &ps); + SendMessageA(listbox, WM_USER+1, 0, 0); /* Mark */ + + ok_sequence(wm_lb_setcount, "LB_SETCOUNT", FALSE); + flush_sequence(); + + log_painting_messages--; + log_all_parent_messages--; + + DestroyWindow(listbox); + /* with LBS_HASSTRINGS */ listbox = CreateWindowExA(WS_EX_NOPARENTNOTIFY, "ListBox", NULL, WS_CHILD | LBS_NOTIFY | LBS_OWNERDRAWVARIABLE | LBS_HASSTRINGS | WS_VISIBLE, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- server/window.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/server/window.c b/server/window.c index 4ebfec3da12..51f573751c7 100644 --- a/server/window.c +++ b/server/window.c @@ -1532,7 +1532,24 @@ static void redraw_window( struct window *win, struct region *region, int frame, { if ((tmp = crop_region_to_win_rect( win, region, frame ))) { - if (!subtract_region( tmp, win->update_region, tmp )) + if ((child_rgn = create_empty_region())) + { + rectangle_t rect = win->window_rect; + + offset_rect( &rect, -rect.left, -rect.top ); + set_region_rect( child_rgn, &rect ); + + if (subtract_region( child_rgn, child_rgn, tmp ) && is_region_empty( child_rgn ) ) + { + /* region covers whole window: validate everything */ + free_region( tmp ); + tmp = NULL; + } + + free_region( child_rgn ); + } + + if (tmp && !subtract_region( tmp, win->update_region, tmp )) { free_region( tmp ); return; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- server/window.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/server/window.c b/server/window.c index 51f573751c7..abb85949024 100644 --- a/server/window.c +++ b/server/window.c @@ -1316,6 +1316,18 @@ static struct region *crop_region_to_win_rect( struct window *win, struct region struct region *tmp; if (!get_window_visible_rect( win, &rect, frame )) return NULL; + + if (win->parent && is_window_using_parent_dc( win )) + { + int offset_x, offset_y; + + if (!get_window_visible_rect( win->parent, &rect, 0 )) return NULL; + + offset_x = rect.left + (frame ? win->window_rect.left : win->client_rect.left); + offset_y = rect.top + (frame ? win->window_rect.top : win->client_rect.top); + offset_rect( &rect, -offset_x, -offset_y ); + } + if (!(tmp = create_empty_region())) return NULL; set_region_rect( tmp, &rect ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/win32u/dce.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index cfa3510c60b..727b5c31417 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -1157,6 +1157,8 @@ static BOOL send_erase( HWND hwnd, UINT flags, HRGN client_rgn, HDC hdc = 0; RECT dummy; + TRACE( "hwnd %p, flags %08x, client_rgn %p\n", hwnd, flags, client_rgn ); + if (!clip_rect) clip_rect = &dummy; if (hdc_ret || (flags & UPDATE_ERASE)) { @@ -1167,6 +1169,13 @@ static BOOL send_erase( HWND hwnd, UINT flags, HRGN client_rgn, { INT type = NtGdiGetAppClipBox( hdc, clip_rect ); + if (get_class_long( hwnd, GCL_STYLE, FALSE ) & CS_PARENTDC) + { + RECT client_rect; + get_client_rect( hwnd, &client_rect, get_thread_dpi() ); + intersect_rect( clip_rect, clip_rect, &client_rect ); + } + if (flags & UPDATE_ERASE) { /* don't erase if the clip box is empty */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/win32u/dce.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index 727b5c31417..76570b2747b 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -1444,7 +1444,15 @@ INT WINAPI NtUserGetUpdateRgn( HWND hwnd, HRGN hrgn, BOOL erase ) if ((update_rgn = send_ncpaint( hwnd, NULL, &flags ))) { - retval = NtGdiCombineRgn( hrgn, update_rgn, 0, RGN_COPY ); + RECT client_rect; + HRGN client_rgn; + + get_window_rects( hwnd, COORDS_SCREEN, NULL, &client_rect, get_thread_dpi() ); + + client_rgn = NtGdiCreateRectRgn( client_rect.left, client_rect.top, client_rect.right, client_rect.bottom ); + retval = NtGdiCombineRgn( hrgn, update_rgn, client_rgn, RGN_AND ); + NtGdiDeleteObjectApp( client_rgn ); + if (send_erase( hwnd, flags, update_rgn, NULL, NULL )) { flags = UPDATE_DELAYED_ERASE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/win32u/dce.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index 76570b2747b..fa8f61301be 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -1471,13 +1471,20 @@ INT WINAPI NtUserGetUpdateRgn( HWND hwnd, HRGN hrgn, BOOL erase ) BOOL WINAPI NtUserGetUpdateRect( HWND hwnd, RECT *rect, BOOL erase ) { UINT flags = UPDATE_NOCHILDREN; - HRGN update_rgn; + HRGN update_rgn, client_rgn; + RECT client_rect; BOOL need_erase; if (erase) flags |= UPDATE_NONCLIENT | UPDATE_ERASE; if (!(update_rgn = send_ncpaint( hwnd, NULL, &flags ))) return FALSE; + get_window_rects( hwnd, COORDS_SCREEN, NULL, &client_rect, get_thread_dpi() ); + + client_rgn = NtGdiCreateRectRgn( client_rect.left, client_rect.top, client_rect.right, client_rect.bottom ); + NtGdiCombineRgn( update_rgn, update_rgn, client_rgn, RGN_AND ); + NtGdiDeleteObjectApp( client_rgn ); + if (rect && NtGdiGetRgnBox( update_rgn, rect ) != NULLREGION) { HDC hdc = NtUserGetDCEx( hwnd, 0, DCX_USESTYLE ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/user32/tests/dce.c | 8 ++++---- dlls/user32/tests/win.c | 24 ++++++++++++------------ dlls/win32u/dce.c | 35 ++++++++++++++++++----------------- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/dlls/user32/tests/dce.c b/dlls/user32/tests/dce.c index 18f31e3708c..64becb6084d 100644 --- a/dlls/user32/tests/dce.c +++ b/dlls/user32/tests/dce.c @@ -515,10 +515,10 @@ static void test_begin_paint(void) EndPaint( hwnd_parentdc, &ps ); GetClientRect( hwnd_parent, &parent_rect ); - todo_wine ok( rect.left == parent_rect.left, "rect.left = %ld, expected %ld\n", rect.left, parent_rect.left ); - todo_wine ok( rect.top == parent_rect.top, "rect.top = %ld, expected %ld\n", rect.top, parent_rect.top ); - todo_wine ok( rect.right == parent_rect.right, "rect.right = %ld, expected %ld\n", rect.right, parent_rect.right ); - todo_wine ok( rect.bottom == parent_rect.bottom, "rect.bottom = %ld, expected %ld\n", rect.bottom, parent_rect.bottom ); + ok( rect.left == parent_rect.left, "rect.left = %ld, expected %ld\n", rect.left, parent_rect.left ); + ok( rect.top == parent_rect.top, "rect.top = %ld, expected %ld\n", rect.top, parent_rect.top ); + ok( rect.right == parent_rect.right, "rect.right = %ld, expected %ld\n", rect.right, parent_rect.right ); + ok( rect.bottom == parent_rect.bottom, "rect.bottom = %ld, expected %ld\n", rect.bottom, parent_rect.bottom ); hdc = GetDC( hwnd_parent ); todo_wine ok( GetPixel( hdc, 60, 60 ) == cr, "error drawing outside of window client area\n" ); diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 775164e3e9f..b92c386251b 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -6274,49 +6274,49 @@ static void test_csparentdc(void) struct parentdc_test test_answer; #define nothing_todo {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}} - const struct parentdc_test test1 = + const struct parentdc_test test1 = { {{0, 0, 150, 150}, {0, 0, 150, 150}, {0, 0, 150, 150}}, nothing_todo, - {{0, 0, 40, 40}, {-20, -20, 130, 130}, {0, 0, 40, 40}}, {{0, 0, 0, 0}, {1, 1, 1, 1}, {0, 0, 0, 0}}, - {{0, 0, 40, 40}, {-40, -40, 110, 110}, {0, 0, 40, 40}}, {{0, 0, 0, 0}, {1, 1, 1, 1}, {0, 0, 0, 0}}, + {{0, 0, 40, 40}, {-20, -20, 130, 130}, {0, 0, 40, 40}}, nothing_todo, + {{0, 0, 40, 40}, {-40, -40, 110, 110}, {0, 0, 40, 40}}, nothing_todo, }; - const struct parentdc_test test2 = + const struct parentdc_test test2 = { {{0, 0, 150, 150}, {0, 0, 50, 50}, {0, 0, 50, 50}}, nothing_todo, - {{0, 0, 40, 40}, {-20, -20, 30, 30}, {0, 0, 30, 30}}, {{0, 0, 0, 0}, {1, 1, 0, 0}, {0, 0, 0, 0}}, - {{0, 0, 40, 40}, {-40, -40, 10, 10}, {0, 0, 10, 10}}, {{0, 0, 0, 0}, {1, 1, 0, 0}, {0, 0, 0, 0}}, + {{0, 0, 40, 40}, {-20, -20, 30, 30}, {0, 0, 30, 30}}, nothing_todo, + {{0, 0, 40, 40}, {-40, -40, 10, 10}, {0, 0, 10, 10}}, nothing_todo, }; - const struct parentdc_test test3 = + const struct parentdc_test test3 = { {{0, 0, 150, 150}, {0, 0, 10, 10}, {0, 0, 10, 10}}, nothing_todo, {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}}, nothing_todo, {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}}, nothing_todo, }; - const struct parentdc_test test4 = + const struct parentdc_test test4 = { {{0, 0, 150, 150}, {40, 40, 50, 50}, {40, 40, 50, 50}}, nothing_todo, {{0, 0, 40, 40}, {20, 20, 30, 30}, {20, 20, 30, 30}}, nothing_todo, {{0, 0, 40, 40}, {0, 0, 10, 10}, {0, 0, 10, 10}}, nothing_todo, }; - const struct parentdc_test test5 = + const struct parentdc_test test5 = { {{0, 0, 150, 150}, {20, 20, 60, 60}, {20, 20, 60, 60}}, nothing_todo, {{0, 0, 40, 40}, {-20, -20, 130, 130}, {0, 0, 40, 40}}, {{0, 0, 0, 0}, {1, 1, 1, 1}, {0, 0, 0, 0}}, - {{0, 0, 40, 40}, {-20, -20, 20, 20}, {0, 0, 20, 20}}, {{0, 0, 0, 0}, {1, 1, 0, 0}, {0, 0, 0, 0}}, + {{0, 0, 40, 40}, {-20, -20, 20, 20}, {0, 0, 20, 20}}, nothing_todo, }; - const struct parentdc_test test6 = + const struct parentdc_test test6 = { {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}}, nothing_todo, {{0, 0, 40, 40}, {0, 0, 10, 10}, {0, 0, 10, 10}}, nothing_todo, {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}}, nothing_todo, }; - const struct parentdc_test test7 = + const struct parentdc_test test7 = { {{0, 0, 0, 0}, {0, 0, 0, 0}, {0, 0, 0, 0}}, nothing_todo, {{0, 0, 40, 40}, {-20, -20, 130, 130}, {0, 0, 40, 40}}, {{0, 0, 0, 0}, {1, 1, 1, 1}, {0, 0, 0, 0}}, diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index fa8f61301be..93107f93930 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -1083,10 +1083,13 @@ static BOOL get_update_flags( HWND hwnd, HWND *child, UINT *flags ) */ static HRGN send_ncpaint( HWND hwnd, HWND *child, UINT *flags ) { - HRGN whole_rgn = get_update_region( hwnd, flags, child ); - HRGN client_rgn = 0; + HRGN whole_rgn; DWORD style; + TRACE( "hwnd %p, flags %08x\n", hwnd, *flags ); + + whole_rgn = get_update_region( hwnd, flags, child ); + if (child) hwnd = *child; if (hwnd == get_desktop_window()) return whole_rgn; @@ -1096,6 +1099,7 @@ static HRGN send_ncpaint( HWND hwnd, HWND *child, UINT *flags ) UINT context; RECT client, window, update; INT type; + HRGN nc_rgn = 0; context = set_thread_dpi_awareness_context( get_window_dpi_awareness_context( hwnd )); @@ -1107,23 +1111,17 @@ static HRGN send_ncpaint( HWND hwnd, HWND *child, UINT *flags ) update.left < client.left || update.top < client.top || update.right > client.right || update.bottom > client.bottom) { - client_rgn = NtGdiCreateRectRgn( client.left, client.top, client.right, client.bottom ); - NtGdiCombineRgn( client_rgn, client_rgn, whole_rgn, RGN_AND ); - /* check if update rgn contains complete nonclient area */ - if (type == SIMPLEREGION && EqualRect( &window, &update )) + if (type == SIMPLEREGION && update.left <= window.left && update.top <= window.top && + update.right >= window.right && update.bottom >= window.bottom) { - NtGdiDeleteObjectApp( whole_rgn ); - whole_rgn = (HRGN)1; + nc_rgn = (HRGN)1; } - } - else - { - client_rgn = whole_rgn; - whole_rgn = 0; + else + nc_rgn = whole_rgn; } - if (whole_rgn) /* NOTE: WM_NCPAINT allows wParam to be 1 */ + if (nc_rgn) /* NOTE: WM_NCPAINT allows wParam to be 1 */ { if (*flags & UPDATE_NONCLIENT) { @@ -1134,13 +1132,12 @@ static HRGN send_ncpaint( HWND hwnd, HWND *child, UINT *flags ) if (style & WS_VSCROLL) set_standard_scroll_painted( hwnd, SB_VERT, FALSE ); - send_message( hwnd, WM_NCPAINT, (WPARAM)whole_rgn, 0 ); + send_message( hwnd, WM_NCPAINT, (WPARAM)nc_rgn, 0 ); } - if (whole_rgn > (HRGN)1) NtGdiDeleteObjectApp( whole_rgn ); } set_thread_dpi_awareness_context( context ); } - return client_rgn; + return whole_rgn; } /*********************************************************************** @@ -1264,6 +1261,8 @@ HDC WINAPI NtUserBeginPaint( HWND hwnd, PAINTSTRUCT *ps ) RECT rect; UINT flags = UPDATE_NONCLIENT | UPDATE_ERASE | UPDATE_PAINT | UPDATE_INTERNALPAINT | UPDATE_NOCHILDREN; + TRACE( "hwnd %p, %p\n", hwnd, ps ); + NtUserHideCaret( hwnd ); if (!(hrgn = send_ncpaint( hwnd, NULL, &flags ))) return 0; @@ -1288,6 +1287,8 @@ HDC WINAPI NtUserBeginPaint( HWND hwnd, PAINTSTRUCT *ps ) */ BOOL WINAPI NtUserEndPaint( HWND hwnd, const PAINTSTRUCT *ps ) { + TRACE( "hwnd %p, %p\n", hwnd, ps ); + NtUserShowCaret( hwnd ); flush_window_surfaces( FALSE ); if (!ps) return FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/user32/tests/dce.c | 2 +- dlls/win32u/dce.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/user32/tests/dce.c b/dlls/user32/tests/dce.c index 64becb6084d..c7e498c60d0 100644 --- a/dlls/user32/tests/dce.c +++ b/dlls/user32/tests/dce.c @@ -521,7 +521,7 @@ static void test_begin_paint(void) ok( rect.bottom == parent_rect.bottom, "rect.bottom = %ld, expected %ld\n", rect.bottom, parent_rect.bottom ); hdc = GetDC( hwnd_parent ); - todo_wine ok( GetPixel( hdc, 60, 60 ) == cr, "error drawing outside of window client area\n" ); + ok( GetPixel( hdc, 60, 60 ) == cr, "error drawing outside of window client area\n" ); ReleaseDC( hwnd_parent, hdc ); } diff --git a/dlls/win32u/dce.c b/dlls/win32u/dce.c index 93107f93930..58d7c30ee4e 100644 --- a/dlls/win32u/dce.c +++ b/dlls/win32u/dce.c @@ -420,6 +420,11 @@ static void update_visible_region( struct dce *dce ) top_win = wine_server_ptr_handle( reply->top_win ); win_rect = wine_server_get_rect( reply->win_rect ); top_rect = wine_server_get_rect( reply->top_rect ); + if (flags & DCX_PARENTCLIP) + { + win_rect.right = top_rect.right; + win_rect.bottom = top_rect.bottom; + } paint_flags = reply->paint_flags; } else size = reply->total_size; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5665
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=146137 Your paranoid android. === w10pro64 (32 bit report) === user32: win.c:3818: Test failed: GetForegroundWindow returned 000202C2 win.c:3749: Test failed: SetForegroundWindow failed, error 0 win.c:3752: Test failed: GetForegroundWindow returned 000202C2 win.c:3789: Test failed: GetForegroundWindow returned 000202C2 win.c:3865: Test failed: GetActiveWindow() = 000200F6 win.c:3868: Test failed: GetFocus() = 00000000 win.c:3871: Test failed: GetFocus() = 00000000 win.c:3874: Test failed: GetFocus() = 00000000 win.c:3877: Test failed: GetActiveWindow() = 000200F6 win.c:3881: Test failed: GetFocus() = 00000000 win.c:3884: Test failed: GetFocus() = 00000000 === w10pro64 (64 bit report) === user32: win.c:3818: Test failed: GetForegroundWindow returned 000000000003017E win.c:3749: Test failed: SetForegroundWindow failed, error 0 win.c:3752: Test failed: GetForegroundWindow returned 000000000003017E win.c:3789: Test failed: GetForegroundWindow returned 000000000003017E win.c:3877: Test failed: GetActiveWindow() = 00000000000102B8 win.c:3881: Test failed: GetFocus() = 0000000000000000 win.c:3884: Test failed: GetFocus() = 0000000000000000 === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000000E600F0, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
I hope that I've fixed discovered test failures, it would be nice to get some comments and reviews. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72647
It has ~100 changed lines (excluding tests) across 8 commits, which is hard to review at a glance. Would you move test commits prior to fix commits? The test commits will introduce `todo_wine` before `ok()`s expected to fail, and fix commits will remove them. That would make it easier to understand what fixes what. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72777
On Sat Jun 8 11:51:09 2024 +0000, Jinoh Kang wrote:
It has ~100 changed lines (excluding tests) across 8 commits, which is hard to review at a glance. Would you move test commits prior to fix commits? The test commits will introduce `todo_wine` before `ok()`s expected to fail, and fix commits will remove them. That would make it easier to understand what fixes what. There's the only single test commit, and it's already a separate one. Remaning patches don't introduce new tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72781
There's the only single test commit, and it's already a separate one.
Being separate isn't enough, it should be moved earlier in the order of commits.
Remaning patches don't introduce new tests.
Yes, which is why I believe this might need more tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72782
On Sat Jun 8 12:16:43 2024 +0000, Jinoh Kang wrote:
Remaning patches don't introduce new tests. Yes, which is why I believe this might need more tests. Let me be clear: "not enough tests" is not a criteria for rejection. It just makes the patch hard to understand and/or verify, which tends to discourage reviewers from looking into it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72783
There's the only single test commit, and it's already a separate one.
Being separate isn't enough, it should be moved earlier in the order of commits.
Remaning patches don't introduce new tests.
Yes, which is why I believe this might need more tests.
Previous comment was wrong about the tests. Sorry for confusion. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72784
On Sat Jun 8 12:16:43 2024 +0000, Jinoh Kang wrote:
There's the only single test commit, and it's already a separate one.
Being separate isn't enough, it should be moved earlier in the order of commits.
Remaning patches don't introduce new tests.
Yes, which is why I believe this might need more tests. Previous comment was wrong about the tests. Sorry for confusion. Personally I think that existing tests cover most of the being fixed functionality judging by the amount of removed todo_wine statements.
Feel free to suggest the areas of new tests to add though, I'm open to suggestions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72787
Jinoh Kang (@iamahuman) commented about server/window.c:
struct region *tmp;
if (!get_window_visible_rect( win, &rect, frame )) return NULL; + + if (win->parent && is_window_using_parent_dc( win )) + { + int offset_x, offset_y; + + if (!get_window_visible_rect( win->parent, &rect, 0 )) return NULL; + + offset_x = rect.left + (frame ? win->window_rect.left : win->client_rect.left); + offset_y = rect.top + (frame ? win->window_rect.top : win->client_rect.top);
This subtracts `rect.left` from `rect.left` and `rect.top` from `rect.top`. In other words, It ignores the left-top coordinate for no apparent reason. What if the parent window is partially occluded by the frame of ancestors? Then `rect.left` would be larger than the window edge, shifting the update rect further towards upper-left corner. Instead, this should be changed to something like[^1][^2][^3]: ```suggestion:-1+0 offset_x = win->parent->client_rect.left - win->parent->window_rect.left + win->window_rect.left; offset_y = win->parent->client_rect.top - win->parent->window_rect.top + win->window_rect.top; ``` You can split this into multiple calls to `offset_rect` or just multiple lines if you like. Also, it would be nice to have tests for the case when `rect.left != win->parent->client_rect.left - win->parent->window_rect.left` (same goes for `top`). Specifically, the test should prevent the suggested code from regressing. [^1]: user32:dce test result: https://testbot.winehq.org/JobDetails.pl?Key=146182. [^2]: user32:msg test result: https://testbot.winehq.org/JobDetails.pl?Key=146183. [^3]: user32:win test result: https://testbot.winehq.org/JobDetails.pl?Key=146184. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72788
On Sat Jun 8 14:04:59 2024 +0000, Jinoh Kang wrote:
This subtracts `rect.left` from `rect.left` and `rect.top` from `rect.top`. In other words, It ignores the left-top coordinate for no apparent reason. What if the parent window is partially occluded by the frame of ancestors? Then `rect.{left,top}` would be larger than the window edge, shifting the update rect further towards upper-left corner. Instead, this should be changed to something like[^1][^2][^3]: ```suggestion:-1+0 offset_x = win->parent->client_rect.left - win->parent->window_rect.left + win->window_rect.left; offset_y = win->parent->client_rect.top - win->parent->window_rect.top + win->window_rect.top; ``` You can split this into multiple calls to `offset_rect` or multiple lines if you like. Also, it would be nice to have tests for the case when `rect.left != win->parent->client_rect.left - win->parent->window_rect.left` (same goes for `top`). Specifically, the test should prevent the suggested code from regressing. [^1]: user32:dce test result: https://testbot.winehq.org/JobDetails.pl?Key=146182. [^2]: user32:msg test result: https://testbot.winehq.org/JobDetails.pl?Key=146183. [^3]: user32:win test result: https://testbot.winehq.org/JobDetails.pl?Key=146184. (Note: having the regression test *would be nice*, but is not necessary. Normally I would try to test for this kind of thing, but feel free to skip if it seems like too much work.)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72790
On Sat Jun 8 14:04:19 2024 +0000, Jinoh Kang wrote:
(Note: having the regression test *would be nice*, but is not necessary. Normally I would try to test for this kind of thing, but feel free to skip if it seems like too much work.) This is how it's done in other places of the code that use offset_x/offset_y variables.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72792
This is how it's done in other places of the code that use offset_x/offset_y variables.
Would you give me an example? I do see several `offset_[xy]`s, but none of them seem to use rect.left/top to subtract it from itself. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72800
Also, it would be nice to have tests for the case when `rect.left != win->parent->client_rect.left - win->parent->window_rect.left` (same goes for `top`). Specifically, the test should prevent the suggested code from regressing.
I submitted the test as MR !5836. This MR (!5665) fails the added test, and my suggestion fixes it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72953
Jinoh Kang (@iamahuman) commented about server/window.c:
struct region *tmp;
if (!get_window_visible_rect( win, &rect, frame )) return NULL; + + if (win->parent && is_window_using_parent_dc( win )) + { + int offset_x, offset_y; + + if (!get_window_visible_rect( win->parent, &rect, 0 )) return NULL;
What if the parent window is using parent DC too? Shall we ascend the ancestor chain until we meet a non-`CS_PARENTDC` window? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72955
On Wed Jun 12 17:53:10 2024 +0000, Jinoh Kang wrote:
Also, it would be nice to have tests for the case when `rect.left != win->parent->client_rect.left - win->parent->window_rect.left` (same goes for `top`). Specifically, the test should prevent the suggested code from regressing. I submitted the test as MR !5836. This MR (!5665) fails the added test, and my suggestion fixes it. Thank you.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72991
On Wed Jun 12 17:53:25 2024 +0000, Jinoh Kang wrote:
What if the parent window is using parent DC too? Shall we ascend the ancestor chain until we meet a non-`CS_PARENTDC` window? I think that your test answers this question, because "static" and "parentdc_class" both have CS_PARENTDC style.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_72992
On Wed Jun 12 22:59:24 2024 +0000, Dmitry Timoshkov wrote:
Thank you. No worries. Please apply the fix or otherwise address the review before resolving.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_73024
On Wed Jun 12 23:00:11 2024 +0000, Dmitry Timoshkov wrote:
I think that your test answers this question, because "static" and "parentdc_class" both have CS_PARENTDC style. Not really, since `CS_PARENTDC` is ineffective for an overlapped window. Sorry for confusion.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_73025
On Wed Jun 12 22:59:24 2024 +0000, Jinoh Kang wrote:
No worries. Please apply the fix or otherwise address the review before resolving. To reiterate, the following change is required to get the regions correct:
```suggestion:-1+0 offset_x = win->parent->client_rect.left - win->parent->window_rect.left + win->window_rect.left; offset_y = win->parent->client_rect.top - win->parent->window_rect.top + win->window_rect.top; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5665#note_73026
participants (4)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Jinoh Kang (＠iamahuman)
-
Marvin