Re: [PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 2)

[Rico Schüller]

I think we also need a test to prove that "return MapWindowPoints( hwnd, 
0, lppnt, 1 ) != 0;" is wrong. I'd probably split the changes to 
ScreenToClient/ClientToScreen and MapWindowPoints, because they are not 
necessarily bound together.

What about something like:
wnd2 = CreateWindow("static", "wnd2", WS_POPUP, 0, 0, 150, 150, NULL, 
NULL, NULL, NULL);
ok(wnd2 != NULL, "CreateWindow failed\n");
SetLastError(0xdeadbeef);
p.x = p.y = 0;
ret = ScreenToClient(wnd2, &p);
err = GetLastError();
ok(ret, "ScreenToClient() failed\n");
ok(p.x == 0 && p.y == 0, "Failed got(%d, %d), expected (%d, %d)\n", p.x, 
p.y, 0, 0);
ok(err == 0xdeadbeef, "GetLastError failed, got %x, expected %x\n", err, 
0xdeadbeef);

Also msdn says: "Do not use ScreenToClient when in a mirroring 
situation, that is, when changing from left-to-right layout to 
right-to-left layout. Instead, use MapWindowPoints. For more 
information, see "Window Layout and Mirroring" in Window Features." We 
may also check that case. So they may not share the exact same steps.

Cheers
Rico

On 20.10.2012 12:27, Christian Costa wrote:
> Based ont a patch of Rico Schuller.
>
> Try 2:
>    - Test window handle in ClienToScreen and ScreenToClient instead of relying on GetLastError().
>    - Don't rename params.
> ---
>   dlls/user32/tests/win.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++
>   dlls/user32/winpos.c    |   28 +++++-
>   2 files changed, 237 insertions(+), 4 deletions(-)

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 2)

[Christian Costa]

Ok. I'll add it. Thanks!

Le 20/10/2012 21:35, Rico Schüller a écrit :
> I think we also need a test to prove that "return MapWindowPoints( 
> hwnd, 0, lppnt, 1 ) != 0;" is wrong. I'd probably split the changes to 
> ScreenToClient/ClientToScreen and MapWindowPoints, because they are 
> not necessarily bound together.
>
> What about something like:
> wnd2 = CreateWindow("static", "wnd2", WS_POPUP, 0, 0, 150, 150, NULL, 
> NULL, NULL, NULL);
> ok(wnd2 != NULL, "CreateWindow failed\n");
> SetLastError(0xdeadbeef);
> p.x = p.y = 0;
> ret = ScreenToClient(wnd2, &p);
> err = GetLastError();
> ok(ret, "ScreenToClient() failed\n");
> ok(p.x == 0 && p.y == 0, "Failed got(%d, %d), expected (%d, %d)\n", 
> p.x, p.y, 0, 0);
> ok(err == 0xdeadbeef, "GetLastError failed, got %x, expected %x\n", 
> err, 0xdeadbeef);
>
> Also msdn says: "Do not use ScreenToClient when in a mirroring 
> situation, that is, when changing from left-to-right layout to 
> right-to-left layout. Instead, use MapWindowPoints. For more 
> information, see "Window Layout and Mirroring" in Window Features." We 
> may also check that case. So they may not share the exact same steps.
>
> Cheers
> Rico
>
> On 20.10.2012 12:27, Christian Costa wrote:
> > Based ont a patch of Rico Schuller.
> >
> > Try 2:
> >    - Test window handle in ClienToScreen and ScreenToClient instead 
> > of relying on GetLastError().
> >    - Don't rename params.
> > ---
> >   dlls/user32/tests/win.c |  213 
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >   dlls/user32/winpos.c    |   28 +++++-
> >   2 files changed, 237 insertions(+), 4 deletions(-)

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 2)

[Rico Schüller]

Well, I just like to add my favorite version for now. It handles only 
ClientToScreen and ScreenToClient and fixes the return values. It 
doesn't touch MapWindowPoints and it doesn't check for the comment on 
msdn. I think those both extra points belong into separate patches. 
Feedback is welcome.

Cheers
Rico


On 20.10.2012 22:58, Christian Costa wrote:
> Ok. I'll add it. Thanks!
>
> Le 20/10/2012 21:35, Rico Schüller a écrit :
> > I think we also need a test to prove that "return MapWindowPoints(
> > hwnd, 0, lppnt, 1 ) != 0;" is wrong. I'd probably split the changes to
> > ScreenToClient/ClientToScreen and MapWindowPoints, because they are
> > not necessarily bound together.
> >
> > What about something like:
> > wnd2 = CreateWindow("static", "wnd2", WS_POPUP, 0, 0, 150, 150, NULL,
> > NULL, NULL, NULL);
> > ok(wnd2 != NULL, "CreateWindow failed\n");
> > SetLastError(0xdeadbeef);
> > p.x = p.y = 0;
> > ret = ScreenToClient(wnd2, &p);
> > err = GetLastError();
> > ok(ret, "ScreenToClient() failed\n");
> > ok(p.x == 0 && p.y == 0, "Failed got(%d, %d), expected (%d, %d)\n",
> > p.x, p.y, 0, 0);
> > ok(err == 0xdeadbeef, "GetLastError failed, got %x, expected %x\n",
> > err, 0xdeadbeef);
> >
> > Also msdn says: "Do not use ScreenToClient when in a mirroring
> > situation, that is, when changing from left-to-right layout to
> > right-to-left layout. Instead, use MapWindowPoints. For more
> > information, see "Window Layout and Mirroring" in Window Features." We
> > may also check that case. So they may not share the exact same steps.

[PATCH] user32: Fix error handling in MapWindowPoints, ClientToScreen and ScreenToClient and add tests for them. (try 2)

[Christian Costa]

I don't mind working on this issue or let you do it but it's not worth 
working both on or review very different versions of the patch that are 
not converging.
The fact the patch gets in does not prevent to add further tests 
afterwards. Note that in the version I sent based on your work I grouped 
tests for each of 3 functions
for better readability.


Le 21/10/2012 13:23, Rico Schüller a écrit :
> Well, I just like to add my favorite version for now. It handles only 
> ClientToScreen and ScreenToClient and fixes the return values. It 
> doesn't touch MapWindowPoints and it doesn't check for the comment on 
> msdn. I think those both extra points belong into separate patches. 
> Feedback is welcome.
>
> Cheers
> Rico
>
> On 20.10.2012 22:58, Christian Costa wrote:
> > Ok. I'll add it. Thanks!
> >
> > Le 20/10/2012 21:35, Rico Schüller a écrit :
> > > I think we also need a test to prove that "return MapWindowPoints(
> > > hwnd, 0, lppnt, 1 ) != 0;" is wrong. I'd probably split the changes to
> > > ScreenToClient/ClientToScreen and MapWindowPoints, because they are
> > > not necessarily bound together.
> > >
> > > What about something like:
> > > wnd2 = CreateWindow("static", "wnd2", WS_POPUP, 0, 0, 150, 150, NULL,
> > > NULL, NULL, NULL);
> > > ok(wnd2 != NULL, "CreateWindow failed\n");
> > > SetLastError(0xdeadbeef);
> > > p.x = p.y = 0;
> > > ret = ScreenToClient(wnd2, &p);
> > > err = GetLastError();
> > > ok(ret, "ScreenToClient() failed\n");
> > > ok(p.x == 0 && p.y == 0, "Failed got(%d, %d), expected (%d, %d)\n",
> > > p.x, p.y, 0, 0);
> > > ok(err == 0xdeadbeef, "GetLastError failed, got %x, expected %x\n",
> > > err, 0xdeadbeef);
> > >
> > > Also msdn says: "Do not use ScreenToClient when in a mirroring
> > > situation, that is, when changing from left-to-right layout to
> > > right-to-left layout. Instead, use MapWindowPoints. For more
> > > information, see "Window Layout and Mirroring" in Window Features." We
> > > may also check that case. So they may not share the exact same steps.