[PATCH v3 0/2] MR6186: comctl32/progress: Add states for progress bar.

[Jacob Czekalla (＠JacobCzekalla)]

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

--
  v3: comctl32/progress: Add states for progress bar.
      comctl32/tests: Add tests for progress bar states.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186

[PATCH v3 1/2] comctl32/tests: Add tests for progress bar states.

[Jacob Czekalla]

From: Jacob Czekalla <jczekalla(a)codeweavers.com>

---
 dlls/comctl32/tests/progress.c | 101 ++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/dlls/comctl32/tests/progress.c b/dlls/comctl32/tests/progress.c
index b6716366498..8cfd437a98d 100644
--- a/dlls/comctl32/tests/progress.c
+++ b/dlls/comctl32/tests/progress.c
@@ -23,16 +23,27 @@
 #include "winbase.h"
 #include "wingdi.h"
 #include "winuser.h"
-#include "commctrl.h" 
+#include "commctrl.h"
+#include "vssym32.h"
 
 #include "wine/test.h"
-
 #include "v6util.h"
+#include "msg.h"
 
 static HWND hProgressParentWnd;
 static const char progressTestClass[] = "ProgressBarTestClass";
 static BOOL (WINAPI *pInitCommonControlsEx)(const INITCOMMONCONTROLSEX*);
 
+/* For message tests */
+enum seq_index
+{
+    CHILD_SEQ_INDEX,
+    PARENT_SEQ_INDEX,
+    NUM_MSG_SEQUENCES
+};
+
+static struct msg_sequence *sequences[NUM_MSG_SEQUENCES];
+
 static HWND create_progress(DWORD style)
 {
     return CreateWindowExA(0, PROGRESS_CLASSA, "", WS_VISIBLE | style,
@@ -308,6 +319,90 @@ static void test_PBM_STEPIT(void)
     }
 }
 
+static WNDPROC old_proc;
+
+static const struct message paint_pbm_setstate_seq[] =
+{
+    {PBM_SETSTATE, sent},
+    {WM_PAINT, sent},
+    {WM_ERASEBKGND, sent | defwinproc},
+    {0}
+};
+static const struct message pbm_setstate_seq[] =
+{
+    {PBM_SETSTATE, sent},
+    {0}
+};
+
+static LRESULT WINAPI test_pbm_setstate_proc(HWND hwnd, UINT message, WPARAM wp, LPARAM lp)
+{
+    static int defwndproc_counter = 0;
+    struct message msg = {0};
+    LRESULT ret;
+
+    if (message == PBM_SETSTATE
+        || message == WM_PAINT
+        || message == WM_ERASEBKGND)
+    {
+        msg.message = message;
+        msg.flags = sent | wparam | lparam;
+        if (defwndproc_counter)
+            msg.flags |= defwinproc;
+        msg.wParam = wp;
+        msg.lParam = lp;
+        add_message(sequences, CHILD_SEQ_INDEX, &msg);
+    }
+
+    ++defwndproc_counter;
+    ret = CallWindowProcA(old_proc, hwnd, message, wp, lp);
+    --defwndproc_counter;
+    return ret;
+}
+
+void test_bar_states(void)
+{
+    HWND progress_bar;
+    int test_states[6] = {PBST_NORMAL, PBST_PAUSED, PBST_ERROR, PBST_ERROR, 0, PBFS_PARTIAL};
+    int state;
+
+    progress_bar = create_progress(0);
+
+    old_proc = (WNDPROC)SetWindowLongPtrA(progress_bar, GWLP_WNDPROC, (LONG_PTR)test_pbm_setstate_proc);
+    flush_events();
+    flush_sequences(sequences, NUM_MSG_SEQUENCES);
+
+    for (int i = 0; i < 4; i++)
+    {
+        if (i != 0)
+        {
+            state = SendMessageA(progress_bar, PBM_SETSTATE, test_states[i], 0);
+            flush_events();
+            /* No paint message if new and old state are the same */
+            if (i == 3)
+                ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
+            else
+                ok_sequence(sequences, CHILD_SEQ_INDEX, paint_pbm_setstate_seq, "PBM_SETSTATE", TRUE);
+
+            todo_wine ok(state == test_states[i-1], "Expected %d, but got %d.\n", test_states[i-1], state);
+
+        }
+        state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0);
+        todo_wine ok(state == test_states[i], "Expected %d, but got %d.\n", test_states[i], state);
+    }
+
+    /* Check bad state */
+    for (int i = 4; i < 6; i++)
+    {
+        state = SendMessageA(progress_bar, PBM_SETSTATE, test_states[i], 0);
+        ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
+        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, but got %d\n", PBST_ERROR, state);
+    }
+
+    DestroyWindow(progress_bar);
+}
+
 static void init_functions(void)
 {
     HMODULE hComCtl32 = LoadLibraryA("comctl32.dll");
@@ -337,9 +432,11 @@ START_TEST(progress)
 
     if (!load_v6_module(&ctx_cookie, &hCtx))
         return;
+    init_msg_sequences(sequences, NUM_MSG_SEQUENCES);
 
     test_setcolors();
     test_PBM_STEPIT();
+    test_bar_states();
 
     unload_v6_module(ctx_cookie, hCtx);
 
-- 
GitLab


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

[PATCH v3 2/2] comctl32/progress: Add states for progress bar.

[Jacob Czekalla]

From: Jacob Czekalla <jczekalla(a)codeweavers.com>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56352
---
 dlls/comctl32/progress.c       | 28 ++++++++++++++++++++++------
 dlls/comctl32/tests/progress.c | 14 +++++++-------
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/dlls/comctl32/progress.c b/dlls/comctl32/progress.c
index 6d556f10094..fcecd7bb293 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,20 @@ 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;
+
+    if (state != prev_state)
+        InvalidateRect(hwnd, NULL, TRUE);
+    return prev_state;
+}
+
 /***********************************************************************
  *           ProgressWindowProc
  */
@@ -569,6 +586,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 +729,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 8cfd437a98d..3930de901d6 100644
--- a/dlls/comctl32/tests/progress.c
+++ b/dlls/comctl32/tests/progress.c
@@ -379,25 +379,25 @@ void test_bar_states(void)
             flush_events();
             /* No paint message if new and old state are the same */
             if (i == 3)
-                ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
+                ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", FALSE);
             else
-                ok_sequence(sequences, CHILD_SEQ_INDEX, paint_pbm_setstate_seq, "PBM_SETSTATE", TRUE);
+                ok_sequence(sequences, CHILD_SEQ_INDEX, paint_pbm_setstate_seq, "PBM_SETSTATE", FALSE);
 
-            todo_wine ok(state == test_states[i-1], "Expected %d, but got %d.\n", test_states[i-1], state);
+            ok(state == test_states[i-1], "Expected %d, but got %d.\n", test_states[i-1], state);
 
         }
         state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0);
-        todo_wine ok(state == test_states[i], "Expected %d, but got %d.\n", test_states[i], state);
+        ok(state == test_states[i], "Expected %d, but got %d.\n", test_states[i], state);
     }
 
     /* Check bad state */
     for (int i = 4; i < 6; i++)
     {
         state = SendMessageA(progress_bar, PBM_SETSTATE, test_states[i], 0);
-        ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
-        todo_wine ok(state == 0, "Expected 0, but got %d\n", state);
+        ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", FALSE);
+        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, but got %d\n", PBST_ERROR, state);
+        ok(state == PBST_ERROR, "Expected %d, but got %d\n", PBST_ERROR, state);
     }
 
     DestroyWindow(progress_bar);
-- 
GitLab

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

[PATCH v3 2/2] comctl32/progress: Add states for progress bar.

[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=147515

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

user32:
input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000006000FC, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> +#include "vssym32.h"
>
> #include "wine/test.h"
> -
>  #include "v6util.h"
> +#include "msg.h"
>
> static HWND hProgressParentWnd;
>  static const char progressTestClass[] = "ProgressBarTestClass";
>  static BOOL (WINAPI *pInitCommonControlsEx)(const INITCOMMONCONTROLSEX*);
>
> +/* For message tests */
> +enum seq_index
> +{
> +    CHILD_SEQ_INDEX,
> +    PARENT_SEQ_INDEX,
> PARENT_SEQ_INDEX is not used.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77797

[Zhiyi Zhang (＠zhiyi)]

On Mon Aug  5 14:25:15 2024 +0000, Zhiyi Zhang wrote:
> ```
> 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.

These are still not addressed. Please make sure that each comment is addressed. You can use the resolve thread feature on Gitlab.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77798

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> }
>  }
>
> +static WNDPROC old_proc;
> +
> +static const struct message paint_pbm_setstate_seq[] =
> +{
> +    {PBM_SETSTATE, sent},
> +    {WM_PAINT, sent},
> +    {WM_ERASEBKGND, sent | defwinproc},
> +    {0}
> +};

Let's add a new line after this.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77799

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> +            msg.flags |= defwinproc;
> +        msg.wParam = wp;
> +        msg.lParam = lp;
> +        add_message(sequences, CHILD_SEQ_INDEX, &msg);
> +    }
> +
> +    ++defwndproc_counter;
> +    ret = CallWindowProcA(old_proc, hwnd, message, wp, lp);
> +    --defwndproc_counter;
> +    return ret;
> +}
> +
> +void test_bar_states(void)
> +{
> +    HWND progress_bar;
> +    int test_states[6] = {PBST_NORMAL, PBST_PAUSED, PBST_ERROR, PBST_ERROR, 0, PBFS_PARTIAL};
> I think it would be cleaner if you organize the test data like the following
static const struct
{
DWORD state;
DWORD previous_state;
const struct message *expected_seq;
BOOL todo;
}
tests[] =
{
{0, 0, pbm_setstate_seq},
{PBST_NORMAL, PBST_NORMAL, pbm_setstate_seq},
{PBST_PAUSED, PBST_NORMAL, paint_pbm_setstate_seq},
{PBST_PAUSED, PBST_PAUSED, pbm_setstate_seq},
{PBST_ERROR, PBST_PAUSED, paint_pbm_setstate_seq},
{PBST_ERROR, PBST_ERROR, pbm_setstate_seq},
{PBFS_PARTIAL, 0, pbm_setstate_seq},
}

Then in a loop. You first set to tests[i].state, expected tests[i].previous_state returned from PBM_SETSTATE, expect the current state being tests[i].state from PBM_GETSTATE and expect the result message sequence to be tests[i].expected_seq. Then add todo_wine_if(tests[i].todo) as needed. This way, you can avoid the if conditions in the code.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77800

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> +            state = SendMessageA(progress_bar, PBM_SETSTATE, test_states[i], 0);
> +            flush_events();
> +            /* No paint message if new and old state are the same */
> +            if (i == 3)
> +                ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
> +            else
> +                ok_sequence(sequences, CHILD_SEQ_INDEX, paint_pbm_setstate_seq, "PBM_SETSTATE", TRUE);
> +
> +            todo_wine ok(state == test_states[i-1], "Expected %d, but got %d.\n", test_states[i-1], state);
> +
> +        }
> +        state = SendMessageA(progress_bar, PBM_GETSTATE, 0, 0);
> +        todo_wine ok(state == test_states[i], "Expected %d, but got %d.\n", test_states[i], state);
> +    }
> +
> +    /* Check bad state */
> These are not interesting. Having tests for PBFS_PARTIAL is enough.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77801

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> +    flush_sequences(sequences, NUM_MSG_SEQUENCES);
> +
> +    for (int i = 0; i < 4; i++)
> +    {
> +        if (i != 0)
> +        {
> +            state = SendMessageA(progress_bar, PBM_SETSTATE, test_states[i], 0);
> +            flush_events();
> +            /* No paint message if new and old state are the same */
> +            if (i == 3)
> +                ok_sequence(sequences, CHILD_SEQ_INDEX, pbm_setstate_seq, "PBM_SETSTATE", TRUE);
> +            else
> +                ok_sequence(sequences, CHILD_SEQ_INDEX, paint_pbm_setstate_seq, "PBM_SETSTATE", TRUE);
> +
> +            todo_wine ok(state == test_states[i-1], "Expected %d, but got %d.\n", test_states[i-1], state);
> +
> Delete this empty line at the end.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77802

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/progress.c:
> if (!load_v6_module(&ctx_cookie, &hCtx))
>          return;
> +    init_msg_sequences(sequences, NUM_MSG_SEQUENCES);
>
> test_setcolors();
>      test_PBM_STEPIT();
> +    test_bar_states();

Let add the tests for comctl32 v5 as well.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6186#note_77803