Created attachment 49408 [details] [review]
Fixes metacity's keyboard resize and move behavior

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

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

I can't duplicate -3- so I don't know how to test it.

The patch works quite well for me.  I did uncover a couple problems in testing:

- you've introduced wraparound and other weirdness with snap-moving.  Hit
Alt+F8, then shift+right a bunch of times and you'll see that whenever it should
move to the right of the screen it moves to the left instead (and can be
partially offscreen).  This is really funny when snap-resizing the left side. 
Hit Alt+F8, shift+left, and then shift+right a couple times--moves the whole
window to the right instead of just changing the position of the left side.  It
doesn't wrap when moving to the left, though.

- from reading the patch I had thought that you had fixed bug 304857 (moving
wireframe offscreen is misleading as it snaps back onscreen after placement is
finished).  Did you intend to fix that as well with that patch, or did I just
get my hopes a little too high?

- resize increment windows (like gnome-terminal) still behave a little weird
with snap resizing if not in wireframe mode (though not as weird as before). 
Oddly, this seems to work fine in wireframe mode (sans the new bug I pointed out
above)

Hi Elijah,

To test -3-, revert the patch, press alt-f8 on a window, then press up arrow
followed by left arrow.  The mouse pointer will move to the northwest corner of
the window and there will be no way to move it back to one of the flat edges. 
The easiest solution is to not allow it to go to the corners with the keyboard.

I didn't notice the wrap around in my testing, but I see it now.  I'll look into it.

I hadn't planned to specifically address the snap back problem, although I did
notice it.  I might be able to take a look at it.

Do all windows with increment size hints set act weird or is it just
gnome-terminal?  I'm asking because it might just be vte fighting metacity
because of some race or something. 

Ah, gotcha.  Yeah, that makes things nicer for -3-.

If you have time to look at closely related bugs (I'm not trying to push but
thought I'd save you some time searching in bugzilla if you're interested in
this category of bugs), besides bug 304857 it'd be great if you could also check
out bug 124582 (I noticed your wireframe snap-move doesn't exhibit this
multi-direction bug), bug 86644 (allowing snap resizing with Alt-middle-click +
shift), and bug 122670 (jerky resize when holding down arrow key; I started
debugging but haven't had time to look at it again).  

I don't know of any other increment size hints windows to test with.  It may
very well just be vte.  *shrug*

Created attachment 49440 [details] [review]
fix weird wrap backward behavior

This updated patch shouldn't have the weird wrap backward behavior.  The old
patch's "next edge" finding routines returned bogus fallback edges in the cases
they couldn't find their respective next edges (i.e., when the window is at the
edge of the screen).

The wraparound while moving the window to the right bug is fixed, but the move
the entire window to the right when shrinking the window by snap moving the left
side isn't.  Just try it out with Alt+F8, left, shift+right, shift+right,
shift+right, etc.  Eventually, the whole window jumps.  Trying the opposite
direction, and the window just stops once it gets to a certain size and the left
edge of the window never budges.

Created attachment 49860 [details] [review]
dont allow window to change position if it won't be resized

Hi, I spent some time looking at this tonight.

So resizing from the top or left edge implies two changes:

1) change in window position (because a window's position is given by it's top
left corner)
2) change in window height or width

The problem was the code was doing change 1 first, then trying to do change 2
(and possibly failing).  

If the window is already shrunk as much as it is allowed to shrink in a given
dimension and a resize attempt tries to shrink it more yet then (2) won't
succeed and (1) was never being undone.

I've changed the code so that it makes sure that (2) will succeed before doing
(1).

Created attachment 49861 [details] [review]
Add a > 0 where it should be.

So right after I clicked send I skimmed through the patch one more time and
noticed this mistake:

+	   else if (height - height_inc)

which should be

+	   else if ((height - height_inc) > 0)

Well, I can get the same bug as before...but only for special windows and it
turns turns out that it's not new (and it's definitely no worse than previous
behavior where a shift+right on the left border would inexplicably move the left
border further left).  So the patch seems to work fine in my testing.  I filed
the other problem as bug 312007.

Comment on attachment 49861 [details] [review]
Add a > 0 where it should be.

Dynamically allocating the rect in draw_xor_rect() seems a bit odd, why not
just "MetaRectangle rect" on the stack? Feel free to ignore this comment if I'm
missing something.

Other than that nitpick I didn't review too closely, but it looks plausible and
if it works I think it's fine to commit.

Hi Havoc,

There was no particularly good technical reason I did it that way, only
historical reasons.  The stuff in effects.c was actually done in March
as a potential workaround for 

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=153022

(The idea being, if you move the wireframe inside the window area then the
window contents overwrite any screen dirt left behind after the wireframe
operation finished).

Since the workaround was just supposed to be a simple bandaid to treat the
symptoms of the problem I decided at the time to make the patch as small as
possible (versus doing s/rect->/shrunk_rect./g and making the patch more
complicated.  

I ended up fixing that bug instead of working around it, so the patch just
stayed on my hard drive until I did this bug. When I did this bug, I needed the
 wireframe-completely-in-window-area behavior again, so the wireframe edge would
line up against window edges when snapping.  When I realized I needed it, I just
applied the old patch and moved on.

Created attachment 50159 [details] [review]
Don't use heap for temporary rect variable.

Hi,

So I committed this:

2005-08-03  Ray Strode	<rstrode@redhat.com>

	Improve the behavior of keyboard move/resize and edge
	snapping.  Still not perfect, bug 310888.

	* src/effects.c (draw_xor_rect): Make the outside of a
	wireframe rectangle line up with the outside edge of its
	window, instead of centering the wireframe edges on the
	window edges.

	* src/keybindings.c (process_keyboard_move_grab): allow
	edge snapping in wireframe mode.  Adjust code to take
	into account changed semantics of find_next_*_edge
	functions.
	(process_keyboard_resize_grab_op_change): new function
	to take some orthogonal logic out of
	process_keyboard_resize_grab_op.  Only allow keyboard
	resize cursor to go to flat edges, not corners.
	(process_keyboard_resize_grab):  allow edge snapping in
	wireframe mode.  Fix up snapping logic.

	* src/place.c (get_{vertical,horizontal}_edges): use
	GArray instead of int *, since the number of output
	edges isn't known until the middle of the function now.
	Use xor rect extents instead of window extends if in
	wireframe mode.
	(meta_window_find_next_{vertical,horizontal}_edge: add
	new source_edge_position parameter to specify which edge
	on the active window to start from when looking for next
	edge on the screen. Return the coordinate of the edge
	found and not the coordinate of where the window should be
	moved to snap to where the edge was found.

	* src/window.c (update_move): all the user to specify
	an edge to resize with mouse in keyboard resize mode.
	window

Sweet, thanks for the work on this Ray, this patch makes things much nicer. 
I'll go ahead and close; I think the other issues are already filed and tracked
under 155458 (and if there are others missing, we probably need to open a new
bug on them anyway).