Re: wined3d: Properly handle WS_VISIBLE and WS_EX_TOPMOST style flags on fullscreen windows.

[Henri Verbeet]

On 29 March 2013 03:31, Sam Edwards <cfsworks(a)gmail.com> wrote:
> Note that the tests are NOT (primarily) intended to make sure that
> WS_VISIBLE and WS_EX_TOPMOST are still set. The main purpose of this patch
> is to fix a slightly more serious bug:
>
> - Create a window without WS_VISIBLE.
> - Create a fullscreen Direct3D device.
> - wined3d applies its own flags (including WS_VISIBLE) to the window, to
> make it fullscreen.
> - Reset the device back to windowed mode.
> - Because wined3d_device_setup_fullscreen_window didn't see the window had
> WS_VISIBLE, wined3d_device_restore_fullscreen_window won't restore the
> style.
>
> This patch corrects that behavior, and the window restores back to its
> original style as on Windows.

I think the idea is basically ok, but I do have some comments:

I think test_window_style() is a more appropriate place for the tests,
and you should add them to the variants in ddraw as well. Ddraw
applications in particular are very fragile in this regard. Note that
the test_window_style() tests are fairly similar to what your tests
do, except that they don't test restoring the window as such. That
test also shows that we're actually not supposed to touch the styles
at all, but that's hard to make work without calling into winex11.
> -    SetWindowPos(window, HWND_TOP, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE);
> +    SetWindowPos(window, HWND_TOPMOST, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE);
> This is really a separate change.
> +    device->style ^= (device->style^style) & WS_VISIBLE;
> Spaces around the (second) ^, please.

wined3d: Properly handle WS_VISIBLE and WS_EX_TOPMOST style flags on fullscreen windows.

[Sam Edwards]

On 03/29/2013 06:16 AM, Henri Verbeet wrote:
> I think the idea is basically ok, but I do have some comments:
>
> I think test_window_style() is a more appropriate place for the tests,
> and you should add them to the variants in ddraw as well. Ddraw
> applications in particular are very fragile in this regard. Note that
> the test_window_style() tests are fairly similar to what your tests
> do, except that they don't test restoring the window as such. That
> test also shows that we're actually not supposed to touch the styles
> at all, but that's hard to make work without calling into winex11.

Ack, you're right! I saw the test_window_style() but second-guessed 
myself and put it into test_reset() instead... That was foolish of me. 
Thanks.

>> -    SetWindowPos(window, HWND_TOP, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE);
>> +    SetWindowPos(window, HWND_TOPMOST, 0, 0, w, h, SWP_FRAMECHANGED | SWP_SHOWWINDOW | SWP_NOACTIVATE);
> This is really a separate change.
I'm a bit confused, then: Without this change, the exstyle test breaks, 
and the guidelines request that every patch be atomic. They also request 
that all tests be included in the same patch.What is the proper 
procedure for this when it comes to multipatch patchsets?
Off the top of my head, I would guess it's one of these:
1. The "all tests in the same patch" rule is only appropriate for 
single-patch submissions, so in multipatch patchsets, it's acceptable to 
include a final patch with nothing but tests.
2. Only introduce the tests in the last patch of the patchset. (That is, 
add them all at once, but only when all of the necessary changes have 
been made to allow the tests to work first.)
3. Include each test in the first patch that will allow the test to 
pass. (That is, add them incrementally.)
4. Add the tests as todo_wine in the first patch, then remove the 
todo_wine in later patches as each test becomes functional. (That is, 
add them all at once as todo_wine, and mark them as non-todo incrementally.)

>> +    device->style ^= (device->style^style) & WS_VISIBLE;
> Spaces around the (second) ^, please.
Done. :)

Thank you,
Sam

wined3d: Properly handle WS_VISIBLE and WS_EX_TOPMOST style flags on fullscreen windows.

[Henri Verbeet]

On 30 March 2013 23:50, Sam Edwards <cfsworks(a)gmail.com> wrote:
> I'm a bit confused, then: Without this change, the exstyle test breaks, and the guidelines request that every patch be atomic. They also request that all tests be included in the same patch.

I'm not sure there really is such a rule.
> Off the top of my head, I would guess it's one of these:
> 1. The "all tests in the same patch" rule is only appropriate for single-patch submissions, so in multipatch patchsets, it's acceptable to include a final patch with nothing but tests.
> 2. Only introduce the tests in the last patch of the patchset. (That is, add them all at once, but only when all of the necessary changes have been made to allow the tests to work first.)
> 3. Include each test in the first patch that will allow the test to pass. (That is, add them incrementally.)
> 4. Add the tests as todo_wine in the first patch, then remove the todo_wine in later patches as each test becomes functional. (That is, add them all at once as todo_wine, and mark them as non-todo incrementally.)

I think either or those is fine in principle, it depends a bit on the
kind of test what's most practical. I guess that for this one you'll
probably want 3.