Re: constraints_experiments is ready

[Elijah Newren <newren gmail com>]

On 11/17/05, Havoc Pennington <hp redhat com> wrote:
> On Thu, 2005-11-17 at 07:36 -0700, Elijah Newren wrote:
> >
> > I find that odd since I really didn't change much (just the minimum
> > necessary to make it actually work) and was just documenting existing
> > practice.  The comments would have been the same plus additions
> > (covering bugs, mainly) to explain how the original version happened
> > to be used.  You may not have realized that their usage had become so
> > strange, but it's there on head as well.  ;-)  My huge
> > move_resize_internal comment would be useful on head even if the other
> > code on the branch didn't get merged.
>
> I realize it's not your fault the overall situation is hosed, but at the
> same time I don't understand the one change (the gravity stuff). Maybe
> point me to the bugs that change fixes?

Which change?

> I'm not trying to veto your patch just think about the situation.

That's fine; especially given how long it took me to figure out how
meta_window_move_resize_internal() and all the gravity stuff works.

> > > - another thing to check is resizing with all the different gravities
> > >   (we have a test program for that iirc)
> >
> > Right, I used that program.  I fixed all the bugs that Metacity had
> > with it so that it works correctly on the branch.
>
> Ah, so maybe this answers my question I just asked ;-)
>
> Could the "gravity" arg to move_resize_internal now be reduced to a flag
> "use gravity size hint" vs. "just use static gravity" ? (corresponding
> to application configure request vs. metacity move-resizing internally?)
> No clue what the answer to this question is.

No, it couldn't.  Doing that would (a) cause bug 312007 to return (the
constraints code *must* know the gravity corresponding to the user or
application action or else stuff like minimum size hints will cause
windows to move when attempting to resize them back to the minimum
size), (b) break _NET_MOVERESIZE_WINDOW (whose whole point is that
apps may want to resize with a different gravity than their normal one
and it's a race for them to change their gravity and then do a
configure request).

> Another idea is to just split apart the configure request handling from
> our internal resizing.

That would make sense and probably make
meta_window_move_resize_internal() much saner; it would mean placing
the adjust_for_gravity call in the meta_window_new_with_attrs(),
meta_window_configure_request(), and maybe
meta_window_apply_session_info().

> > I just cut & pasted it from src/display.c to src/boxes.c and similarly
> > for meta_rectangle_equal().  Now that you bring it up though, I wasn't
> > very sure about who the original author of those two functions were
> > and so my copyright notice in src/boxes.c might not be correct.  Could
> > you check?  I attributed it to you and Rob and Anders.
>
> Pretty sure it's Owen, or "the gtk team" - anyway it's copied from gdk.

For both meta_rectangle_intersect() and meta_rectangle_equal()?

> I think we're just using different terminology.
>
> window->rect.x would be the X server position of the window relative to
> its parent (frame or root window)
>
> On initial map of a NW gravity window we are going to create a window
> frame and put it at the old window->rect.x and move window->rect.x by
> the frame offset, but window->rect.x itself is never the position of the
> frame because when we add the frame we change window->rect.x to be
> relative to the frame (and we move the window to the right by the width
> of the left frame edge)
>
> i.e. it's an invariant that window->rect.x is the server-side static
> gravity position of the window relative to the window's parent (frame or
> root)

Re-read and analyze the algorithm you just described.  The effect is
that the x & y specified by the app corresponds to the location of the
frame window, rather than the internal/app window.  My comment was
just trying to explain that.

> > I think I can explain them.  Let me start by looking at cases other
> > than new window and session restore.  If the window's gravity is not
> > passed to meta_window_move_resize_internal() then bugs like 109953 and
> > 312007 result.  So, it is necessary for configure requests and user
> > resize operations to do that.  The ICCCM says that initial window map
> > should be treated like a configure request (at least, according to one
> > of your comments in the code).  So it makes sense to treat it the
> > same.
>
> Right
>
> > As for session restore, yeah, I'm not so sure.  I can hard code
> > NorthWestGravity if you like (that's what used to be done before), but
> > since hardcoding NorthWestGravity was done in multiple other places
> > (in some cases in a hidden way) and was wrong and buggy and had to be
> > fixed by using the window's gravity, I figured it probably made sense
> > to change the session_restore one too.
>
> For session restore you have to figure out if we're saving the position
> assuming gravity or not. i.e. if we have a SouthEastGravity window, are
> we saving the lower-right corner of the frame? If so then we want to use
> SouthEastGravity on restore. If we always save the top-left corner then
> using gravity on restore would break. Right?

Ah, makes sense.  And now that I grep over the code again it appears I
looked into this before as well.  The session saving uses
meta_window_get_geometry() which uses
meta_window_get_gravity_position() in order to figure out the location
to save.  So hard-coding NorthWestGravity in the call from
meta_window_apply_session_info() to meta_window_move_resize_internal()
was indeed wrong and just happened to have been corrected by the fact
that adjust_for_gravity() ignores the gravity passed to
meta_window_move_resize_internal() and instead uses the size hints
gravity.

I think this might be the clearest picture I've said yet, so let me
repeat something in case it didn't make sense elsewhere: We can't
really continue with handling gravity like this because the fact that
meta_window_move_resize_internal() is told the wrong gravity means
that constraints is told the wrong gravity so that minimum size hints
can't correctly handle fixing up the size (i.e. bug 109953).  It also
would break _NET_MOVERESIZE_WINDOW...

> > The fact that you didn't use the window's gravity caused multiple
> > bugs, 109953 and 312007 in particular.  The gravity you should have
> > wanted to use _was_ the size hints gravity in all cases other than
> > maybe session restore.
>
> I don't think that's right - e.g. if we're doing a resize of a window
> because the user is dragging the edge of the window, using the window's
> gravity I would expect to break badly, no? The size_hints.gravity is
> meant to change the interpretation of application configure requests,
> for our own internal stuff I wouldn't think we want it...

Note the addendum I sent; this paragraph was indeed wrong for the
reason you mention.  I corrected it in the follow-up email.

> > >  The adjust_for_gravity() thing
> > >   was supposed to handle the size hints gravity.
> >
> > That covered up for some of the bugs from having hardcoded
> > NorthWestGravity in a couple places instead of using the appropriate
> > gravity.
>
> But the gravity is a property of the resize *request* not of the window.
> i.e. it's an interpretation of the x/y arguments.

Yes, and the resize request continues to be handled in the constraints
code (size increments, minimum size, maximum size, etc.) and so the
constraints code *must* know about the gravity property associated
with that request; that's why the gravity associated with the request
needs to be passed to meta_window_move_resize_internal() (and also why
adjust_for_gravity(), which is only called for
configure-request/new-window/session-restore cases, needs to pay
attention to that gravity instead of ignoring and overriding with
size_hints.gravity).

> So anywhere inside metacity where we come up with an x/y to move a
> window to, if it doesn't hardcode a gravity we need to be passing a
> different x/y according to size_hints.gravity (I believe, right?)

If x/y is specified without gravity, and we know that it's "the normal
x/y thing", then we can just pass NorthWestGravity to
meta_window_move_resize_internal() and everything works great.  But
currently, there are no such cases in the code (unless you want to
count metacity-handled resize operations, in which case we do exactly
that and it does indeed work well).

> Agree with you that on initial map that was an app request so we should
> have used size_hints.gravity.
>
> One way we might think about it is that the gravity arg to
> move_resize_internal should be a gravity *OR* "this is from the
> application, use size_hints.gravity"

If it's from the application (and it's not a _NET_MOVERESIZE_WINDOW
message that specifies its own gravity), why not just have the
application pass size_hints.gravity to
meta_window_move_resize_internal()?  It's exactly what we need in all
three cases anyway (configure request, new window, and session
restore; which occur from meta_window_configure_request(),
meta_window_new_with_attrs(), and meta_window_apply_session_info()
respectively).

- Elijah