Mouse-buttons swapped after winetest

[Paul Vriens]

Hi,

with the latest winetest
http://www.astro.gla.ac.uk/users/paulm/WRT/CrossBuilt/winetest-200506211000-...

my mouse-buttons (left and right) are swapped after the test.

Any idea??

Cheers,

Paul.

[Paul Vriens]

On Tue, 2005-06-21 at 21:45, Paul Vriens wrote:
> Hi,
>
> with the latest winetest
> http://www.astro.gla.ac.uk/users/paulm/WRT/CrossBuilt/winetest-200506211000-...
>
> my mouse-buttons (left and right) are swapped after the test.
>
> Any idea??
>
> Cheers,
>
> Paul.

and to add to that, this is for Win98/NT/W2KProf and WinXP even though
the test output on test.winehq.org:

http://test.winehq.org/data/200506211000/nt4_PaulVriensNT4Srv/user32:syspara...

says:

sysparams.c:1005:testing SPI_{GET,SET}MOUSEBUTTONSWAP
sysparams.c:171:SPI_{GET,SET}MOUSEBUTTONSWAP not supported on this platform. Skipping test

Paul.

[Saulius Krasuckas]

* On Tue, 21 Jun 2005, Paul Vriens wrote:
> - On Tue, 2005-06-21 at 21:45, Paul Vriens wrote:
> > with the latest winetest
> > http://www.astro.gla.ac.uk/users/paulm/WRT/CrossBuilt/winetest-200506211000-...
> >
> > my mouse-buttons (left and right) are swapped after the test.

...
> sysparams.c:1005:testing SPI_{GET,SET}MOUSEBUTTONSWAP
> sysparams.c:171:SPI_{GET,SET}MOUSEBUTTONSWAP not supported on this platform. Skipping test

IMHO the bug is caused by a patch of Vitaly Lipatov [1], which by itself 
showed up a bug, caused by a combined patch from you, Paul, and Justin 
Chevrier [2].

Following [1]:

| @@ -1010,7 +1010,7 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
|          SetLastError(0xdeadbeef);
|          rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, vals[i], 0,
|                                    SPIF_UPDATEINIFILE | SPIF_SENDCHANGE );
| -        if (!test_error_msg(rc,"SPI_{GET,SET}MOUSEBUTTONSWAP"))
| +        if (!test_error_msg(rc!=vals[i],"SPI_{GET,SET}MOUSEBUTTONSWAP"))
|              return;
  ...
| @@ -1023,7 +1023,7 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
|  
|      rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, old_b, 0,
|                                SPIF_UPDATEINIFILE );
| -    ok(rc!=0,"***warning*** failed to restore the original value: rc=%d err=%ld\n",rc,GetLastError());
| +    ok(!rc,"***warning*** failed to restore the original value: rc=%d err=%ld\n",rc,GetLastError());

Such checks doesn't work on winME.  I have reverted them and tests is 
doing fine.  Further, following [2]:

| @@ -955,9 +1007,12 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
|  
|      for (i=0;i<sizeof(vals)/sizeof(*vals);i++)
|      {
| +        SetLastError(0xdeadbeef);
|          rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, vals[i], 0,
|                                    SPIF_UPDATEINIFILE | SPIF_SENDCHANGE );
| -        ok(rc!=0,"%d: rc=%d err=%ld\n",i,rc,GetLastError());
| +        if (!test_error_msg(rc,"SPI_{GET,SET}MOUSEBUTTONSWAP"))
| +            return;

This wrong, IMHO, as the "return" statement eliminates a possibility to 
restore SM_SWAPBUTTON metrics after the for-loop ends.  I would use 
"break" or "continue" here.

Then would be nice to print "err" in following format "err=%0x%08lx" or 
such after a restoration.  Any more ideas?

[1] http://www.winehq.org/hypermail/wine-cvs/2005/06/0474.html
    http://cvs.winehq.org/cvsweb/wine/dlls/user/tests/sysparams.c.diff?r1=1.37&a...
[2] http://www.winehq.org/hypermail/wine-cvs/2005/03/0110.html
    http://cvs.winehq.org/cvsweb/wine/dlls/user/tests/sysparams.c.diff?r1=1.34&a...

[Paul Vriens]

On Fri, 2005-06-24 at 17:44, Saulius Krasuckas wrote:
> - On Tue, 21 Jun 2005, Paul Vriens wrote:
> > - On Tue, 2005-06-21 at 21:45, Paul Vriens wrote:
> > > with the latest winetest
> > > http://www.astro.gla.ac.uk/users/paulm/WRT/CrossBuilt/winetest-200506211000-...
> > >
> > > my mouse-buttons (left and right) are swapped after the test.
>
> ...
> > sysparams.c:1005:testing SPI_{GET,SET}MOUSEBUTTONSWAP
> > sysparams.c:171:SPI_{GET,SET}MOUSEBUTTONSWAP not supported on this platform. Skipping test
>
> IMHO the bug is caused by a patch of Vitaly Lipatov [1], which by itself 
> showed up a bug, caused by a combined patch from you, Paul, and Justin 
> Chevrier [2].
>
> Following [1]:
>
> | @@ -1010,7 +1010,7 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
> |          SetLastError(0xdeadbeef);
> |          rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, vals[i], 0,
> |                                    SPIF_UPDATEINIFILE | SPIF_SENDCHANGE );
> | -        if (!test_error_msg(rc,"SPI_{GET,SET}MOUSEBUTTONSWAP"))
> | +        if (!test_error_msg(rc!=vals[i],"SPI_{GET,SET}MOUSEBUTTONSWAP"))
> |              return;
>   ...
> | @@ -1023,7 +1023,7 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
> |  
> |      rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, old_b, 0,
> |                                SPIF_UPDATEINIFILE );
> | -    ok(rc!=0,"***warning*** failed to restore the original value: rc=%d err=%ld\n",rc,GetLastError());
> | +    ok(!rc,"***warning*** failed to restore the original value: rc=%d err=%ld\n",rc,GetLastError());
>
> Such checks doesn't work on winME.  I have reverted them and tests is 
> doing fine.  Further, following [2]:

I agree these changes don't make sense. Although Vitaly says in his
patch:
/* MSDN says it returns previous state of swapbutton always */
it doesn't seem to be the case for at least win98/winME/NT/W2K and XP
(which is almost all versions !)
Reverting the above fixes, it for me as well.
> | @@ -955,9 +1007,12 @@ static void test_SPI_SETMOUSEBUTTONSWAP(
> |  
> |      for (i=0;i<sizeof(vals)/sizeof(*vals);i++)
> |      {
> | +        SetLastError(0xdeadbeef);
> |          rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP, vals[i], 0,
> |                                    SPIF_UPDATEINIFILE | SPIF_SENDCHANGE );
> | -        ok(rc!=0,"%d: rc=%d err=%ld\n",i,rc,GetLastError());
> | +        if (!test_error_msg(rc,"SPI_{GET,SET}MOUSEBUTTONSWAP"))
> | +            return;
>
> This wrong, IMHO, as the "return" statement eliminates a possibility to 
> restore SM_SWAPBUTTON metrics after the for-loop ends.  I would use 
> "break" or "continue" here.

I agree that this can be done nicer, but I'd like to have some comments
from Vitaly about his patch before changing anything
> Then would be nice to print "err" in following format "err=%0x%08lx" or 
> such after a restoration.  Any more ideas?

Why do you want to change that? I thought %08lx is pretty common in most
of the Wine code. (I remember seeing your email on wine-devel about that
though).

Cheers,

Paul.

[Vitaly Lipatov]

On Saturday 25 June 2005 00:28, Paul Vriens wrote:
> > IMHO the bug is caused by a patch of Vitaly Lipatov [1],
> > which by itself showed up a bug, caused by a combined patch
> > from you, Paul, and Justin Chevrier [2].

...
> > | @@ -1023,7 +1023,7 @@ static void
> > | test_SPI_SETMOUSEBUTTONSWAP(
> > |
> > |      rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP,
> > | old_b, 0, SPIF_UPDATEINIFILE ); -   
> > | ok(rc!=0,"***warning*** failed to restore the original
> > | value: rc=%d err=%ld\n",rc,GetLastError()); +   
> > | ok(!rc,"***warning*** failed to restore the original
> > | value: rc=%d err=%ld\n",rc,GetLastError());
> >
> > Such checks doesn't work on winME.  I have reverted them and
> > tests is doing fine.  Further, following [2]:
>
> I agree these changes don't make sense. Although Vitaly says
> in his patch:
> /* MSDN says it returns previous state of swapbutton always */
> it doesn't seem to be the case for at least win98/winME/NT/W2K
> and XP (which is almost all versions !)

Some strange thing. I am surprised.
> Reverting the above fixes, it for me as well.

Please fix the test to be WinXX conformed.
> > | @@ -955,9 +1007,12 @@ static void
> > | test_SPI_SETMOUSEBUTTONSWAP(
> > |
> > |      for (i=0;i<sizeof(vals)/sizeof(*vals);i++)
> > |      {
> > | +        SetLastError(0xdeadbeef);
> > |          rc=SystemParametersInfoA( SPI_SETMOUSEBUTTONSWAP,
> > | vals[i], 0, SPIF_UPDATEINIFILE | SPIF_SENDCHANGE ); -     
> > |   ok(rc!=0,"%d: rc=%d err=%ld\n",i,rc,GetLastError()); +  
> > |      if
> > | (!test_error_msg(rc,"SPI_{GET,SET}MOUSEBUTTONSWAP")) +    
> > |        return;
> >
> > This wrong, IMHO, as the "return" statement eliminates a
> > possibility to restore SM_SWAPBUTTON metrics after the
> > for-loop ends.  I would use "break" or "continue" here.

Break will be fine here.
> I agree that this can be done nicer, but I'd like to have some
> comments from Vitaly about his patch before changing anything

1. I believed (and tested) its returns previous state. But 
"errare humanum est".
2. The present behavior is needed for MS Office 97 runs correctly 
in WINE.
3. I see there is some problems with test
4. I do not think MSDN lies about _this_ message

Sorry, I will come back in 10 days only. Feel free make any 
changes. I will tested it anyway.
Sorry for inconvenience.

[Saulius Krasuckas]

* On Fri, 24 Jun 2005, Paul Vriens wrote:
> - On Fri, 2005-06-24 at 17:44, Saulius Krasuckas wrote:
> > Then would be nice to print "err" in following format "err=%0x%08lx" or 
> > such after a restoration.  Any more ideas?

Sorry, I meant 0x%08lx.
> Why do you want to change that? I thought %08lx is pretty common in most 
> of the Wine code. (I remember seeing your email on wine-devel about that 
> though).

I just see two versions of formatting in Wine and want to get rid of one 
of it.  Not a large difference which.

OTOH, which one case brings you more clearness while reading logs: 
00000021 or 0x0000021 ?  I prefer the latter one.

[Vitaly Lipatov]

Hi,
Can you pay attention to this patch? It really fixes the problem but not 
discussed in the list and not applied:
http://www.winehq.com/hypermail/wine-patches/2005/08/0018.html

В сообщении от 25 Июнь 2005 00:28 Paul Vriens написал(a):
> On Fri, 2005-06-24 at 17:44, Saulius Krasuckas wrote:

...
> > Such checks doesn't work on winME.  I have reverted them and tests is
> > doing fine.  Further, following [2]:
>
> I agree these changes don't make sense. Although Vitaly says in his
> patch:
> /* MSDN says it returns previous state of swapbutton always */
> it doesn't seem to be the case for at least win98/winME/NT/W2K and XP
> (which is almost all versions !)
> Reverting the above fixes, it for me as well.