Re: constraints_experiments is ready

[Havoc Pennington <hp redhat com>]

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.

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

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


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

- remember to drop README change on merge

- 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

- another thing to check is resizing with all the different gravities
  (we have a test program for that iirc)

- 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

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

- 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

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

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

- META_DIRECTION_UP/DOWN alternate names I find odd

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

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

- ConstraintPriority enum lacks spaces around "=" 

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

- In this comment I think you mean "border" not "frame" ?

+  /* 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

- 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

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

- in general I would say move_resize_internal() needs work. the
  big comment there seems to confuse more than enlighten ;-) 

  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

- 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 adjust_for_gravity() thing 
  was supposed to handle the size hints 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

- +  /* 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?

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

- have you tested in reduced_resources mode?

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

- +   * 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)

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

Havoc