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 694046 - Keyboard resizing is broken for gtk+ windows
Keyboard resizing is broken for gtk+ windows
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-02-17 21:12 UTC by Giovanni Campagna
Modified: 2013-03-14 12:02 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
keybindings: Make sure to set the window as unfrozen during keyboard resize (1.89 KB, patch)
2013-03-13 03:49 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Fix freezing of windows with keyboard resizing (6.23 KB, patch)
2013-03-13 18:36 UTC, Owen Taylor
committed Details | Review
Make handling of windows that don't respond to _NET_WM_SYNC_REQUEST reliable (8.29 KB, patch)
2013-03-13 19:36 UTC, Owen Taylor
committed Details | Review

Description Giovanni Campagna 2013-02-17 21:12:24 UTC
- Focus a Gtk3 window
- Press Alt-F8, down, then up any number of times
- Don't move the mouse!
- Press Enter

In theory, this is a keyboard resizing operation - and in fact it does that on any window that doesn't support the extended XSync protocol.
For Gtk3 windows, though, the window actor appears to be stuck. The window is correctly resized, the outer_rect of the window changes (as you can see looking at the animation in the overview), but the window is never painted again.
Trying a new grab op, mouse or keyboard, has no effect, and you need to close the window.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-03-13 03:49:32 UTC
Created attachment 238757 [details] [review]
keybindings: Make sure to set the window as unfrozen during keyboard resize

This seems to be a simple oversight done when window freezing was first
introduced. Without this, the window actor will be frozen without getting
updates.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-03-13 03:50:29 UTC
This technically isn't a new bug since frame sync, but
meta_compositor_set_updates() was a FIXME before.
Comment 3 Owen Taylor 2013-03-13 14:52:23 UTC
Sorry - I've been sitting on a partial diagnosis, but hadn't written anything down because the fix was messy. I'm pretty sure the problem here comes from the commit:

 0503f6bb9a9298d10543371d4044c27792d137d8

 Consistently use meta_grab_op_is_resizing() for _NET_WM_SYNC_REQUEST
    
 In different places we checked the grab op differently when determing
 whether we are using _NET_WM_SYNC_REQUEST. This was somewhat covered
 up previously by the fact that we only had a sync alarm when using
 _NET_WM_SYNC_REQUEST, but that is no longer the case, so consistently
 use meta_grab_op_is_resizing() everywhere.
    
 https://bugzilla.gnome.org/show_bug.cgi?id=685463

Which started turning on freezing for keyboard resizes - previously we didn't do that.

But the problem is that some of the things that are there for mouse resizing aren't there for keyboard resizing - for mouse resizing we're continually keeping track of the position of the mouse, and when we get a sync back from the user, we use the last tracked position for a new resize. We don't do any sort of queuing like this for keyboard resizing.

So, what happens is that you get the first resize, then it hits the:

  if (dx == 0 && dy == 0)
    return;

in update_resize() and returns. I think this is actually *also* a problem for mouse resizes - we don't unfreeze if the user doesn't move the mouse again, so we lag behind unnecessarily, but usually the user is moving the mouse continually so it's hard to notice.

Then there's a second problem that if there's a pending resize at the end of the grab op, then we'll never unfreeze. For mouse resizes we unfreeze at mouse release (which isn't quite right either - we can get one frame of unsynchronized display from that - theoretically we should still be waiting for the configure request update.)

Your patch sort of brute forces things by unfreezing the window again right after we freeze it for a keyboard resize - this is going to break the frame sync working right for keyboard resize, and if we're going to do that, it's better just to revert out the patch (or do something to that effect a bit more consistently) and not freeze in the first place for keyboard resizes.

Three possible approaches to a fix:

 A) Just revert out using frame sync for anything but mouse resizes.

 B) Make keyboard resizes work exactly like mouse resizes - hold off the next *resize* until we get confirmation from the client, etc.

 B') Ignore the fact that keyboard resizes aren't like mouse resizes, and and add a unfreeze in the no-change shortcut, and an unfreeze at the end of keyboard resizing and hope for the best.

 C) Do A) and, bypass the resize-throttling for keyboard resizes (and maximize, snap, etc.) but still use _NET_WM_SYNC_REQUEST to get visual synchronization. Have some sort of "freeze until the sync value reaches N" system in MetaWindow.

The WM spec doesn't *require* throttling resizes when using _NET_WM_SYNC_REQUEST - you can send multiple _NET_WM_SYNC_REQUEST/Configure requests and expect the client to handle that properly and update the counter at the end. (Possibility of client bugs of course.)

I think I like C) best, but I don't know yet how invasive it's going to be, and how much risk it will pose.
Comment 4 Jasper St. Pierre (not reading bugmail) 2013-03-13 17:43:25 UTC
(In reply to comment #3)
> Sorry - I've been sitting on a partial diagnosis, but hadn't written anything
> down because the fix was messy. I'm pretty sure the problem here comes from the
> commit:
> 
>  0503f6bb9a9298d10543371d4044c27792d137d8
> 
>  Consistently use meta_grab_op_is_resizing() for _NET_WM_SYNC_REQUEST

Nope. I bisected it and it goes all the way back to https://git.gnome.org/browse/mutter/commit/?id=c9343e3ee3df0159fb3000bd38c61d9ff984fd8e
Comment 5 Owen Taylor 2013-03-13 18:36:45 UTC
Created attachment 238811 [details] [review]
Fix freezing of windows with keyboard resizing

Here's a something that works fairly well. It's basically along the
lines of approach C), except that I didn't fool around at all with
what resizes use _NET_WM_SYNC_REQUEST - it's sort of tricky because
a keyboard resize is *also* a mouse resize - you can start a resize
with the keyboard then use the mouse.

Bugs and future improvements:

 * If you have a unresponsive window, and the resize ends before
   we mark it unresponsive, you end up in a stuck state for that
   window. This is a regression in this patch, and needs
   to be fixed, since windows that draw a different
   size then they are are very confusing to the user.

 * If you have an unresponsive window, and keyboard resize it,
   the detection code doesn't run since it's tied up with the
   mouse event throttling code, so it ends up in a stuck state.

The fix for both of these is likely to tie the unresponsive-window
timer to the window, rather than to the grab and to separate it
out from the resize-frequency throttling code that is used without
the sync request.

 * Frame sync works well for keyboard resizing, along as you
   are pressing individual keys. Once you hold down a key,
   because we don't wait, and don't coellesce, the frame gets
   out of sync - we don't redraw the window until it signals
   that it is done, but before that we might have resized the
   frame again. Not something I want to try and fix 3.8.

 * There's no attempt to apply _NET_WM_SYNC_REQUEST to maximize,
   unmaximize, tiling, and other one-off resizes.

====

During resizing we froze window updates when configuring the
window, and unfroze the window updates when processing the
next resize. This wasn't absolutely reliable, because we might
not have a next resize. Instead tie window freezing more
directly to the current sync request value - a window is
frozen until it catches up with the last value we sent it
in _NET_WM_SYNC_REQUEST.

Testing with unresponsive clients showed that there was a bug
where window->disable_sync once set, would not actually disable
sync, but it *would* disable noticing that the client was
unresponsive for the next resize. Fix that by checking for
->disable_sync before sending _NET_WM_SYNC_REQUEST.
Comment 6 Owen Taylor 2013-03-13 19:36:37 UTC
Created attachment 238813 [details] [review]
Make handling of windows that don't respond to _NET_WM_SYNC_REQUEST reliable

Previously, we were handling failure to respond to _NET_WM_SYNC_REQUEST
in the code path for throttling motion events. But this meant that
if a window didn't respond to _NET_WM_SYNC_REQUEST and there were no
motion events - for a keyboard resize, or after the end of the grab
operation - it would end up in a stuck state.

Use a separate per-window timeout to reliably catch the failure to respond
to _NET_WM_SYNC_REQUEST.
Comment 7 Jasper St. Pierre (not reading bugmail) 2013-03-13 20:25:33 UTC
Review of attachment 238811 [details] [review]:

OK.
Comment 8 Jasper St. Pierre (not reading bugmail) 2013-03-13 20:26:33 UTC
Review of attachment 238813 [details] [review]:

::: src/core/window.c
@@ +4692,3 @@
 	      window->xwindow, False, 0, (XEvent*) &ev);
 
+  window->sync_request_timeout_id = g_timeout_add (1000,

Seems arbitrary, but OK.
Comment 9 Owen Taylor 2013-03-14 12:02:40 UTC
Pushed my patches. Added a comment for the 1-second timeout (which
was the same as before - the code was just reorganized.)

Attachment 238811 [details] pushed as 592374b - Fix freezing of windows with keyboard resizing
Attachment 238813 [details] pushed as 97a4cc8 - Make handling of windows that don't respond to _NET_WM_SYNC_REQUEST reliable