After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 758186 - Wine fullscreen-windows crash on minimize - patch included
Wine fullscreen-windows crash on minimize - patch included
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
3.18.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-11-16 16:47 UTC by RobertMader
Modified: 2015-12-14 19:35 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
diff-file for mutter/src/x11/window-x11.c (239 bytes, text/plain)
2015-11-16 16:47 UTC, RobertMader
  Details
patch file as discussed below (516 bytes, patch)
2015-11-17 12:43 UTC, RobertMader
none Details | Review
window: Allow minimizing windows which don't advertise support for it (2.26 KB, patch)
2015-12-05 18:47 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description RobertMader 2015-11-16 16:47:51 UTC
Created attachment 315691 [details]
diff-file for mutter/src/x11/window-x11.c

Wine does minimize windows on alt-tab now as Windows does.
Mutter and other Metacity based WMs suffer from a bug that let them crash as they do not support XIconifyWindow without MWM_FUNC_MINIMIZE.

This is a patch from MATE that makes it work again which is of great benefit for wine user/gamer.
Tested on Fedora 23, wine-staging 1.7.53, Starcraft 2

Related:

https://github.com/mate-desktop/marco/issues/166
https://github.com/mate-desktop/marco/pull/167
https://bugs.winehq.org/show_bug.cgi?id=37765

alternative patch:
https://github.com/mate-desktop/marco/pull/169

Hoping to attract interest and best regards,
Robert
Comment 1 RobertMader 2015-11-16 17:01:50 UTC
Muffin also has this code version.
Comment 2 Jasper St. Pierre (not reading bugmail) 2015-11-16 17:09:41 UTC
Er, so we just flat ignore the minimize field from the window? I'm not a big fan of that. Why do some of these windows say they don't support minimize?
Comment 3 RobertMader 2015-11-16 17:21:22 UTC
Stefan Dösinger describes it here:
https://github.com/mate-desktop/marco/issues/166
Comment 4 Owen Taylor 2015-11-16 20:23:44 UTC
I think the idea is that a window shouldn't be able to minimize itself if there's no way for the user to unminimize it, or it just doesn't make sense. The origin of the check is:

    2001-10-07  Havoc Pennington  <hp@pobox.com>
    
        * src/window.c (meta_window_client_message): don't allow
        shade/maximize/minimize for windows that don't support those
        operations. (minimizing the panel = bad)

But this doesn't actually correspond exactly to has_minimize_func, so changing it a bit to look instead at whether the window is !skip_taskbar would seem fine to me - skip_taskbar should tell us exactly whether the user has a way to get back to the window if minimized.

It could potentially break an assumption that tasks lists have that minimized windows only are those with NET_WM_ACTION_MINIMIZE, but

 - I suppose MATE tested this with their panel, and we don't care about external process task lists for Mutter.
 - The official task list shell extension in GNOME actually allows minimizing windows that are !has_minimize_func - these windows that have minimize grayed out in their window menu, but enabled in the task list popup menu.

In summary, I think changing the check to !skip_taskbar is fine, removing it is probably a bad idea.
Comment 5 Jasper St. Pierre (not reading bugmail) 2015-11-16 20:28:42 UTC
(In reply to Owen Taylor from comment #4)
>  - I suppose MATE tested this with their panel, and we don't care about
> external process task lists for Mutter.

This isn't necessarily true -- the dock in the Elementary DE is external. I think this is still fine though.
Comment 6 RobertMader 2015-11-17 11:32:44 UTC
Works fine for me with !window->skip_taskbar. 
Tested with starcraft 2, diablo 3 and warcraft 3.
Comment 7 RobertMader 2015-11-17 12:43:54 UTC
Created attachment 315746 [details] [review]
patch file as discussed below
Comment 8 RobertMader 2015-11-25 13:26:27 UTC
Any chance that this gets included during gnome 3.18 livecycle?

It has been adopted in other window managers and seems to work fine.
It would greatly improve out-of-the-box wine-gaming experience for many users with little risk, eliminating the need to install other DEs beside gnome.

Regards
Comment 9 RobertMader 2015-11-25 14:28:30 UTC
Update:
This also affects xwayland. So the patch solves this issue for wayland-sessions, too.
Comment 10 Jasper St. Pierre (not reading bugmail) 2015-12-05 18:47:04 UTC
Created attachment 316812 [details] [review]
window: Allow minimizing windows which don't advertise support for it

Wine removes the minimize func from its Motif hints on full-screen
windows, because, as the Win32 API literally says, the minimize button
is indeed not visible on full-screen windows.

Given that this code was added to prevent minimizing a panel by
accident, I don't necessarily think that it's relevant anymore.
Comment 11 Rui Matos 2015-12-14 14:29:44 UTC
Review of attachment 316812 [details] [review]:

::: src/x11/window-x11.c
@@ +2442,3 @@
       meta_verbose ("WM_CHANGE_STATE client message, state: %ld\n",
                     event->xclient.data.l[0]);
+      if (event->xclient.data.l[0] == IconicState)

Doesn't this mean that a skip_taskbar window is allowed to disappear here?
Comment 12 Jasper St. Pierre (not reading bugmail) 2015-12-14 16:21:39 UTC
(In reply to Rui Matos from comment #11)
> Doesn't this mean that a skip_taskbar window is allowed to disappear here?

Sure, but since it itself sent the _NET_WM_STATE message, I think that's OK. Anywhere else that might show it (window menu, panel) has disabled it because of the has_minimize_func.
Comment 13 Rui Matos 2015-12-14 16:43:24 UTC
Comment on attachment 316812 [details] [review]
window: Allow minimizing windows which don't advertise support for it

(In reply to Jasper St. Pierre from comment #12)
> (In reply to Rui Matos from comment #11)
> > Doesn't this mean that a skip_taskbar window is allowed to disappear here?
> 
> Sure, but since it itself sent the _NET_WM_STATE message, I think that's OK.
> Anywhere else that might show it (window menu, panel) has disabled it
> because of the has_minimize_func.

Ok, I don't have a strong opinion, just wanted to make sure that's really what you intended here. The window should still be accessible from the overview in any case.
Comment 14 Jasper St. Pierre (not reading bugmail) 2015-12-14 16:47:50 UTC
skip_taskbar windows aren't in the overview, though. Or did I get that wrong?
Comment 15 Rui Matos 2015-12-14 17:20:57 UTC
(In reply to Jasper St. Pierre from comment #14)
> skip_taskbar windows aren't in the overview, though. Or did I get that wrong?

Aha, you're right, they aren't. In that case, I don't like this as much. Why do you want to allow applications to do this? It's orthogonal to the bug report anyway.
Comment 16 Jasper St. Pierre (not reading bugmail) 2015-12-14 17:55:08 UTC
Read the original WINE report. The issue is that XIconifyWindow (which sends the WM_CHANGE_STATE message) doesn't work when windows don't have the minimize reported.

Since MWM_DECOR_MINIMIZE does not have any definition or specification, and no other window manager special cases WM_CHANGE_STATE for MWM_DECOR_MINIMIZE, I'm matching the behavior of other WMs.

So we're allowing windows to minimize themselves even if they set MWM_DECOR_MINIMIZE. That's the window-x11.c part of the patch.

The special case was added to prevent users from hiding skip-taskbar windows through (namely, gnome-panel). To prevent users from minimizing windows like that in the future, we automatically remove the minimize button from windows that are marked as skip-taskbar to make sure that users can't accidentally minimize it with no way to get it back.
Comment 17 Rui Matos 2015-12-14 18:39:54 UTC
Right, I understood all that. And I think window-x11.c should be changed so that !window->skip_taskbar is a requirement to minimize a window. Not a strong opinion though, so feel free to push as you prefer.
Comment 18 Jasper St. Pierre (not reading bugmail) 2015-12-14 19:35:31 UTC
Attachment 316812 [details] pushed as 8cc345f - window: Allow minimizing windows which don't advertise support for it


I obviously disagree, so pushing now.