Re: wined3d: Properly handle WS_VISIBLE and WS_EX_TOPMOST style flags on fullscreen windows.
On 29 March 2013 03:31, Sam Edwards cfsworks@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.
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
On 30 March 2013 23:50, Sam Edwards cfsworks@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:
- 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.
- 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.)
- Include each test in the first patch that will allow the test to pass. (That is, add them incrementally.)
- 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.
-
Henri Verbeet -
Sam Edwards