[Bug 56149] New: Celtic Kings demo: window decorations missing in virtual desktop (VD size = desktop size)
https://bugs.winehq.org/show_bug.cgi?id=56149
Bug ID: 56149 Summary: Celtic Kings demo: window decorations missing in virtual desktop (VD size = desktop size) Product: Wine Version: 9.0-rc3 Hardware: x86-64 URL: https://archive.org/download/celtic-kings/Celtic%20Kin gs.zip OS: Linux Status: NEW Keywords: download, regression Severity: normal Priority: P2 Component: winex11.drv Assignee: wine-bugs@winehq.org Reporter: gyebro69@gmail.com CC: gabrielopcode@gmail.com, zzhang@codeweavers.com Regression SHA1: 790133e95036597092443b30c0fe0aa6a40a9167 Distribution: ArchLinux
Created attachment 75829 --> https://bugs.winehq.org/attachment.cgi?id=75829 missing window decorations in virtual desktop
When I set the size of the virtual desktop so that it matches my desktop resolution (1440X900 in my case) then the opening movie as well as the main menu of the game appears without window decorations/window borders.
This occurs since commit 790133e95036597092443b30c0fe0aa6a40a9167.
This commit has the side effect that the patch from Zhiyi Zhang which is intended to fix bug #55810 simply doesn't work. Bug #55810#c14
To reproduce the problem in Celtic Kings demo: 1. set up a VD that matches your desktop resolution 2. run the demo
Celtic Kings.zip md5: 08657c24bf275fc7fd751b546a58daee
Tested with wine-9.0-rc3-22-gdef5e1a61d8 X.Org X Server 1.21.1.10 XFCE 4.18
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #1 from Béla Gyebrószki gyebro69@gmail.com --- Created attachment 75830 --> https://bugs.winehq.org/attachment.cgi?id=75830 the screen prior to commit 790133e95
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #2 from Gabriel Ivăncescu gabrielopcode@gmail.com --- The reason it "worked" before the blamed commit is because it was actually broken when it comes to a fullscreen virtual desktop, it didn't remove the decorations when it was actually fullscreen at all.
So when the game resized the VD to a lower resolution, it simply kept the decorations that were never removed in the first place.
Obviously, it should add the decorations back on a resize, not sure why it doesn't.
Also while testing this I found another issue: if you set the virtual desktop to less-than-fullscreen in winecfg such as 800x600, and then use "wine explorer /desktop=foo,1920x1080" (assuming that's your fullscreen resolution), the created desktop will cover the whole screen, but not the taskbar. I can confirm that having it set in winecfg to 800x600 is the issue, because if it's not set to a virtual desktop at all, or it's set to 1920x1080 as well, it works as expected.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #3 from Esme Povirk madewokherd@gmail.com --- If _NET_WM_STATE_FULLSCREEN is being toggled correctly then the window manager should take care of the window's decorations.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #4 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Ok, my patch is actually wrong, there's something weirder at play here. Sorry about that. Adding Rémi Bernon to CC since he may know something.
The original seems to be "correct" in the sense that root_window is actually the virtual desktop in question, and the event is sent to the actual "root" window (i.e. DefaultRootWindow(display)). With my patch it actually becomes pretty much a no-op (requesting the actual root window to be fullscreen).
So the interesting bit is this: the virtual desktop is *already* fullscreen by that point and covering the taskbar. And setting its _NET_WM_STATE_FULLSCREEN actually makes it behind the taskbar, which I cannot understand why. It does still hide the decorations though. Any insights?
I'd expect a _NET_WM_STATE_FULLSCREEN window being set again to _NET_WM_STATE_FULLSCREEN to be a no-op, but clearly for some reason it puts the taskbar in front of it.
After reverting my commit, do you think adding some sort of check if the window is already _NET_WM_STATE_FULLSCREEN and avoid setting it in such case be a good solution?
https://bugs.winehq.org/show_bug.cgi?id=56149
Gabriel Ivăncescu gabrielopcode@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rbernon@codeweavers.com
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #5 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Created attachment 75832 --> https://bugs.winehq.org/attachment.cgi?id=75832 Don't set it to fullscreen if it was already.
Something like this. Does this patch help?
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #6 from Béla Gyebrószki gyebro69@gmail.com --- (In reply to Gabriel Ivăncescu from comment #5)
Created attachment 75832 [details] Don't set it to fullscreen if it was already.
Something like this. Does this patch help?
The patch works for me, thank you.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #7 from Rémi Bernon rbernon@codeweavers.com --- Eh, I misread that change and thought it was changing the window to which we were sending the message... sorry.
Anyway, that new patch just reverts the blamed commit so it's probably why it solves this regression.
Then, regarding setting the fullscreen twice, I don't think this is something we should be bothering with. We don't when setting it for normal windows.
If the host window manager incorrectly behaves and shows the taskbar above fullscreen windows, it's a WM bug that should be fixed upstream, not in Wine.
Reading the windows properties is prone to race conditions and only makes the code more complicated than it needs.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #8 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Created attachment 75847 --> https://bugs.winehq.org/attachment.cgi?id=75847 Don't send it twice, but also sync it properly.
How about this instead? I got rid of the read_net_wm_states call now because I properly sync the net_wm_state of the desktop (as it should be, imo).
I did search on the internet and it seems it's fickle already, many reported problems. Do you not repro the issue? I think having a bit workarounds is OK if it's really a WM bug, because it makes fullscreen virtual desktops almost unusable (where they worked before).
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #9 from Rémi Bernon rbernon@codeweavers.com --- I don't see the issue on GNOME, but I also don't have a taskbar.
Kwin is known to have issues with leaving its taskbar over fullscreen windows, as well as incorrect focus state, under some convoluted circumstances. This is a kwin bug (reported as https://bugs.kde.org/show_bug.cgi?id=466160) and it should be fixed there, not in Wine (otherwise they will keep not fixing bugs).
Some other WM might have bugs as well, and they should fix them as well. I think having workarounds in Wine has always been frowned upon, first because it makes Wine (and esp winex11) code more complicated, and second because working around an environment bug simply hides it and makes it less likely to be fixed.
Wine triggers a lot of WM bugs, because of our very specific request pattern, still doesn't mean we should consider it's our fault. We don't do any kind of dupe request check when making or removing fullscreen from normal window, I don't see why we should here.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #10 from Gabriel Ivăncescu gabrielopcode@gmail.com --- I'm not sure if it happens with normal windows. For one thing, update_net_wm_states bails out instantly if it's the virtual desktop window, any idea why? So net_wm_states is never synced properly for virtual desktops, neither is fullscreen.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #11 from Rémi Bernon rbernon@codeweavers.com --- Probably because it is handled elsewhere and because the desktop window decorations / topmost / etc aren't really a thing.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #12 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Ok, this is not about double-setting it to fullscreen at all. According to an old bug I found when searching: https://bugs.openjdk.org/browse/JDK-6500686
This happens if _MOTIF_WM_HINTS is set without MWM_FUNC_RESIZE | MWM_FUNC_MOVE, and then setting it to fullscreen, and it affects several WMs already (not just Kwin, compiz too for instance). I confirmed it's the cause of the issue.
It worked before because X11DRV_SetDesktopWindow was setting it to fullscreen *before* the hint. Any fullscreen attempt after the hint removes resize makes it fail.
We already set MWM_FUNC_MOVE on desktop windows, what do you think about also setting MWM_FUNC_RESIZE only for fullscreen desktop windows?
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #13 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Also we **already** have a workaround for exactly this, see is_window_resizable in window.c:
/* Metacity needs the window to be resizable to make it fullscreen */
But that never gets called for virtual desktops since it uses a different code path above. So I guess that answers the question why this bug only happens on virtual desktop windows and not "normal windows".
I don't see why we shouldn't apply a similar workaround for virtual desktop windows.
https://bugs.winehq.org/show_bug.cgi?id=56149
Gabriel Ivăncescu gabrielopcode@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Fixed by SHA1| |d9b5bf9a719b1145f514230f170 | |ccc6b969a6679 Resolution|--- |FIXED
--- Comment #14 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Fixed by d9b5bf9a719b1145f514230f170ccc6b969a6679 (and the original taskbar regression it was supposed to fix by 75a774f90a86d1e39b2ea4fd5716ec74ac5aae64).
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #15 from Béla Gyebrószki gyebro69@gmail.com --- (In reply to Gabriel Ivăncescu from comment #14)
Fixed by d9b5bf9a719b1145f514230f170ccc6b969a6679 (and the original taskbar regression it was supposed to fix by 75a774f90a86d1e39b2ea4fd5716ec74ac5aae64).
OK, it's fixed as far as commit 75a774f90a86d1e39b2ea4fd5716ec74ac5aae64. But what does commit 4054795ff199c3f65612f351a88daf17f56a6db9 exactly do ? It is essentially reintroduces the original problem for me in a VD.
https://bugs.winehq.org/show_bug.cgi?id=56149
Gabriel Ivăncescu gabrielopcode@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |---
--- Comment #16 from Gabriel Ivăncescu gabrielopcode@gmail.com --- It basically sets _NET_WM_STATE_FULLSCREEN (which is what causes it to display fullscreen / possibly no decorations and on top of taskbar) after it sets the position of the window. The position is what causes it to set the hints (the previous commit, 75a774f90a86d1e39b2ea4fd5716ec74ac5aae64), and the hints have to be set correctly before _NET_WM_STATE_FULLSCREEN.
This kind of sequence isn't exclusive to virtual desktops. For normal fullscreen windows, update_net_wm_states is what sets it to _NET_WM_STATE_FULLSCREEN, and it's called after setting the hints in sync_window_position. (note that for VDs, it returns immediately, so it doesn't)
I have no idea why it fails for you. I can't reproduce it. But I reopened, I'll try fiddle a bit.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #17 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Created attachment 75858 --> https://bugs.winehq.org/attachment.cgi?id=75858 Update it also before SetWindowPos.
Just to confirm, does this patch help? I'm not sure if it's a good way, looks like a hack, but at least as a confirmation.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #18 from Béla Gyebrószki gyebro69@gmail.com --- (In reply to Gabriel Ivăncescu from comment #17)
Created attachment 75858 [details] Update it also before SetWindowPos.
Just to confirm, does this patch help? I'm not sure if it's a good way, looks like a hack, but at least as a confirmation.
Yes, that fixes all of my remaining woes regarding virtual desktop :) Any chance of it being merged in ?
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #19 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Created attachment 75859 --> https://bugs.winehq.org/attachment.cgi?id=75859 Here's a "proper" patch.
I doubt the previous one would be accepted since it's hackish (even if WMs have bugs as we can see), so here's a try for a "proper" one. This basically makes virtual desktops less special and moves it to use the NET_WM_STATE callsite for normal fullscreen windows.
Does it work for you? Fingers crossed.
Rémi, do you think something like this patch would be acceptable? I realize the whole NtUserGetPrimaryMonitorRect thing is wrong and only works for one monitor, but that was already the case before, and I don't want to change it during code freeze since it won't fix a regression.
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #20 from Béla Gyebrószki gyebro69@gmail.com --- (In reply to Gabriel Ivăncescu from comment #19)
Created attachment 75859 [details] Here's a "proper" patch.
I doubt the previous one would be accepted since it's hackish (even if WMs have bugs as we can see), so here's a try for a "proper" one. This basically makes virtual desktops less special and moves it to use the NET_WM_STATE callsite for normal fullscreen windows.
Does it work for you? Fingers crossed.
It works just as good as the previous "hack", thank you!
https://bugs.winehq.org/show_bug.cgi?id=56149
--- Comment #21 from Gabriel Ivăncescu gabrielopcode@gmail.com --- Great, thanks for testing!
https://bugs.winehq.org/show_bug.cgi?id=56149
Béla Gyebrószki gyebro69@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1|d9b5bf9a719b1145f514230f170 |44aa651dc540f2b2c6ef8b8c996 |ccc6b969a6679 |6889c4438f3a7 Resolution|--- |FIXED Status|REOPENED |RESOLVED
--- Comment #22 from Béla Gyebrószki gyebro69@gmail.com --- Commit 44aa651dc540f2b2c6ef8b8c9966889c4438f3a7 fixes the problem for me in all the cases where I encountered this bug. Thank you Gabriel for your persistent work on this bug.
https://bugs.winehq.org/show_bug.cgi?id=56149
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #23 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 9.0-rc5.
-
WineHQ Bugzilla