Re: [PATCH 1/4] ddraw: Restore resolution on EXCLUSIVE->NORMAL switch

[Henri Verbeet]

On 21 May 2013 13:18, Stefan Dösinger  wrote:
> The new code does not deliver the resolution change messages to the
> correct window, see the todo_wine markers in the next patch. Getting
> this right isn't simply a matter of passing the correct window to
> wined3d_device_restore_fullscreen_window because the delivery of the
> correct messages in the new window == old window case is mostly a lucky
> side effect of the window style hacks. Those hacks fail if we pass a
> different window, because the style of the new window is different from
> what wined3d expects, so it doesn't send any messages.

Well, wined3d_device_restore_fullscreen_window() isn't really supposed
to generate messages at all, the resize on mode changes happens in
device_parent_mode_changed(). The implication seems to be that
restoring the display mode is supposed to happen after the new
swapchain is created, but before "ddraw->cooperative_level" is
updated.
> /*****************************************************************************
> -
> - IDirectDraw7::RestoreDisplayMode
> -
> -
> -
> - Restores the display mode to what it was at creation time. Basically.
> -
> -
> -
> - Returns
> -
> - DD_OK on success
> -
> - DDERR_NOEXCLUSIVE mode if the device isn't in fullscreen mode
> -
> -
> - *****************************************************************************/

I don't think that really adds anything.
> -static HRESULT WINAPI ddraw7_SetCooperativeLevel(IDirectDraw7 *iface, HWND window, DWORD cooplevel)
> +static HRESULT WINAPI ddraw_set_cooperative_level(struct ddraw *ddraw, HWND window,
> -
>    DWORD cooplevel, BOOL restore_mode)

Shadowing the global "restore_mode" is a bit unfortunate.
> - TRACE("iface %p, window %p, flags %#x.\n", iface, window, cooplevel);
> - TRACE("ddraw %p, window %p, flags %#x, restore_mode %s.\n", ddraw, window, cooplevel,
> -
>        restore_mode ? "true" : "false");

I think %#x is generally clear enough for BOOLs.
>  if(cooplevel & DDSCL_ALLOWREBOOT)
> -
>    WARN("(%p) Unhandled flag DDSCL_ALLOWREBOOT, harmless\n", This);
> -
>    WARN("(%p) Unhandled flag DDSCL_ALLOWREBOOT, harmless\n", ddraw);
> if(cooplevel & DDSCL_ALLOWMODEX)
> -
>    WARN("(%p) Unhandled flag DDSCL_ALLOWMODEX, harmless\n", This);
> -
>    WARN("(%p) Unhandled flag DDSCL_ALLOWMODEX, harmless\n", ddraw);
> if(cooplevel & DDSCL_FPUSETUP)
> -
>    WARN("(%p) Unhandled flag DDSCL_FPUSETUP, harmless\n", This);
> -
>    WARN("(%p) Unhandled flag DDSCL_FPUSETUP, harmless\n", ddraw);

The "(%p)" there is pretty pointless, I usually remove them if I'm
editing the line anyway.
> @@ -86,6 +86,7 @@ struct ddraw
>  /* DirectDraw things, which are not handled by WineD3D */
>  DWORD                   cooperative_level;
> - BOOL                    setcooplevel_ddraw1;

We should probably consolidate the various BOOLs in struct ddraw into
a flags field.

[PATCH 1/4] ddraw: Restore resolution on EXCLUSIVE->NORMAL switch

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2013-05-21 16:04, schrieb Henri Verbeet:
> Well, wined3d_device_restore_fullscreen_window() isn't really
> supposed to generate messages at all, the resize on mode changes
> happens in device_parent_mode_changed().

Things make a lot more sense considering device_parent_mode_changed
and its relation to the focus window message hook.

The attached patch passes all the tests without todo_wine. However,
I'm not sure if there are any unintended side effects when I change
the order of the swapchain re-creation and focus window
release/acquire. I'll probably put this into a different patch for
bisecting reasons.
> > /*****************************************************************************

+ * IDirectDraw7::RestoreDisplayMode
> > -
> > -
> > -
> > - Restores the display mode to what it was at creation
> >
> > time. Basically. + * + * Returns + *  DD_OK on success + *
> > DDERR_NOEXCLUSIVE mode if the device isn't in fullscreen mode +
> > -
> > -
> >
> > *****************************************************************************/
> >

I don't think that really adds anything.
It's merely moved from its original position. I'd like to keep it for
consistency as pretty much all ddraw methods have a small
documentation comment.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRnI2/AAoJEN0/YqbEcdMwot8QAIlBRbgu6F8ZsWezYm7l6oHD
U9z6MnfHEl9M5jyUmxnIzJPGVvwG2Dd7Kot6+SNwUJs8lK4zM6aCyGmts2q6KK83
Ep3nCsQoGLtWnw1ZfsksW0692jj4wxaOkQMa9FtKR3zIcy82JtB+kNov1yepnOyy
7FMtwgxll4YG6Sfz2fBi6mEQaqTThkvWwEWiezh84SH/M8hWn99MfTwhi6vVT9Cp
A1PgGXm88KJnB1XDe+E2M6Dv8lSyr/59cfyEtCZkv4llfIDscPMINpZKCRsFKORf
b+2liqbXZvAIrsrUXq/nP3BHQdsuf2x0apOGhYwWp4QfqsdT8REY3Zww0V/ggadg
iY17KFejc5e6jBvleF4N5H2JkJUeihhKFqyBJmzHcE/XABEUJa18LIyQdZbbvNNB
YnN29cyps7J0/DMV0BhEmWCnWPPrj4EAHwaD1xbPFGPe7Zp7I12+9dORTisXIvOp
CPho/G/iRHPxqJ9rsgMKzEP0GBewpy6nCwRDzpRw/3M3u9mfmkwR8ekpY6yGZDSz
DIEIcbZ5MkG6fgtfipPAvLmPiBpMs0gB+Ifhxx77T2mIj8STjuq3JQYHhnU0jMwt
CB4pCuiwLrKNTDIZOMqzEJQTDhynmL2Ajmpuh1f2w577uR9N2iHheVVff19a2n5H
8VQ+S33c/3IA7et5U8NO
=WQuN
-----END PGP SIGNATURE-----

[PATCH 1/4] ddraw: Restore resolution on EXCLUSIVE->NORMAL switch

[Henri Verbeet]

On 22 May 2013 11:20, Stefan Dösinger  wrote:
> The attached patch passes all the tests without todo_wine. However,
> I'm not sure if there are any unintended side effects when I change
> the order of the swapchain re-creation and focus window
> release/acquire. I'll probably put this into a different patch for
> bisecting reasons.

Yeah, makes sense. Applications tend to be pretty sensitive to this
kind of stuff.
> It's merely moved from its original position. I'd like to keep it for
> consistency as pretty much all ddraw methods have a small
> documentation comment.

It's not a big deal, but I generally remove those if I touch the
function for some other reason, unless there's information in there
that's not immediately obvious from just looking at the function.