Created attachment 53001 [details] [review]
implementation patch

here's the patch...

(ps: in passing, is it possible to bind a keybinding to a combination of mouse
and modifiers?  e.g. "Shift-left-mouse-button"?   if not, let me know and I'll
file a bug for that too ;)

While this is possible with the mouse (with HEAD anyway), it may make sense to
have a similar ability available for keyboard users (see bug 106470 comment 39
for related rationale).  A couple points:

- one can achieve the same effect, using a few more keystrokes, using the
keyboard snapping abilities (after alt+f7, hold shift when pressing arrow keys;
it's also worth noting that this keyboard snapping has been fixed on head so it
doesn't display the same bugs it used to).  Do we expect users to have
sufficiently many windows that your in-one-keystroke-only keybindings would be a
large enough advantage?

- your gconf key names are vague and will likely cause confusion.  Something
like move_to_ne_corner would be much better.  Same thing goes for function names.

- your keybindings provide no ability to snap to the edge of the screen in a
single direction.  Perhaps it would be better to provide this ability and then
users could just use two keybindings to snap to the corners?  (Admittedly,
that'd be a lot nicer if bug 112560 were fixed first)

First and third points will need to be explored/discussed before any decision
can be made; marking patch as needs-work due to the second point.

*ahem* I meant bug 106740 comment 39, not bug 106470.  :)

regarding 'ability to snap to the edge of the screen in a
single direction' (point 3) -- to be honest I'm not sure I see the utility of
moving a window towards the middle of a screen side, instead of a corner.   this
could be just my idea of what's useful, though, I clearly use the screen corners
to hold my windows. ;)

The difficulty with multiple move keypresses (point 1), is that it requires the
user to mentally compute how many keypresses will be required.  IMO, it's more
natural for the user to simply hit the key and know the desired action has been
taken.  It also interacts badly with focus-follows-mouse; would the second or
third presses be accidentally sent to the now-exposed window that was partially
underneath the target?

re second point: renaming is no problem.

Comment on attachment 53001 [details] [review]
implementation patch

Leaving the big picture issues aside, when you dereference window->frame you
need to check that it's not null (undecorated windows)

regarding unidirectional edge snapping: I wasn't thinking of anything like a snap _towards_ the middle, just something that would keep one coordinate fixed while moving the window.  Part of the reason for the question is because with partial struts (i.e. taskbars that don't extend the full range of the screen), "corner" may not be well defined.  In such cases, since the real corner of the screen is covered with a panel, there will be multiple "corners".  Your patch would make the window move to the point as close as possible to the actual corner of the screen (which would be one of the "corners"), but why aren't there quick access keys for the other corners as well?  Having the user be able to e.g. throw their window as far left as possible and then as far up as possible should be nearly as quick for you while allowing a little more versatility for such users by allowing them to pick the order of the directions.  Besides, perhaps there's utility for just slamming a window to one side instead of moving in both directions?  I really don't know what'd be better, I'm just trying to bring up things that may not come up in your normal usage but which might affect how we want such a patch to work. 

It looks like you didn't actually try the focus-follows-mouse thing, or only did so with other window managers.  ;-)  It's not relevant with Metacity because (a) the pointer is warped during the movement preventing another window from getting focus (though that's kind of annoying in some ways of its own), (b) we have a grab during that time so we could keep other windows from getting focus due to that, abd (c) we've entered keynav mode (similar to alt-tabbing) so that it isn't the window under the mouse that's relevant for focus choices until the user moves the mouse around again.

Point taken on the mental computation of how many keypress would be required; I think that'd probably be a good reason to have a fixed-up version of this patch or a uni-directional version.

Elijah --

many thanks for the comments!

Partial struts are, indeed, something I hadn't considered.  Given that, a "side" version makes sense (in addition to the "corner"?)

true enough, you've caught me on not actually *trying* that focus-follows-mouse thing in metacity. busted. ;)

Anyway, I'll have a go at another version of the patch RSN, once I get some more tuits.  As a matter of interest, what should I do to avoid the issue Havoc raised?  simply ignore windows for which window->frame == NULL?

side version + corner sounds like it may be reasonable; let's ping Havoc and see what he thinks.

Since you need to know the size of the window, and need to add the size of the frame to it if there be one, just make use of meta_window_get_outer_rect() to determine those dimensions; e.g.
  MetaWindow outer;
  meta_window_get_outer_rect (window, &outer);
Then, instead of refering to the window frame's width and height (which will crash whenever the window has no frame), you can refer to outer.width and outer.height which will be safe and handle both cases.

I have no real opinion on these keybindings, except they should probably be disabled by default since they most likely aren't worth "using up" the global keyspace

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.

Created attachment 68219 [details] [review]
metacity-2.14.5.jm.corners.patch

hi guys --

Here's an update of that patch, against 2.14.5. 

- It renames the functions and keys to "move_to_corner_{x}" (where x = nw/ne/sw/se), as requested.

- It also adds a set of "move_to_side_{x}" (where x = n/s/e/w) bindings, which move the window to be flush with the screen edge.

- The bindings are unset by default.

- I used the meta_window_get_outer_rect() api as suggested, but had to
traverse window->frame anyway -- the outer rect alone didn't provide
enough info to get the window locations flush with screen edges... however, I'm checking to ensure it's non-NULL as requested.

Works nicely, too ;)

Gosh, this fell off the radar a bit.

It looks good apart from two things:

1) a bit of code rot, easily fixed;
2) the four last params of handle_move_to_corner_backend are ints which can be zero or nonzero; stylistically they ought to be gbooleans to match the rest of the code.

I have made these two changes and committed. It will be in the next 2.17.* release. Thank you very much!

hooray!  thanks!

Sorry it sat unreviewed for so long! I'm in the middle of a reviewing/bugclosing blitz.