user32: Add additional tests for scroll state, make them pass under Wine

[Alex Villací­s Lasso]

This patch adds a few tests for scrollbar behavior in Windows. It shows 
that if the window was created with at least WS_VSCROLL or WS_HSCROLL 
styles, WinXP returns default information for scrollbar range, but Win98 
returns an error, until first initialized. This error does not change if 
styles are set or cleared before initializing ranges. On both versions, 
setting either scrollbar range automatically initializes both if 
required (Wine still returned error for the other scrollbar - fix 
included). I have tried to test this on Win98 and WinXP-SP2, but maybe 
other versions are different. Could you please test this patch? Also, is 
it OK to tell apart Win9x vs. WinXP behavior in the way it is done in 
this patch? Once either behavior is identified, it is consistent on all 
future tests, as shown by the tests. Wine fixed to implement Win98 behavior.

Changelog:

* Document and test differences between WinXP and Win98 scrollbar behavior
* Make tests pass under Wine (Win98 behavior)

user32: Add additional tests for scroll state, make them pass under Wine

[James Hawkins]

On Mon, Jun 30, 2008 at 3:34 PM, Alex Villací­s Lasso
 wrote:
> This patch adds a few tests for scrollbar behavior in Windows. It shows that
> if the window was created with at least WS_VSCROLL or WS_HSCROLL styles,
> WinXP returns default information for scrollbar range, but Win98 returns an
> error, until first initialized. This error does not change if styles are set
> or cleared before initializing ranges. On both versions, setting either
> scrollbar range automatically initializes both if required (Wine still
> returned error for the other scrollbar - fix included). I have tried to test
> this on Win98 and WinXP-SP2, but maybe other versions are different. Could
> you please test this patch? Also, is it OK to tell apart Win9x vs. WinXP
> behavior in the way it is done in this patch? Once either behavior is
> identified, it is consistent on all future tests, as shown by the tests.
> Wine fixed to implement Win98 behavior.

Why in the world would you implement Win98 behavior over WinXP,
especially since the WinXP behavior is more logical and (probably,
haven't looked) stated on msdn?

+    if (!(hBarEnabled || vBarEnabled)) {
+        ok(!r2, "GetScrollInfo incorrectly succeeded\n");
+        ok((   r == ERROR_NO_SCROLLBARS /* WinXP */
+            || r == 0xdeadbeef          /* Win98 */ ),
+            "GetLastError returned 0x%08lx expected 0x%08x
(ERROR_NO_SCROLLBARS or 0xdeadbeef)\n",
+            r, ERROR_NO_SCROLLBARS);

That's hideous spacing just for the sake of lining up some error codes.

+        ok(si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected
0xdeadbeef\n", si.nMin);
+        ok(si.nMax == 0xdeadbeef, "si.nMax == 0x%x, expected
0xdeadbeef\n", si.nMax);
+        ok(si.nPos == 0xdeadbeef, "si.nPos == 0x%x, expected
0xdeadbeef\n", si.nPos);
+        ok(si.nPage == 0xdeadbeef, "si.nPage == 0x%x, expected
0xdeadbeef\n", si.nPage);
+
+        if (r == ERROR_NO_SCROLLBARS) behavior_winxp = TRUE;
+        if (r == 0xdeadbeef) behavior_win98 = TRUE;
+    } else {
+        if (r2) {
+            ok(si.nMin == 0, "si.nMin == %d, expected 0\n", si.nMin);
+            ok(si.nMax == 100, "si.nMax == %d, expected 100\n", si.nMax);
+            ok(si.nPos == 0, "si.nPos == %d, expected 0\n", si.nPos);
+            ok(si.nPage == 0, "si.nPage == %d, expected 0\n", si.nPage);
+
+            behavior_winxp = TRUE;
+        } else {
+            ok(r == 0xdeadbeef, "GetScrollInfo failed, error is %ld
(0x%08lx) expected 0xdeadbeef\n", r, r);
+
+            behavior_win98 = TRUE;
+        }
+    }

This is seriously complex (in a bad way).  I think you've tried to
factor too much into one function.  You don't need to set and check
the version, just test for both cases that are expected, like all the
other tests:

ok(si.nMin == 0 ||
     si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected 0 or
0xdeadbeef\n", si.nMin);

user32: Add additional tests for scroll state, make them pass under Wine

[Reece Dunn]

2008/6/30 James Hawkins :
> On Mon, Jun 30, 2008 at 3:34 PM, Alex Villací­s Lasso
>  wrote:
> > ...
> -
>    ok(si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected
>
> 0xdeadbeef\n", si.nMin);
> -
>    ok(si.nMax == 0xdeadbeef, "si.nMax == 0x%x, expected
>
> 0xdeadbeef\n", si.nMax);
> -
>    ok(si.nPos == 0xdeadbeef, "si.nPos == 0x%x, expected
>
> 0xdeadbeef\n", si.nPos);
> -
>    ok(si.nPage == 0xdeadbeef, "si.nPage == 0x%x, expected
>
> 0xdeadbeef\n", si.nPage);
> -
> -
>    if (r == ERROR_NO_SCROLLBARS) behavior_winxp = TRUE;
> -
>    if (r == 0xdeadbeef) behavior_win98 = TRUE;
> - } else {
> -
>    if (r2) {
> -
>        ok(si.nMin == 0, "si.nMin == %d, expected 0\n", si.nMin);
> -
>        ok(si.nMax == 100, "si.nMax == %d, expected 100\n", si.nMax);
> -
>        ok(si.nPos == 0, "si.nPos == %d, expected 0\n", si.nPos);
> -
>        ok(si.nPage == 0, "si.nPage == %d, expected 0\n", si.nPage);
> -
> -
>        behavior_winxp = TRUE;
> -
>    } else {
> -
>        ok(r == 0xdeadbeef, "GetScrollInfo failed, error is %ld
>
> (0x%08lx) expected 0xdeadbeef\n", r, r);
> -
> -
>        behavior_win98 = TRUE;
> -
>    }
> - }
>
> This is seriously complex (in a bad way).  I think you've tried to
> factor too much into one function.  You don't need to set and check
> the version, just test for both cases that are expected, like all the
> other tests:
>
> ok(si.nMin == 0 ||
>     si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected 0 or
> 0xdeadbeef\n", si.nMin);

Francois Gouget also implemented a broken() method so you can get Wine
to do the right thing, something like:

ok(si.nMin == 0 /* XP */ ||
     broken(si.nMin == 0xdeadbeef) /* 98 */ ,
     "si.nMin == 0x%x, expected 0 or 0xdeadbeef\n", si.nMin);

- Reece

user32: Add additional tests for scroll state, make them pass under Wine

[Alex Villací­s Lasso]

> Why in the world would you implement Win98 behavior over WinXP,
> especially since the WinXP behavior is more logical and (probably,
> haven't looked) stated on msdn?

I did not implement Win98 behavior - it was already implemented that way 
in Wine, except for one difference that was found and changed to be 
consistent. Having said that, I agree that it is better to implement the 
WinXP behavior. I will do that in a future patch.

Reece Dunn escribió:
> 2008/6/30 James Hawkins :
> > On Mon, Jun 30, 2008 at 3:34 PM, Alex Villací­s Lasso
> >  wrote:
> > > ...
> > -
> >    ok(si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected
> >
> > 0xdeadbeef\n", si.nMin);
> > -
> >    ok(si.nMax == 0xdeadbeef, "si.nMax == 0x%x, expected
> >
> > 0xdeadbeef\n", si.nMax);
> > -
> >    ok(si.nPos == 0xdeadbeef, "si.nPos == 0x%x, expected
> >
> > 0xdeadbeef\n", si.nPos);
> > -
> >    ok(si.nPage == 0xdeadbeef, "si.nPage == 0x%x, expected
> >
> > 0xdeadbeef\n", si.nPage);
> > -
> > -
> >    if (r == ERROR_NO_SCROLLBARS) behavior_winxp = TRUE;
> > -
> >    if (r == 0xdeadbeef) behavior_win98 = TRUE;
> > - } else {
> > -
> >    if (r2) {
> > -
> >        ok(si.nMin == 0, "si.nMin == %d, expected 0\n", si.nMin);
> > -
> >        ok(si.nMax == 100, "si.nMax == %d, expected 100\n", si.nMax);
> > -
> >        ok(si.nPos == 0, "si.nPos == %d, expected 0\n", si.nPos);
> > -
> >        ok(si.nPage == 0, "si.nPage == %d, expected 0\n", si.nPage);
> > -
> > -
> >        behavior_winxp = TRUE;
> > -
> >    } else {
> > -
> >        ok(r == 0xdeadbeef, "GetScrollInfo failed, error is %ld
> >
> > (0x%08lx) expected 0xdeadbeef\n", r, r);
> > -
> > -
> >        behavior_win98 = TRUE;
> > -
> >    }
> > - }
> >
> > This is seriously complex (in a bad way).  I think you've tried to
> > factor too much into one function.  You don't need to set and check
> > the version, just test for both cases that are expected, like all the
> > other tests:
> >
> > ok(si.nMin == 0 ||
> >     si.nMin == 0xdeadbeef, "si.nMin == 0x%x, expected 0 or
> > 0xdeadbeef\n", si.nMin);
>
> Francois Gouget also implemented a broken() method so you can get Wine
> to do the right thing, something like:
>
> ok(si.nMin == 0 /* XP */ ||
>      broken(si.nMin == 0xdeadbeef) /* 98 */ ,
>      "si.nMin == 0x%x, expected 0 or 0xdeadbeef\n", si.nMin);
> - Reece

Thanks for the reminder. I will use it.