Re: constraints_experiments is ready

[Elijah Newren <newren gmail com>]

On 11/17/05, Havoc Pennington <hp redhat com> wrote:
> Hi,
>
> I read over this but didn't check it at the math/algorithm level.
>
> My only high-level worry here is that move_resize_internal and
> gravity-handling situation seems to be getting worse instead of better.

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.

> Other than that most of my comments are nitpicky. Including below.
>
> I got back 4 hours ago from a 32-hour round trip to Germany, so if I
> seem a little confused I probably am.
>
> Havoc
>
> On Wed, 2005-11-16 at 11:21 -0700, Elijah Newren wrote:
> >   - I don't know whether XRandR events cause
> >     workspace.c:meta_workspace_invalidate_work_area() and
> >     workspace.c:ensure_work_areas_validated() to be called.  I have assumed
> >     that it does--if it doesn't, my stuff won't work (though I don't think
> >     the old constraints.c stuff would either).
>
> The flow should have been ConfigureNotify on root window ->
> meta_screen_resize() -> meta_screen_update_struts() in the old code

Sweet, so it all works.  :)  I haven't change that code, so it still
has that same order (well s/screen/window/ for the last function you
named as there never was a meta_screen_update_struts()) and
meta_window_update_struts() calls invalidate_work_areas.  Since my
code piggy-backs on the workspace validating and invalidation this is
all good.

> >   - XMMS and other I-wanna-be-my-own-window-manager apps have some issues.
>
> There's a shocker ;-)

:-)

> >   - Apps can be auto vertically-maximized or auto horizontally-maximized if
> >     they are too large when initially placed.  Users who aren't aware of
> >     the separate maximization states *might* find this confusing and try to
> >     resize from the edge, and then find they are unable.  Should I just
> >     make them maximal size in the appropriate direction instead and only do
> >     the auto-maximization if it's needed for both directions?
>
> Makes sense to me. I would think the single-direction maximization is a
> "hidden feature" only.

Okay, I'll make the change.

> Other comments:
>
> - I'm tempted to suggest redoing the ChangeLog patch into a "final
>   state only" ChangeLog entry, but maybe that isn't as useful as
>   seeing the full history

I think a quick summary final state makes sense.  I'm not sure that
reading about my aborted attempts and backtracks would be that
enlightening to keep in the real ChangeLog.  Plus I made the ChangeLog
a little more verbose on the branch so that other people would have an
easier time following things.

> - remember to drop README change on merge

right.

> - something to check still works is that if you randr the screen, the
>   windows get pushed onto the smaller screen, and then if you resize
>   back larger the windows go back where they were ; a similar case
>   is moving panels around, they should push windows away and then
>   the windows pop back if you remove the panel

I tested the moving the panels around case; works great.  I'll make
sure to test randr too.

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

> - hardcoding buf size 25 all over for rectangle to string is pretty
>   gross, suggest either suck it up and malloc this or define some
>   wacky macros that declare/use the buffer variable, or at least make
>   the buffer size a #define
>   (also I think there's a g_snprintf vs. snprintf, which guarantees
>   nul termination and is more portable)
>
> - you might #ifdef WITH_VERBOSE_MODE those things too

Yep, that does need cleaning.

> - I don't know if you changed rectangle_intersect(); the original was
>   based on the gdk one which owen was proud of for handling all kinds
>   of wacky cases, so if you changed it you might want to see if it
>   still does (cases were src == dest and negative dimensions and
>   crap like that)

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.

> - drop #ifdef PRINT_DEBUG in favor of meta_topic(), no? you could do
>   #ifdef WITH_VERBOSE_MODE when needed, or add a
>   WITH_EXPENSIVE_VERBOSITY or something

Probably WITH_EXPENSIVE_VERBOSITY or maybe even rip it out.  It was
there to get the ugly debugging done and left there "just in case".  I
haven't really needed it now that there's a working test program.

> - I think shove_into_region is an unclear name but no big ideas on
>   better ones
>
> - _find_linepoint_closest_to_point  has weird arg formatting
>   and no spaces around * operator ;-)

The weird formatting was partially due to the fact that we don't have
any MetaPoint structure and I found it easier to read point arguments
as x,y on the same line when there were several in a row.  Not a big
deal to change, though.  And yeah, the lack of spaces was a
make-it-easier-to-check-the-formula (since it'd all fit on one line)
and then just some absent-mindedness of forgetting to change
back--obviously not going to miss formatting problems even in a 14K
lines long patch.  ;-)

> - I think MENU_OP_RECOVER is a placebo for our consciences, since
>   the chances you know to alt+rightclick for menu but not
>   alt+leftclick for move seem pretty low ... ;-) but it's not
>   hurting anything since the item is only there if we're offscreen...
>
>   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.  Let me explain in more detail.  In bug
106740 comment 39, you said it was important usability-wise to:
  - allow slamming windows to edge
  - avoid accidentally getting windows under the panel
    (some apps for example try to hardcode a position at 0,0)
  - do the right thing when a new panel is added

But if you think about it, that should apply to all four edges, not
just the top.  Indeed the gripe in bug 152898 is that the different
edges of the screen are treated inconsistently.  Now, one way to treat
the edges consistently would be to disallow windows from moving off
the screen at all.  That was obviously a possibility but a choice you
ignored for the left, right, and bottom edges implying that there is
advantage to allowing the window to go off the screen.

The new code handles all these requirements (the three you listed plus
providing the advantages of allowing a window to be offscreen) by a
timeout resistance at the screen edge plus the require_fully_onscreen
and require_on_single_xinerama flags; users can throw their windows to
the screen edges easily, but if they really want their windows
offscreen then they need to continue to hold the mouse button down for
0.75 seconds before the window pops partially offscreen.  (0.75 was
just a random number I picked, but it seems to work and when I asked
Bryan if it should be altered he said things seemed to work find as
they are).

Now, the top edge is still somewhat special because most users need
their titlebars.  So, if a user is moving via the titlebar then they
meet infinite resistance at the top edge of the screen instead of just
a timeout resistance.  This is basically just implementing what you
suggested in bug 106740 comment 6 (which is now reasonable since the
more-important usability requirements of e.g. allowing easy slamming
of windows to the screen edge have been taken care of).  So, users who
don't know about alt-click or alt+f7 for moving can't move their
titlebar offscreen (good), and those that do know about them can do so
if they surpass the necessary resistance (good).

Rob had previously been worried about the case where users who don't
know about alt-click or alt+f7 accidentally move their titlebar above
the top panel.  That would be *extremely* difficult to do and I
really, really doubt it will ever happen.  BUT, I handled it anyway. 
I tested some random users (and, in fact, remembered  my own
experience with sawfish where this happened to me and I couldn't
figure out how to get it back onscreen for like 20-30 minutes--though
in that case it was due to a bug in sawfish: sawfish would place
windows initially too big for the screen with the titlebar offscreen,
which is something that won't happen with metacity).  This testing
suggested that users won't discover alt+click, alt+f7,
right-click-on-frame or right-click-on-tasklist very soon if it all. 
However, all of them try left-click-on-frame relatively soon (I guess
the idea is that the only way they know to modify the window is to
resize and moving the window onscreen is a form of modifying it? 
dunno, but this is just the experience).  Now alt+click and alt+f7
users don't need left-click-to-resize (and likely won't use it as it's
much less efficient due to being a very small area and Fitts's Law)
but even if they happen to do it anyway when the titlebar is offscreen
then showing a window menu is rather innocuous and something they'll
quickly learn.  In the hypothetical chance that a user who doesn't
know about alt+click or alt+f7 ever moves their window offscreen
(again, something that I strongly doubt will happen), this measure is
there to save them and seems to work well.

> - META_DIRECTION_UP/DOWN alternate names I find odd

We had a proliferation of enums which were of the form
   BLA_BLA_LEFT
   BLA_BLA_RIGHT
   BLA_BLA_TOP
   BLA_BLA_BOTTOM
and
   BLA_BLA_LEFT
   BLA_BLA_RIGHT
   BLA_BLA_UP
   BLA_BLA_DOWN

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

> - #include <config.h> removed from top of constraints.c, it's
>   usually right to have this as the first include in all files
>   (just to be sure it never ends up after some other header somehow)

I can add it.

> - "/* Stupid disallowing of nested C comments..." comment I'd just
>   take out ...

okay.

> - ConstraintPriority enum lacks spaces around "="

easy enough to fix.

> - maybe it'd be more obvious to just have:
>     struct Constraint {
>      const char * name; int priority; ConstraintFunc func;
>    }
>   and then an array of those? right now you're keeping
>   several arrays/enums in sync.

Hmm.  Makes sense.  Originally I only had the enum and the function
array was added much later.  Then, nearer to the end I tacked on the
function name array for debugging.  Seems like your method would be a
nice cleanup.

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

> +  /* 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.
> +   */
>
> - Note that metacity always sets the border to 0 in order to make
>   this issue go away :-P though I was also trying to write the code
>   correctly assuming their was a border at least for a while

Yes, I know.  It makes sense but it's yet another thing to track when
reading meta_window_move_resize_internal() and realizing that it
accepts three totally different kinds of input (more if you are
looking at head, though I think that's more a reflection of bugs than
an intention to actually have it take more than three different kinds
of input).

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

> - in window_new_with_attrs you are now using
>   window->size_hints.win_gravity for the initial resize,
>   not convinced that's right but reading further ;-)
>
> - similarly in apply_session_info
>
> - OK, reading further you have a big comment about not understanding
>   this, so I'm not sure I feel more confident ;-)
>   (I'm not saying I fully get it either...)

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.

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.

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

>   Maybe get some little test xlib programs going that use the
>   various gravity hints/attributes ... EWMH spells out what
>   "correct" behavior is on size_hints.win_gravity

You already have written such a test program (and then filed bug
109953 because metacity failed on that program).  I read over the EWMH
and ICCCM stuff relating to gravities and believe the branch complies.
 But it seems odd that you'd pick up on the gravity changes--all I
really did as far as gravity was concerned was to make metacity honor
it everywhere instead of occasionally ignoring it and hard-coding to
NorthWestGravity.  The huge comment you see (by the (2) case) in
meta_window_move_resize_internal() where I'm confused about how
adjust_for_gravity() works isn't from any changes I made but again
just documenting the existing code (which, in the case of
adjust_for_gravity(), luckily worked so I didn't have to figure it
out).

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

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

> maybe we can just
>   pass in the size hints gravity whenever we have a resize request
>   from the app, but I'm not sure that works or why I did it the
>   other way

It does work and that's what I do.  :)  Since I did this and correctly
handled the input arguments to meta_window_move_resize_internal
(meaning to handle old practice since I couldn't figure out how to
change it), I was able to nuke the following FIXME comment that
existed in meta_window_configure_request():

  /* FIXME passing the gravity on only_resize thing is kind of crack-rock.
   * Basically I now have several ways of handling gravity, and things
   * don't make too much sense. I think I am doing the math in a couple
   * places and could do it in only one function, and remove some of the
   * move_resize_internal arguments.
   */

> - +  /* FIXME: Uh, why not use meta_window_get_position() to be like all
> the
> +   * other code in Metacity?
> +   */
>
>   just fix this instead of adding comment?

Yeah, I'd occasionally write comments when I noticed something but was
busy trying to figure out some other change.  Slipped through the
cracks.  :)

> - +  /* Return value is ignored, but whatever... */
>   if this is a glib timeout, it isn't ignored, "return false"
>   means to remove the timeout iirc... ?

Looks like a copy-and-paste problem.  There was a different function I
had that wasn't a timeout for which I had at some point been ignoring
the return value.  I copy and pasted it to start that function and
didn't remove the comment.  Oops.

> - have you tested in reduced_resources mode?

Nope, I suck.  I will do so.

> - #if 0 around EnterNotify handler should probably just be
>   deleting the code instead

Right.  Another slip on forgetting the cleanup.

> - +   * In a format that I (Elijah) understand: The x & y are trying to
> +   * provide the position of the upper-left corner of the client
> +   * (i.e. inner) window, but may do so relative to where the upper
> +   * left of the frame is if there is one.  The width and height are
> +   * always of the client (i.e. inner) window.
>
>    heh ;-) it is not complicated, rect.x is always what
>    XGetWindowAttributes() would return which is relative to the parent
>    window. ;-) The idea is that this rect is a reliably indicator of
>    server state so we never round trip to get our window size/pos
>
>    (most WMs simplify this by always putting a frame window on a window
>    and just setting the frame to the same size as the window if
>    "undecorated", but I had the idea it would be cooler to just have no
>    frame on undecorated windows for reasons I don't recall)

Yeah, it was a comment I put there when I started a few months ago
because I really wasn't thinking in terms of X implementational
details.  I don't really need the comment at all anymore since I
figured out all those X implementational details and agree that it
makes sense with that background.  It may be useful for others who try
to patch Metacity, though.  However, I can nuke it if you like.

> - I increasingly feel that boolean function args should
>   almost always be flags or enums, so
>    meta_window_maximize (window, META_MAXIMIZE_HORIZONTAL |
>    META_MAXIMIZE_VERTICAL)
>   Then you can read the code without reference to the header file.

Makes sense, I can change that.

Thanks,
Elijah