On 25.08.2015 17:01, Rodrigo Rivas wrote:
[Sorry for the double post, I forgot to reply to the list]
On Tue, Aug 25, 2015 at 3:35 PM, Nikolay Sivov bunglehead@gmail.com wrote:
On 25.08.2015 16:08, Rodrigo Rivas wrote:
I sent this on July 15th, but it went through the cracks. Maybe the mail text was not detailed enough, so I will try and explain it better this time.
The error behaviour of functions BeginDeferWindowPos(), DeferWindowPos() and EndDeferWindowPos() in Wine is different than in Windows.
In Windows, when DeferWindowPos() is used with an invalid HWND, this function fails, but the eventual EndDeferWindowPos() will succeed and move all the other windows in the set. In Wine, however, DeferWindowPos() does not check the HWND, so it never fails. But then, it fails in EndDeferWindowPos(), moving only the windows passed before the invalid HWND and leaving the windows after the invalid HWND unmoved.
The Windows behaviour is consistent in every version I tested (XP and 7).
The attached patch makes the Wine error behaviour the same as the Windows one. It also considers the case where the HWND is valid when calling DeferWindowPos() but is destroyed before calling EndDeferWindowPos(): The error is simply ignored.
It fixes bug #23187.
dlls/user32/tests/Makefile.in | 1 + dlls/user32/tests/defer.c | 168 ++++++++++++++++++++++++++++++++++++++++++ dlls/user32/winpos.c | 14 ++-- 3 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 dlls/user32/tests/defer.c
I don't think you need a new file for that, how about moving this to win.c?
It may make sense in win.c. I assumed that user32/tests/win.c is related to user32/win.c. And since the affected functions are in user32/winpos.c... maybe I should have named it user32/tests/winpos.c? But I don't have a strong preference If you really want it, would you like me to do the merge and resend the patch? Or is it not necessary?
We try to avoid adding new files for every new test, to keep number of files under reasonable limit. So yes, I think it's better to have that moved to win.c, and no, renaming it is not necessary.
Regarding fix itself, it's probably better to split it to two - fix DeferWindowPos() in first + related tests, and End* one in the second.
In this I disagree. Changing DeferWindowPos() without changing EndDeferWindowPos() would make little sense, because the error checking is moving from EDWP to DWP. The special case of the window being destroyed between DWP and EDWP is just a consequence of EDWP not having any error checking left. You could have one patch that adds the check to DWP and another that removes it from EDWP, but what's the point? The intermediate step will not even pass the test case, it will fail at TestDestroyDeferWindowPos().
So to fair you would have to split in: a) fix DWP + tess1+test2; b) fix EDWP + test3.
Sure, that's what I meant, to have one fix + its tests in first patch, followed by second fix + its own tests. But let's see if someone else has anything to add to this, it might be okay to have it they way you did.