Re: prevents WM_DISPLAYCHANGE message being sent when a call to ChangeDisplaySettingsEx fails due to the requested change to the display not occurring.
On 24/03/2008, chris morgan cmorgan@mail.leather-wallet.co.uk wrote:
test code run on windows nt4 sp6, messages checked using ms spk++:
DEVMODE dev_mode; memset(&dev_mode,0,sizeof(DEVMODE)); dev_mode.dmSize = sizeof(DEVMODE); // Get the current display settings if (!EnumDisplaySettings(NULL,ENUM_CURRENT_SETTINGS,&dev_mode)) { printf("Could not EnumDisplaySettings, giving up.\n"); return 0; } // Call ChangeDisplaySettings with the current settings, // No WM_DISPLAYCHANGE message sent ChangeDisplaySettings(&dev_mode, 0); // Change something (that works) dev_mode.dmBitsPerPel = 32; // WM_DISPLAYCHANGE message sent ChangeDisplaySettings(&dev_mode, 0); // Change something to an impossible value dev_mode.dmBitsPerPel = 999; // No WM_DISPLAYCHANGE message sent ChangeDisplaySettings(&dev_mode, 0);
This should be incorporated into a conformance/regression test. That way, this bug will not reappear and can also be verified as the correct behaviour on different versions of Windows.
- if (width == screen_width && height == screen_height)
- {
- return;
- }
w.r.t. the implementation: * ChangeDisplaySettings returns a long (http://msdn2.microsoft.com/en-us/library/ms533260(VS.85).aspx), so the correct value must be returned on error; * the fix does not match the test cases (screen size vs colour depth).
In addition, your tests should: * check that changing the screen resolution causes a WM_DISPLAYCHANGE message to be sent; * if there are no other tests, ChangeDisplaySettings should be checked to see what happens with invalid arguments (for completeness); * the return value is not checked for expected values (see http://msdn2.microsoft.com/en-us/library/ms533260(VS.85).aspx); * GetLastError() is not checked to see what it is set to (see other tests for examples); * restore the display settings back to what they were before the tests.
Thanks for improving Wine, - Reece
thanks for the feedback, i'm new to this so it's very much appreciated - helped a lot.
I've updated the patch which hopefully covers the issues raised, but wanted to ask about the returning of an error:
w.r.t. the implementation:
- ChangeDisplaySettings returns a long
(http://msdn2.microsoft.com/en-us/library/ms533260(VS.85).aspx), so the correct value must be returned on error;
as failure to change the colour depth currently isn't treated as an error which seems sensible as it enables many older apps to run presumably without issue so thought it better not to change that.
from X11DRV_desktop_SetCurrentMode
/* Ignore the depth mismatch * * Some (older) applications require a specific bit depth, this will allow them * to run. X11drv performs a color depth conversion if needed. */
- the fix does not match the test cases (screen size vs colour depth).
oops, bad oversight. to fix this i've created a variable to store the state of screens colour depth as it would be perceived by a client. this is used to detect changes to screen depth and prevent unexpected WM_DISPLAYCHANGE from being sent.
i.e. a program calls ChangeDisplaySettings changing the colour depth and receives back a WM_DISPLAYCHANGE message. As far as the client is concerned the screen is at the new colour depth as it's call to ChangeDisplaySettings succeeded.
If the program called ChangeDisplaySettings with setting the same colour depth as it's previous call then, if no other setting had changed, it would not expect to receive a WM_DISPLAYCHANGE message as it's view of the screen's colour depth had not changed.
In addition, your tests should:
- check that changing the screen resolution causes a
WM_DISPLAYCHANGE message to be sent;
- if there are no other tests, ChangeDisplaySettings should be
checked to see what happens with invalid arguments (for completeness);
- the return value is not checked for expected values (see
http://msdn2.microsoft.com/en-us/library/ms533260(VS.85).aspx);
- GetLastError() is not checked to see what it is set to (see other
tests for examples);
- restore the display settings back to what they were before the tests.
found the existing test now which seem to cover much of this but will try and extend them further. the new patch seems to pass ok
new patch:
--- dlls/winex11.drv/desktop.c | 12 +++++++++++- dlls/winex11.drv/event.c | 1 + dlls/winex11.drv/winpos.c | 4 ++-- dlls/winex11.drv/x11drv.h | 1 + dlls/winex11.drv/x11drv_main.c | 3 ++- dlls/winex11.drv/xrandr.c | 12 +++++++++++- 6 files changed, 28 insertions(+), 5 deletions(-) --------------1.5.2.5 Content-Type: text/x-patch; name="c65ac7a7c58d8d36c05bf789daf2c5f3cbb64959.diff" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="c65ac7a7c58d8d36c05bf789daf2c5f3cbb64959.diff"
diff --git a/dlls/winex11.drv/desktop.c b/dlls/winex11.drv/desktop.c index 1699318..04ac482 100644 --- a/dlls/winex11.drv/desktop.c +++ b/dlls/winex11.drv/desktop.c @@ -94,8 +94,18 @@ static LONG X11DRV_desktop_SetCurrentMode(int mode) * to run. X11drv performs a color depth conversion if needed. */ } + TRACE("Resizing Wine desktop window to %dx%d\n", dd_modes[mode].dwWidth, dd_modes[mode].dwHeight); - X11DRV_resize_desktop(dd_modes[mode].dwWidth, dd_modes[mode].dwHeight); + + if (client_bpp != dd_modes[mode].dwBPP || + screen_width != dd_modes[mode].dwWidth || + screen_height != dd_modes[mode].dwHeight) + { + // Only call if the desktop has acutally changed + client_bpp = dd_modes[mode].dwBPP; + X11DRV_resize_desktop(dd_modes[mode].dwWidth, dd_modes[mode].dwHeight); + } + return DISP_CHANGE_SUCCESSFUL; }
diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index cd6eaf6..6795d90 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -1106,6 +1106,7 @@ LRESULT X11DRV_WindowMessage( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) case WM_X11DRV_SET_WIN_FORMAT: return X11DRV_set_win_format( hwnd, (XID)wp ); case WM_X11DRV_RESIZE_DESKTOP: + client_bpp = wp; X11DRV_resize_desktop( LOWORD(lp), HIWORD(lp) ); return 0; default: diff --git a/dlls/winex11.drv/winpos.c b/dlls/winex11.drv/winpos.c index ad62236..6c68858 100644 --- a/dlls/winex11.drv/winpos.c +++ b/dlls/winex11.drv/winpos.c @@ -564,7 +564,7 @@ void X11DRV_resize_desktop( unsigned int width, unsigned int height )
if (GetWindowThreadProcessId( hwnd, NULL ) != GetCurrentThreadId()) { - SendMessageW( hwnd, WM_X11DRV_RESIZE_DESKTOP, 0, MAKELPARAM( width, height ) ); + SendMessageW( hwnd, WM_X11DRV_RESIZE_DESKTOP, client_bpp, MAKELPARAM( width, height ) ); } else { @@ -573,7 +573,7 @@ void X11DRV_resize_desktop( unsigned int width, unsigned int height ) virtual_screen_rect.right - virtual_screen_rect.left, virtual_screen_rect.bottom - virtual_screen_rect.top, SWP_NOZORDER | SWP_NOACTIVATE | SWP_DEFERERASE ); - SendMessageTimeoutW( HWND_BROADCAST, WM_DISPLAYCHANGE, screen_bpp, + SendMessageTimeoutW( HWND_BROADCAST, WM_DISPLAYCHANGE, client_bpp, MAKELPARAM( width, height ), SMTO_ABORTIFHUNG, 2000, NULL ); }
diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 52670e8..8c28f81 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -525,6 +525,7 @@ extern Window root_window; extern unsigned int screen_width; extern unsigned int screen_height; extern unsigned int screen_bpp; +extern unsigned int client_bpp; extern unsigned int screen_depth; extern RECT virtual_screen_rect; extern unsigned int text_caps; diff --git a/dlls/winex11.drv/x11drv_main.c b/dlls/winex11.drv/x11drv_main.c index d445e64..58dd680 100644 --- a/dlls/winex11.drv/x11drv_main.c +++ b/dlls/winex11.drv/x11drv_main.c @@ -71,6 +71,7 @@ Visual *visual; unsigned int screen_width; unsigned int screen_height; unsigned int screen_bpp; +unsigned int client_bpp; unsigned int screen_depth; RECT virtual_screen_rect; Window root_window; @@ -517,7 +518,7 @@ static BOOL process_attach(void) } } if (!screen_depth) screen_depth = DefaultDepthOfScreen( screen ); - screen_bpp = depth_to_bpp( screen_depth ); + client_bpp = screen_bpp = depth_to_bpp( screen_depth );
XInternAtoms( display, (char **)atom_names, NB_XATOMS - FIRST_XATOM, False, X11DRV_Atoms );
diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index fd930a2..e977aa1 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -179,6 +179,7 @@ static LONG X11DRV_XRandR_SetCurrentMode(int mode) unsigned int i; int j; DWORD dwBpp = screen_bpp; + DWORD modeBPP = dd_modes[mode].dwBPP;
wine_tsx11_lock(); root = RootWindow (gdi_display, DefaultScreen(gdi_display)); @@ -188,6 +189,7 @@ static LONG X11DRV_XRandR_SetCurrentMode(int mode) { FIXME("Cannot change screen BPP from %d to %d\n", dwBpp, dd_modes[mode].dwBPP); } + mode = mode%real_xrandr_modes_count;
TRACE("Changing Resolution to %dx%d @%d Hz\n", @@ -229,7 +231,15 @@ static LONG X11DRV_XRandR_SetCurrentMode(int mode) wine_tsx11_unlock(); if (stat == RRSetConfigSuccess) { - X11DRV_resize_desktop( dd_modes[mode].dwWidth, dd_modes[mode].dwHeight ); + if (client_bpp != modeBPP || + screen_width != dd_modes[mode].dwWidth || + screen_height != dd_modes[mode].dwHeight) + { + // Only call if the desktop has acutally changed + client_bpp = modeBPP; + X11DRV_resize_desktop( dd_modes[mode].dwWidth, dd_modes[mode].dwHeight ); + } + return DISP_CHANGE_SUCCESSFUL; }
-
chris morgan -
Reece Dunn