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?

Retitling to make slightly more clear, after conversation with Seb in
IRC. 

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.

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.

Oops, forgot to add the gnome-love keyword.  This should be relatively straightforward with the steps I provided to fix this in comment 4.

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?

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.

(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.)

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

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)

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)

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)

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!  :)

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.

*** Bug 353882 has been marked as a duplicate of this bug. ***