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 126497 - can't revert/undo drags/resizes by hitting escape
can't revert/undo drags/resizes by hitting escape
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
trunk
Other Linux
: Normal enhancement
: 2.16.x
Assigned To: Metacity maintainers list
Metacity maintainers list
: 353882 (view as bug list)
Depends on:
Blocks: 155458
 
 
Reported: 2003-11-08 14:52 UTC by Sebastien Bacher
Modified: 2006-09-01 18:06 UTC
See Also:
GNOME target: ---
GNOME version: 2.15/2.16


Attachments
fix + one warning (4.46 KB, patch)
2006-07-06 04:04 UTC, Thomas Andersen
needs-work Details | Review

Description Sebastien Bacher 2003-11-08 14:52:49 UTC
Thie bug was originally reported in the debian BTS:
http://bugs.debian.org/208698

"Dragging windows from title bar or resizing them by drag is not 
cancelled by neither chord click
(pressing the another mouse button down) or by pressing ESC.

Expected behaviour: Drag would stop and window return to the original 
state when either ESC key is pressed or another mouse button pressed 
during drag."
Comment 1 Luis Villa 2004-01-02 18:21:10 UTC
This bug doesn't really make sense to me, Sebastien- all of these 
actions cancel on button-up, so for this report to be relevant/make 
sense, you'd have to hit 'esc' while also still holding down the 
left click button, which doesn't really make sense as a user action. 
Can Akira clarify, please?
Comment 2 Luis Villa 2004-01-02 23:13:12 UTC
Retitling to make slightly more clear, after conversation with Seb in
IRC. 
Comment 3 Rob Adams 2004-01-02 23:18:33 UTC
seems reasonable to me; I probably won't have time to get around to it
for a while, but I can't see any obstacle to a patch to this effect
being accepted.
Comment 4 Elijah Newren 2006-04-27 19:51:44 UTC
This is already handled for using either Alt+F7 or Alt+F8 followed by either the arrow keys or moving the mouse in order to move or resize the window; it just needs to be done for mouse-only move/resize.  For the mouse-only case though, we don't do a keyboard grab and thus won't get notification of the escape key being pressed.  Because of our special handling of grabbing the keyboard, and this case not having that special handling, this also results in some other weirdness[1].  So, basic steps to get this implemented:
  A) Do the keyboard grab stuff for these mouse-only move/resize operations in
     meta_display_begin_grab_op().
  B) Add a hook for handling for these type of grab_ops to
     meta_display_process_key_event().
  C) Make sure that other key presses during these operations don't cause
     weirdness[1].  This basically means stuff other than XK_Escape should be
     ignored.  See process_keyboard_move_grab() for hints (including the handling
     of XK_Escape needed for B).

[1] Try pressing special keys like Alt+F2 while moving the window around.  Neat, huh?

Just so that I don't forget: There is an additional problem, in that there is some code that dumbly mucks with display->grab_initial_window_pos in window.c, but I view that as a bug of the other code and I'll add a note to the other relevant bug.
Comment 5 Elijah Newren 2006-04-27 19:56:16 UTC
Oops, forgot to add the gnome-love keyword.  This should be relatively straightforward with the steps I provided to fix this in comment 4.
Comment 6 Thomas Andersen 2006-07-06 04:04:41 UTC
Created attachment 68437 [details] [review]
fix + one warning

Here is a patch that does as you have described above.
Dragging a window with the mouse now ignores all keystrokes exept for escape which take the window back to where it started.

With the patch this warning comes when grabbing the keyboard:
"Window manager warning: Got a request to focus 0x2a00480 (thomas@mos) with a timestamp of 0.  This shouldn't happen!"
It only comes when the keyboard is grabbed while the mouse is used to move the window. With Alt+f7 it does not. 

I have tracked it to this line:
display->grab_have_keyboard = meta_window_grab_all_keys (window);

This is the same line used in both keyboard and mouse case so I don't understand why this happens?
Comment 7 Thomas Thurman 2006-07-28 02:39:26 UTC
Firstly, the patch does what it says on the tin. So yay for that.

Secondly, I would remove the curly brackets on the second "if" statement in process_mouse_move_grab; there's no sense doing it one way once and another time the other way.

Thirdly, the reason you get the warning is that when using the keyboard, meta_display_begin_grab_op gets called ultimately from event_callback. When using the mouse, MDBGO gets called ultimately from meta_frames_button_press_event. The important difference is that although event_callback correctly sets display->current_time to the time of the event, MFBPE does not, and so display->current_time is zero and you end up with the warning about a zero timestamp.

You can fix this by adding

  gdk_display->current_time = event_get_time (gdk_display, event);

to meta_frames_button_press_event before the call to meta_core_begin_grab_op.
Comment 8 Thomas Thurman 2006-07-28 02:50:11 UTC
(And clear up after the call to meta_core_begin_grab_op by doing

  gdk_display->current_time = CurrentTime;

just as event_callback does. Sorry, should have mentioned that too.)
Comment 9 Havoc Pennington 2006-07-28 03:13:36 UTC
I'm not looking at source right now so may misunderstand, I think it'd be better to add meta_core_get_current_time() than to poke stuff into gdk_display
Comment 10 Thomas Thurman 2006-07-28 03:26:57 UTC
The reason I suggested doing it that way is that the next line passes gdk_display into meta_core_begin_grab op, and in the homologous place in event_callback we poke stuff into the display which its local "display" variable points to, then pass "display" into meta_core_begin_grab_op in just the same way.

I'm not sure what meta_core_get_current_time() would win for us, given that it'd need to be parameterised for both which display we were talking about and which event we were getting the timestamp from, and it would only end up being one line long anyway. You'd need a meta_core_reset_current_time() afterwards to reset it back to zero/CurrentTime, as well.

(Unless I misunderstood)
Comment 11 Havoc Pennington 2006-07-29 04:24:57 UTC
The difference I was thinking of (and again I may be confused) is that in event_callback display is a MetaDisplay* which is our object, while gdk_display is a Display* which belongs to Xlib. We should not be poking in there.

You most likely wouldn't want a reset_current_time; if that made sense, then the right solution would probably be to add a current time argument to the function you would call set/reset around.

MetaDisplay->current_time is (iirc) defined to be the time of the current event - it shouldn't be assigned to otherwise. event_callback owns the task of setting it to the current event's time.

meta_core_get_current_time would simply return MetaDisplay->current_time, i.e. get the time of the current event.

I guess the problem here is that we have a GdkEvent with a time, and MetaDisplay->current_time has been set back to 0 since event_callback has already been run for the corresponding XEvent? Our event filter in core runs before the gtk signal handler in ui.

If so some solutions I would suggest:
 - pass gdk_event->time into begin_grab_op (and modify event_callback to 
   pass display->current_time in)
 - see if MetaDisplay->current_time can be reset to 0 only after 
   gtk processes the event (don't think this is possible, but it 
   might be the ideal solution)
Comment 12 Elijah Newren 2006-07-29 15:01:55 UTC
If you investigate a little further, it turns out Havoc's first suggestion is the right direction for the fix, though begin_grab_op already has a timestamp parameter.  This is another one of those focusing things.  meta_display_begin_grab_op() calls meta_window_grab_all_keys() which calls meta_window_focus().  However, meta_window_grab_all_keys() does not take either an event or a timestamp as a parameter (bug!) so it uses our common hack of calling meta_display_get_current_time().  meta_window_focus() needs a _valid_ timestamp.  Calls to meta_display_get_current_time() often indicate a bug (even more so than uses of CurrentTime) and that a timestamp should be passed instead; see bug 152000 for more details (we just kind of wimped out in that bug and figured we could replace other occurrences as we discovered they were problematic -- hence this issue)
Comment 13 Elijah Newren 2006-07-29 15:36:56 UTC
I haven't tested it, but in looking over the patch, I have a few comments.  Overall it looks good as Thomas said but there are a couple things I found to comment on:

It looks like the patch handles mouse-only window moves (e.g. click on titlebar and drag), but not mouse-only window resizes (e.g. click on window border and drag).  Could you verify?  It ought to handle mouse-only window resizes as well.

-  if (grab_op_is_keyboard (op))
+  if (window)
+    display->grab_have_keyboard = meta_window_grab_all_keys (window);
+  else
+    display->grab_have_keyboard = meta_screen_grab_all_keys (screen);

This is going to cause the keys to be grabbed even when the user just clicks on the minimize button.  It'd be better to only conditionally grab as before and just change the condition from grab_op_is_keyboard to something like grab_op_is_keyboard or grab_op_is_mouse_only (suitably defining grab_op_is_mouse_only, of course).

+      /* End resize and restore to original state.
+       * The move_resize is only needed when !wireframe
+       * since in wireframe we always moveresize at the end
+       * of the grab only.
+       */
+      if (!display->grab_wireframe_active)
+        meta_window_move_resize (display->grab_window,
+                                 TRUE,
+                                 display->grab_initial_window_pos.x,
+                                 display->grab_initial_window_pos.y,
+                                 display->grab_initial_window_pos.width,
+                                 display->grab_initial_window_pos.height);
+      display->grab_was_cancelled = TRUE;

This is just cut-and-paste from elsewhere in the code (which is fine), but in one of my half dozen or so patches sitting on my hard disk from a few months ago, I changed other copies of code like the above to

  /* End resize and restore to original state.  If not in
   * wireframe mode, we need to do a moveresize now to get the
   * position back to the original.  If we are in wireframe mode,
   * we need to avoid allowing meta_display_end_grab_op() to 
   * moveresize the window to the position of the wireframe.
   */
  if (!display->grab_wireframe_active)
    meta_window_move_resize (display->grab_window,
                             TRUE,
                             display->grab_initial_window_pos.x,
                             display->grab_initial_window_pos.y,
                             display->grab_initial_window_pos.width,
                             display->grab_initial_window_pos.height);
  else
    display->grab_was_cancelled = TRUE;

I think that makes what the code is doing a little more clear.  grab_was_cancelled isn't used a whole lot and this comment quickly explains what the code is doing and doesn't confuse grab_was_cancelled as being a variable useful outside of the wireframe mode.  (Yes, the same patch added a comment beside the grab_was_cancelled var of the MetaDisplay struct.)  This change isn't necessary for your patch as I can clean it up with the other ones that need modification later, but I thought I'd point it out.


Marking as needs-work based on the conditions-to-grab thing and the mouse-only resize issue, but it looks like you're close.  Thanks for the patch!  :)
Comment 14 Elijah Newren 2006-08-21 18:39:48 UTC
I wanted to get this in before 2.16.x, so I made the minor modifications needed that I mentioned in comment 13.  Thanks for the patch Thomas, and to Thomas for testing and helping review!

2006-08-21  Elijah Newren  <newren gmail com>

	Allow drags & resizes to be reverted by hitting escape.  Based on
	patch from Thomas Andersen.  #126497.
Comment 15 Elijah Newren 2006-09-01 18:06:08 UTC
*** Bug 353882 has been marked as a duplicate of this bug. ***