[PATCH] comctl32/progress: Fix wrapping of values in PBM_STEPIT and add tests

[Fabian Maurer]

Signed-off-by: Fabian Maurer 
---
 dlls/comctl32/progress.c       |  2 +-
 dlls/comctl32/tests/progress.c | 47 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/dlls/comctl32/progress.c b/dlls/comctl32/progress.c
index 80cced4c66..2c952c2f7b 100644
--- a/dlls/comctl32/progress.c
+++ b/dlls/comctl32/progress.c
@@ -656,7 +656,7 @@ static LRESULT WINAPI ProgressWindowProc(HWND hwnd, UINT message,
         oldVal = infoPtr->CurVal;
         infoPtr->CurVal += infoPtr->Step;
         if(infoPtr->CurVal > infoPtr->MaxVal)
-	    infoPtr->CurVal = infoPtr->MinVal;
+	    infoPtr->CurVal = infoPtr->CurVal % infoPtr->MaxVal;
         if(oldVal != infoPtr->CurVal)
    {
        TRACE("PBM_STEPIT: current pos changed from %d to %d\n", oldVal, infoPtr->CurVal);
diff --git a/dlls/comctl32/tests/progress.c b/dlls/comctl32/tests/progress.c
index 9dd4b55202..d0d57a5170 100644
--- a/dlls/comctl32/tests/progress.c
+++ b/dlls/comctl32/tests/progress.c
@@ -237,6 +237,52 @@ static void test_setcolors(void)
     DestroyWindow(progress);
 }

+static void test_wrapping(void)
+{
+    const int TEST_MAXIMUM = 12;
+    const int TEST_STEP_SMALL = 5;
+    const int TEST_STEP_BIG = 50;
+    HWND progress;
+    int pos_actual;
+    int pos_expected;
+    int i;
+
+    progress = create_progress(0);
+    SendMessageA(progress, PBM_SETRANGE32, 0, TEST_MAXIMUM);
+
+    /* Test PBM_STEPIT with small steps */
+
+    SendMessageA(progress, PBM_SETSTEP, TEST_STEP_SMALL, 0);
+    SendMessageA(progress, PBM_STEPIT, 0, 0);
+    for (i = 1; i < 15; i++)
+    {
+        pos_actual = SendMessageA(progress, PBM_STEPIT, 0, 0);
+        pos_expected = (i * TEST_STEP_SMALL) % TEST_MAXIMUM;
+        if (pos_expected == 0)
+            pos_expected = TEST_MAXIMUM;
+        ok(pos_actual == pos_expected, "Run %d: Expected %d, got %d\n", i, pos_expected, pos_actual);
+    }
+
+    /* Test PBM_STEPIT with a step that is multiple times the maximum */
+
+    SendMessageA(progress, PBM_SETPOS, 0, 0);
+    SendMessageA(progress, PBM_SETSTEP, TEST_STEP_BIG, 0);
+    SendMessageA(progress, PBM_STEPIT, 0, 0);
+    pos_actual = SendMessageA(progress, PBM_GETPOS, 0, 0);
+    pos_expected = TEST_STEP_BIG % TEST_MAXIMUM;
+    ok(pos_actual == pos_expected, "Expected %d, got %d\n", pos_expected, pos_actual);
+
+    /* Test PBM_DELTAPOS */
+
+    SendMessageA(progress, PBM_SETPOS, 0, 0);
+    SendMessageA(progress, PBM_DELTAPOS, 15, 0);
+    pos_actual = SendMessageA(progress, PBM_GETPOS, 0, 0);
+    pos_expected = TEST_MAXIMUM;
+    ok(pos_actual == pos_expected, "Expected %d, got %d\n", pos_expected, pos_actual);
+
+    DestroyWindow(progress);
+}
+
 static void init_functions(void)
 {
     HMODULE hComCtl32 = LoadLibraryA("comctl32.dll");
@@ -260,6 +306,7 @@ START_TEST(progress)

test_redraw();
     test_setcolors();
+    test_wrapping();

cleanup();
 }

[Nikolay Sivov]

On 2/27/2018 12:14 AM, Fabian Maurer wrote:
> @@ -656,7 +656,7 @@ static LRESULT WINAPI ProgressWindowProc(HWND hwnd, UINT message,
>          oldVal = infoPtr->CurVal;
>          infoPtr->CurVal += infoPtr->Step;
>          if(infoPtr->CurVal > infoPtr->MaxVal)
> -
>    infoPtr->CurVal = infoPtr->MinVal;
> -
>    infoPtr->CurVal = infoPtr->CurVal % infoPtr->MaxVal;

This looks wrong. I don't think MinVal should be ignored. Your tests
don't show it because you have [0, 12] range.
> +static void test_wrapping(void)

It should be test_PBM_STEPIT().
> +{
> - const int TEST_MAXIMUM = 12;
> - const int TEST_STEP_SMALL = 5;
> - const int TEST_STEP_BIG = 50;

Constants are not helping here I think, it's easier and is more readable
to have them inlined.

Do we have a bug report for this issue by any chance?

[Fabian Maurer]

On Montag, 26. Februar 2018 23:15:04 CET Nikolay Sivov wrote:
> On 2/27/2018 12:14 AM, Fabian Maurer wrote:
> > @@ -656,7 +656,7 @@ static LRESULT WINAPI ProgressWindowProc(HWND hwnd,
> > UINT message,> 
> >          oldVal = infoPtr->CurVal;
> >          infoPtr->CurVal += infoPtr->Step;
> >          if(infoPtr->CurVal > infoPtr->MaxVal)
> > -
> >    infoPtr->CurVal = infoPtr->MinVal;
> > -
> >    infoPtr->CurVal = infoPtr->CurVal % infoPtr->MaxVal;
>
> This looks wrong. I don't think MinVal should be ignored. Your tests
> don't show it because you have [0, 12] range.

Thanks, completely overlooked that. I rewrote the tests to account for that - and also to 
account for negative stepping values.
> > +static void test_wrapping(void)
>
> It should be test_PBM_STEPIT().
> > +{
> > - const int TEST_MAXIMUM = 12;
> > - const int TEST_STEP_SMALL = 5;
> > - const int TEST_STEP_BIG = 50;
>
> Constants are not helping here I think, it's easier and is more readable
> to have them inlined.

Thanks for the info, changed it.
> Do we have a bug report for this issue by any chance?

We don't, but reactos has it (for wine code): https://jira.reactos.org/browse/CORE-13873

Regards,
Fabian Maurer

[Nikolay Sivov]

On 2/27/2018 12:14 AM, Fabian Maurer wrote:
> -
> - SendMessageA(progress, PBM_SETSTEP, TEST_STEP_SMALL, 0);
> - SendMessageA(progress, PBM_STEPIT, 0, 0);

Maybe have this as SETPOS instead?
> - for (i = 1; i < 15; i++)
> - {
> -
>    pos_actual = SendMessageA(progress, PBM_STEPIT, 0, 0);

That's previous position, not actual/current.

[Fabian Maurer]

On Montag, 26. Februar 2018 23:21:07 CET Nikolay Sivov wrote:
> On 2/27/2018 12:14 AM, Fabian Maurer wrote:
> > -
> > - SendMessageA(progress, PBM_SETSTEP, TEST_STEP_SMALL, 0);
> > - SendMessageA(progress, PBM_STEPIT, 0, 0);
>
> Maybe have this as SETPOS instead?
> > - for (i = 1; i < 15; i++)
> > - {
> > -
> >    pos_actual = SendMessageA(progress, PBM_STEPIT, 0, 0);
>
> That's previous position, not actual/current.

I thought it's easier to work with that way, I've changed it in the rewrite of the patch.

Regards,
Fabian Maurer