Re: constraints_experiments is ready

[Havoc Pennington <hp redhat com>]

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?

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

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

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

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

> >   wait, coming back to this comment after reading frames.c change,
> >   now I'm just confused - is the idea you get a menu on mouse 1
> >   click of any edge?
> 
> Only when it's offscreen. 

OK, sounds worth a shot.

> (the latter was used for example with the find-neighboring-xinerama
> functions).  I decided to consolidate them since it seemed odd to use
> a slightly different name for essentially the same thing in every
> file.  The alternate naming of TOP/UP and BOTTOM/DOWN just to allow it
> to be used for both cases.  I can split the two types of enums if you
> really want, though it becomes a little harder to think of a good enum
> name to differentiate the two.

It's fine the way you have it.


> > - In this comment I think you mean "border" not "frame" ?
> 
> Only if I misunderstood the documentation I was reading.  A comment of
> yours in the code just before the call to
> meta_window_move_resize_internal() from meta_window_new_with_attrs()
> says that "ICCCM says initial map is handled same as configure
> request" and the documentation for XConfigureWindow says that
> 
> "The x and y members are used to set the window's x and y coordinates,
> which are relative to the parent's origin and indicate the position of
> the upper-left outer corner of the window. The width and height
> members are used to set the inside size of the window, not including
> the border, and must be nonzero, or a BadValue error results."
> 

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)

> > +  /* The x & y correspond to the outer window (i.e. including frame),
> > while
> > +   * width and height refer to the inner window (i.e. ignoring the
> > frame).
> > +   * Stupid, stupid, stupid X.  See documentation for
> > XWindowAttributes,
> > +   * XGetWindowAttributes, XMoveWindow, and XResizeWindow.
> > +   */
> >
> 

> > - you have a comment about "should maximize horiz/vert only have
> >   visibility in UI" I would say yes, and this is part of the problem
> >   with their existence at all :-P is that there isn't a good way to
> >   make this visible
> 
> Right, I was basically just working off your comment in bug 113601
> (comment 6).  I figured it made sense to leave a FIXME in the code in
> case anyone wants to do the theme & session fixups that would be
> necessary.  Should I make any changes here?

Don't know. It might be bad to make it visible just because it means
more work and complexity for a goofy hidden feature. Anyhow, I don't
think there's anything important to change here.

> 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?

> > - in general I would say move_resize_internal() needs work. the
> >   big comment there seems to confuse more than enlighten ;-)
> 
> Note that my comment is not there for documenting changes I made--it
> was for documenting the mess that already existed.  I couldn't really
> find a way to clean it up either and thus the reason for documenting
> it instead.  I think dropping the comment and just leaving the mess
> there would cause more problems, so I'm not really sure what you want
> me to fix or how.  Maybe just figure out a way of re-wording it to be
> more clear?

I think this might be a case where changing code is more clarifying than
documentation ;-) but as long as the gravity thing is sorted then
leaving the comment is a fine start.

> > - The point of the original code was that the gravity passed to
> >   move_resize_internal was simply the gravity we programmatically
> >   inside metacity wanted to use. i.e. which corner to move in
> >   order to implement a resize.
> 
> 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...

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

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?)

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 we always use NW gravity
internally, could be reduced to a bool "NW or application request"
but I dunno if it works

Havoc