[PATCH v2 0/2] MR6186: comctl32/progress: Add states for progress bar.
Comctl32 V6 adds three different states into the progress bar: Normal, Paused, and Error. These three states change the color of the bar based on the theme. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56352 -- v2: comctl32/progress: Add states for progress bar. https://gitlab.winehq.org/wine/wine/-/merge_requests/6186
From: Jacob Czekalla <jczekalla(a)codeweavers.com> --- dlls/comctl32/tests/progress.c | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/dlls/comctl32/tests/progress.c b/dlls/comctl32/tests/progress.c index b6716366498..5211dff3065 100644 --- a/dlls/comctl32/tests/progress.c +++ b/dlls/comctl32/tests/progress.c @@ -308,6 +308,44 @@ static void test_PBM_STEPIT(void) } } +void test_bar_states(void) +{ + HWND progress_bar; + int state; + + progress_bar = create_progress(0); + + /* Check for normal state */ + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + ok(state == PBST_NORMAL, "Expected %d (PBST_NORMAL), but got %d.\n", PBST_NORMAL, state); + + /* Check for paused state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_PAUSED, 0); + ok(state == PBST_NORMAL, "Expected %d (PBST_NORMAL), but got %d\n", PBST_NORMAL, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + + /* Check for error state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); + todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check bad state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, 0, 0); + todo_wine ok(state == 0, "Expected 0, but got %d\n", state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check setting current state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + DestroyWindow(progress_bar); +} + static void init_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -340,6 +378,7 @@ START_TEST(progress) test_setcolors(); test_PBM_STEPIT(); + test_bar_states(); unload_v6_module(ctx_cookie, hCtx); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6186
From: Jacob Czekalla <jczekalla(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56352 --- dlls/comctl32/progress.c | 25 +++++++++++++++++++------ dlls/comctl32/tests/progress.c | 14 +++++++------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/dlls/comctl32/progress.c b/dlls/comctl32/progress.c index 6d556f10094..46d8d2c904f 100644 --- a/dlls/comctl32/progress.c +++ b/dlls/comctl32/progress.c @@ -53,6 +53,7 @@ typedef struct COLORREF ColorBar; /* Bar color */ COLORREF ColorBk; /* Background color */ HFONT Font; /* Handle to font (not unused) */ + UINT State; /* State of progress bar */ } PROGRESS_INFO; /* Control configuration constants */ @@ -147,6 +148,7 @@ typedef struct tagProgressDrawInfo int ledW, ledGap; HTHEME theme; RECT bgRect; + UINT state; } ProgressDrawInfo; typedef void (*ProgressDrawProc)(const ProgressDrawInfo* di, int start, int end); @@ -243,7 +245,7 @@ static void draw_theme_bar_H (const ProgressDrawInfo* di, int start, int end) r.top = di->rect.top; r.bottom = di->rect.bottom; r.right = di->rect.left + end; - DrawThemeBackground (di->theme, di->hdc, PP_CHUNK, 0, &r, NULL); + DrawThemeBackground (di->theme, di->hdc, PP_FILL, di->state, &r, NULL); } /* draw themed vertical bar from 'start' to 'end' */ @@ -254,7 +256,7 @@ static void draw_theme_bar_V (const ProgressDrawInfo* di, int start, int end) r.right = di->rect.right; r.bottom = di->rect.bottom - start; r.top = di->rect.bottom - end; - DrawThemeBackground (di->theme, di->hdc, PP_CHUNKVERT, 0, &r, NULL); + DrawThemeBackground (di->theme, di->hdc, PP_FILLVERT, di->state, &r, NULL); } /* draw themed horizontal background from 'start' to 'end' */ @@ -310,6 +312,7 @@ static LRESULT PROGRESS_Draw (PROGRESS_INFO *infoPtr, HDC hdc) TRACE("(infoPtr=%p, hdc=%p)\n", infoPtr, hdc); pdi.hdc = hdc; + pdi.state = infoPtr->State; pdi.theme = GetWindowTheme (infoPtr->Self); /* get the required bar brush */ @@ -522,6 +525,17 @@ static UINT PROGRESS_SetPos (PROGRESS_INFO *infoPtr, INT pos) } } +static UINT PROGRESS_SetState (HWND hwnd, PROGRESS_INFO *infoPtr, UINT state) +{ + UINT prev_state = infoPtr->State; + if (state == PBST_NORMAL || state == PBST_PAUSED || state == PBST_ERROR) + infoPtr->State = state; + else + return 0; + + return prev_state; +} + /*********************************************************************** * ProgressWindowProc */ @@ -569,6 +583,7 @@ static LRESULT WINAPI ProgressWindowProc(HWND hwnd, UINT message, infoPtr->ColorBar = CLR_DEFAULT; infoPtr->ColorBk = CLR_DEFAULT; infoPtr->Font = 0; + infoPtr->State = PBST_NORMAL; TRACE("Progress Ctrl creation, hwnd=%p\n", hwnd); return 0; @@ -711,12 +726,10 @@ static LRESULT WINAPI ProgressWindowProc(HWND hwnd, UINT message, return infoPtr->ColorBk; case PBM_SETSTATE: - if(wParam != PBST_NORMAL) - FIXME("state %Ix not yet handled\n", wParam); - return PBST_NORMAL; + return PROGRESS_SetState(hwnd, infoPtr, wParam); case PBM_GETSTATE: - return PBST_NORMAL; + return infoPtr->State; case PBM_SETMARQUEE: if(wParam != 0) diff --git a/dlls/comctl32/tests/progress.c b/dlls/comctl32/tests/progress.c index 5211dff3065..b7f2659f2df 100644 --- a/dlls/comctl32/tests/progress.c +++ b/dlls/comctl32/tests/progress.c @@ -323,25 +323,25 @@ void test_bar_states(void) state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_PAUSED, 0); ok(state == PBST_NORMAL, "Expected %d (PBST_NORMAL), but got %d\n", PBST_NORMAL, state); state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); - todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); /* Check for error state */ state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); - todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); - todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); /* Check bad state */ state = SendMessageA(progress_bar, PBM_SETSTATE, 0, 0); - todo_wine ok(state == 0, "Expected 0, but got %d\n", state); + ok(state == 0, "Expected 0, but got %d\n", state); state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); - todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); /* Check setting current state */ state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); - todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); - todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); DestroyWindow(progress_bar); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6186
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=147468 Your paranoid android. === debian11 (32 bit report) === comctl32: taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 === debian11b (64 bit WoW report) === comctl32: taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2
``` taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 taskdialog.c:725: Test succeeded inside todo block: Expect state: 3 got state: 3 taskdialog.c:727: Test succeeded inside todo block: Expect state: 2 got state: 2 0244:taskdialog: 2391 tests executed (0 marked as todo, 0 as flaky, 6 failures), 0 skipped. ``` Please remove the todo_wines for these tests as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77654
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/progress.c:
} }
+static UINT PROGRESS_SetState (HWND hwnd, PROGRESS_INFO *infoPtr, UINT state) +{ + UINT prev_state = infoPtr->State;
Let's add a new line after the variable declaration. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77656
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
+ todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + + /* Check for error state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); + todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check bad state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, 0, 0); + todo_wine ok(state == 0, "Expected 0, but got %d\n", state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check setting current state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); There is also the PBFS_PARTIAL state. So please add tests for it and implement it as well.
Also, using a loop with static test data to test these states should be easier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77657
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/progress.c:
} }
+static UINT PROGRESS_SetState (HWND hwnd, PROGRESS_INFO *infoPtr, UINT state) +{ + UINT prev_state = infoPtr->State; + if (state == PBST_NORMAL || state == PBST_PAUSED || state == PBST_ERROR) + infoPtr->State = state;
If the state changes, you should call InvalidateRect(hwnd, ..., TRUE) to schedule a redraw. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77658
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
+ todo_wine ok(state == PBST_PAUSED, "Expected %d (PBST_PAUSED), but got %d\n", PBST_PAUSED, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check bad state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, 0, 0); + todo_wine ok(state == 0, "Expected 0, but got %d\n", state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + + /* Check setting current state */ + state = SendMessageA(progress_bar, PBM_SETSTATE, PBST_ERROR, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0); + todo_wine ok(state == PBST_ERROR, "Expected %d (PBST_ERROR), but got %d\n", PBST_ERROR, state); + Let's add tests to check that a redraw is issued when the state changes. See c8ec431fd4cd370daba96aa18d892fa8d7397ef8 for an example of adding tests to verify if a message triggers WM_PAINT and WM_ERASEBKGND messages.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77659
On Fri Aug 2 13:04:14 2024 +0000, Zhiyi Zhang wrote:
There is also the PBFS_PARTIAL state. So please add tests for it and implement it as well. Also, using a loop with static test data to test these states should be easier. PBFS_PARTIAL isn't a valid state of the progress bar. Only PBST_NORMAL, PBST_PAUSED, and PBST_ERROR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77672
On Fri Aug 2 15:29:17 2024 +0000, Jacob Czekalla wrote:
PBFS_PARTIAL isn't a valid state of the progress bar. Only PBST_NORMAL, PBST_PAUSED, and PBST_ERROR. According to https://learn.microsoft.com/en-us/windows/win32/controls/parts-and-states, PBFS_PARTIAL should be valid. Could you verify it with tests?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77673
participants (4)
-
Jacob Czekalla -
Jacob Czekalla (＠JacobCzekalla)
-
Marvin -
Zhiyi Zhang (＠zhiyi)