GNOME Bugzilla – Bug 118598
Feature Request: Window Packing
Last modified: 2020-11-07 12:37:05 UTC
Metacity lacks window packing options for keyboard shortcuts. Example from Sawfish. pack-window-down pack-window-up pack-window-left pack-window-right allowed you to bind a key combination to these commands and to move your windows using the keyboard. This had to be the single most usefull feature of sawfish and I would love to see it in meta city.
what exactly do these commands actually do when you run them? Have you tried holding shift while you move the window? Does this do what you want?
What those commands allowed you to do was snap windows to each other without removing your hands from the keyboard. It would allow you to open several windows and move them around the screen withough using the mouse. Holding the shift key is helpfull but doesn't allow me to work with many terminals without using the mouse. The behavior of those commands would be much the same only pack-window-left would cause the window in focus to move to the left flush with either the edge of the screen or the next window to the left.
Well, I certainly wouldn't be opposed to a patch that implements these keybindings. But I think most users would just be utterly baffled by them.
This is the same as typing: Alt+space, m, Shift+leftarrow So it is possible via keyboard now though I admit a shortcut binding might be nice.
Created attachment 46740 [details] [review] Implements keyboard shortcuts for packing windows Hi, This patch is against Metacity (CVS, 2005-05-22), and adds shortcut keybindings for packing windows around the screen. This is my first try at hacking Metacity, so please review this patch carefully. What this patch does is: - It factors window moving code out from process_keyboard_move_grab to a dedicated function do_move_window. - It adds a new keybinding function handle_pack_window, and installs all necessary (hopefully) hooks. As a result, the window packing functionality can be accessed directly with one keystroke, which is considerably more convenient.
Comment on attachment 46740 [details] [review] Implements keyboard shortcuts for packing windows Some formatting nitpicks, for example "do_move_window(" should always have space before parens "if (handled) {" should put braces on their own line. I don't see anything wrong on a high level assuming the various cases have been tested (try with shift, control, etc. for both old and new ly-added functionality, be sure to test in reduced_resources and try things like quitting the move with Escape)
Created attachment 47468 [details] [review] Keyboard shortcuts for packing windows I'll attach an updated patch to conform better to Gnu coding style, in case someone is interested. The patch has now been in my personal use for some time, and nothing unusual has cropped up. The shortcuts and the Window menu -> Move keyboard support has been tested, both with and without reduced_resources. It should not introduce any changes to the previous behavior, just add the new shortcut keys. (Note that with reduced_resources, when moving windows via Window menu -> Move, pressing Shift has no effect. This is how Metacity functions even without this patch, and fixing it should be done separately, if needed at all.)
Sorry for the slow response; I was going to try this out. Do you have a version updated against CVS HEAD? The current version has 4 hunks that fail in keybindings.c.
Sorry for the delay on my part -- it appears I did not get mail on your reply (was I not on the CC list?) and noticed it only now. A patch updated for CVS HEAD is attached. Given some changes in metacity HEAD, now packing windows works even if reduced_resources is enabled.
Created attachment 60695 [details] [review] Implement keyboard shortcuts for packing windows. Updated for CVS HEAD.
Patch looks pretty good, and seems to work in my testing. There's a few things that need to be fixed up: + if (direction & META_DIRECTION_UP) { Braces should be on their own line; there's a few of these. + meta_display_end_grab_op(display, Spaces between function name being called and the opening parenthesis; there's a few of these as well. + meta_display_get_current_time(window->display)); meta_display_get_current_time() should not be called in functions that already have a timestamp. In the two of these that you have, you should be using event->xkey.time instead. Calls to do_move_window() would be clearer if you used enums instead of gboolean arguments for smart_snap and small_move. Havoc has been pushing for this and I've found it makes code much nicer to read. :) Thomas, Björn: Any chance I could get one or both of you to test as well and check for any potential problems?
Created attachment 61989 [details] [review] Keyboard shortcuts for packing windows. Thanks for your comments, I've now updated the patch. Also fixed a small deviation from the non-patched behavior, in the handling of XK_Escape.
I think there are some problems about the patch. I can't say if they are caused by the patch or just dormant problems showing up. When you are moving a window (the pointer is a cross) and you press the shortcut for packing it, you get the error: Attempt to perform window operation 10 on window 0x2400003 (Fela Aniku) when operation 1 on 0x2400003 (Fela Aniku) already in effect. I believe that is caused by the meta_window_begin_grab_op which also causes the pointer to flicker as it is turned to a cross and then back to its normal pointer state. Also, handle_pack_window sporadically crashes with the error Log level 6: file edge-resistance.c: line 551 (apply_edge_resistance_to_each_side): assertion failed: (display->grab_edge_resistance_data != NULL) when you pack windows so that their borders are placed outside the edges of the screen.
The first problem: the same warning message is printed also when the "Move window" or "Resize window" shortcut key is pressed, as also these routines try to get a keyboard grab. Should all the routines check whether a grab-equivalent activity is already going on before calling begin_grab_op (something like this seems to be done in some routines using primary_modifier_still_pressed), or should begin_grab_op be able to fail gracefully? The second problem: I am not able to reproduce this. Do the crashes occur if you choose "Move" from the window menu and move the window around the screen using arrow keys with shift pressed? However, the contents of handle_pack_window should probably be wrapped in "if (window && window->has_move_func) { ". Could you check whether the crashes still occur if this change is made?
Created attachment 62030 [details] [review] Keyboard shortcuts for packing windows. Also, the previous version did not check whether a grab succeeded. This one now uses meta_display_begin_grab_op which returns the success status. However, the cursor still flickers to the cross state when the window is packed -- the pointer state is changed in meta_display_begin_grab_op. Packing windows not taking a grab probably is possible, but it seems that currently Metacity's edge snapping code is somewhat tied to the grab code. Changing this may require larger modifications in parts of Metacity I'm not familiar with (at least yet).
You are right about the first problem, everytime you active a keyboard shortcut when you already have the window mouse-grabbed the "Attempt to perform window operation 10..." message is shown. I think it hints at an underlying problem but it has nothing to do with your patch. I cannot reproduce the crash in the second problem with your new version of the patch. Neither can I get the assertion to fail by shift+arrow-moving windows. It seems like the change from meta_window_begin_grab_op() to meta_display_begin_grab_op() and the check if a grab_op is in progress solves the problem. I think the problem is that the if statement on line 3387 in display.c succeeds and meta_display_begin_grab_op() returns FALSE before the edge resistance stuff have been setup which is done on line 3515. Some more comments: If possible, I think that it is better not to grab the display. I don't know how to avoid it, but there should be some way. :) IMHO code that uses guards instead of nested if statements are more readable. Instead of: if (some condition) if (some other condition) { do stuff ... } You write: if (!some condition) return; if (!some other condition) return; do stuff ... I don't know if the Metacity devs agree with me though. :)
Thanks for all the extra testing Björn. :) The "Attempt to perform window operation...when operation...already in effect" warning is just common to metacity's structure and isn't anything to worry about. We don't have a way to nicely switch operation type and in fact even require users to release all keys before starting a new operation (see bug 112560 for more info). I don't know how Björn was able to get the crashes with the older version of the patch. I'm not doubting it, I just wasn't able to figure out how to duplicate even after trying to look through the code for clues. Björn: Any pointers on how I might try to do so? I'd like to understand how it can do that even with the older version. In terms of the grabbing, the patch doesn't actually grab the server (i.e. XGrabServer); it just tries to grab the keyboard and the mouse. You could perform this operation without either of those grabs (see handle_move_to_workspace() for an example of another op that acts like this), but you'd have to handle computing the resistance/snapping edges manually (which might require modifications to edge-resistance.c) and it may not make sense to not grab the pointer since you're warping it. Of course, warping the pointer causes a bug of its own (for mouse focus, anyway) and we may want to get rid of that... Regarding the nested if's versus if & return, I disagree with Björn. ;-) Additionally, it looks like Paul was just copying the old code, and since changing as little as possible makes patches much easier to follow, this is a nice bonus. So, let me summarize what I think are the remaining issues: - I want to understand how Björn got a crash with the old version of the patch - We need to determine if we really want to do the pointer warping thing (though, I'd be fine with punting this into being part of bug 320108) - If we do keep the pointer warping thing, the bug in mouse focus where the window loses focus when the pointer goes over the panel needs to be fixed. I'll mark the patch as needs work because of the 2nd or 3rd items; though I want to understand the first to make sure there isn't some deeper bug in the edge resistance stuff or elsewhere before it's applied. Thanks for the work, Pauli. :)
It was these lines in the old patch: + meta_window_begin_grab_op (window, + META_GRAB_OP_KEYBOARD_MOVING, + event->xkey.time); Note that the grab_op can fail (because it calls meta_display_begin_grab_op()), but the code didn't take that in account. In meta_display_begin_grab_op() there is code that checks if a grab is already in effect, if so it returns (its the if (display->grab_op != META_GRAB_OP_NONE) check). In that case, meta_display_compute_resistance_and_snapping_edges (display) isn't called. Because that isn't called, initialize_grab_edge_resistance_data() isn't called and that causes g_assert (display->grab_edge_resistance_data != NULL); to fail. I guess the reason I can trigger the assertion failures is because I have such a slow graphics card (and big screen) so that handle_pack_window() is reentered before the grab has ended in meta_display_end_grab_op (display, event->xkey.time);
Ah right, that makes sense. I must have been dense, because I looked through that part of the code and missed it. So, that just leaves fixing one thing (either the 2nd or 3rd item mentioned in comment 17) before we can commit this.
#2: This can probably be taken care of by removing the call to meta_window_update_keyboard_move. #3: I am not able to reproduce this (if I understand your comment 17 correctly). For me, when the pointer is warped on top of the panel, the window that I am packing around the screen still retains its focus.
(In reply to comment #20) > #3: I am not able to reproduce this (if I understand your comment 17 > correctly). For me, when the pointer is warped on top of the panel, the window > that I am packing around the screen still retains its focus. Which focus mode were you using (i.e. what's the output of 'gconftool-2 --get /apps/metacity/general/focus_mode')?
Ok, I see it now, if the focus mode is set to "mouse", but not if it is "sloppy" or "click". I thought that you meant sloppy focus, as the mouse focus cannot be selected from gnome-window-properties.
btw, I think you should be able to fix #3 by just turning mouse_mode off (since they are using the keyboard to navigate/manipulation windows instead of the mouse anyway...) See do_handle_move_to_workspace() for an example of how this is done.
I would suggest fixing the support for _NET_MOVERESIZE_WINDOW in metacity. That way this sort of enhancements can be supplied implemented outside Metacity itself.
bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all old feature requests in Bugzilla which have not seen updates for many years. If you still use metacity and if you are still requesting this feature in a currently supported version of GNOME (currently that would be 3.38), then please feel free to report it at https://gitlab.gnome.org/GNOME/metacity/-/issues/ Thank you for reporting this issue and we are sorry it could not be implemented.