This is sepseically problematic when I use eveolution and select 'add
sender to addressbook' in evolution. A small popup with only name and
email address of the sender is then placed almost completely under the
panel. 

Batch adding GNOME2 keyword to Metacity bugs.  Sorry for the spam.

I can't seem to reproduce this bug with Metacity from the latest
Ximian snapshot (metacity-2.4.2.0.200210242020-0.snap.ximian.1),
although I may be doing something wrong.

The problem here is that the window placement algorithm isn't aware of
struts on the display -- it uses the raw xinerama size of the display
for placement calculation.

I'd like to fix this bug and improve the window placement at the same
time, but I need to know how to find the "effective" height of an
xinerama screen that takes into account the struts.  You obviously
must already calculate this somewhere so tell me how to do it and I'll
go ahead and fix this bug.

This was easier to fix than I thought.  Didn't even need to worry
about struts.

Patch attached.

-Rob

Created attachment 13449 [details] [review]
Avoid META_WINDOW_DOCK as well

That's not really the right fix though, or we wouldn't bother having
struts.

Basically instead of a single workspace->work_area variable, 
we need workspace->work_areas[N_XINERAMA_SCREENS]
I think.

OK this is a big patch, and fixes this bug, as well as #86682, as well
as hopefully pretty much every other issue with placement, expecially
as it relates to xinerama.

This patch implements a per-xinerama work area on each workspace, and
properly deals with struts whose corresponding windows appear on only
one xinerama or any subset of the xineramas.  It should also properly
handle struts that appear on multiple xineramas, or a subset of
workspaces, etc.  It does the best it can with panels placed on the
edge of the screen, and indeed these work well except that the
nautilus desktop seems to respond poorly to them.  Not sure if this is
a nautilus issue or a WM issue though.

Windows will now maximize full-screen without the "dead zone" and
move/resize constraints have been updated to deal properly with the
partial-width struts as well, so windows can be moved so the title bar
will go into what was previously a dead zone.

This patch also includes the "visually centering" first-fit algorithm
described in #91481 since that patch would conflict with this one, and
I needed to update the placement algorithm to use the per-xinerama
work areas.

One question though: it appears as though constraints.* is completely
unused?  What is this code there for?  I tried to update it to use the
new code, but since it isn't actually used, I don't know how sucessful
I was.

-Rob

Created attachment 13500 [details] [review]
per-xinerama work areas

sorry that patch is screwed up.  CVS diff is having difficulties
(gnome anonymous CVS really sucks to a surprising extent).

I'll let you know when I get a good one up.

-Rob

Created attachment 13501 [details] [review]
hopefully correct version of the patch

One thing that I forgot to mention:

The patch above also modifies the placement algorithm to ignore dock
windows for the purposes of putting other windows to the right or
below.  This makes it so that a top or left panel doesn't interfere
with the placement algorithm.

By the way, when I said "It does the best it can with panels placed on
the edge of the screen, and indeed these work well except that the
nautilus desktop seems to respond poorly to them," I meant panels that
are an the edge between two monitors.

constraints.[hc] is a rewrite of the constraints code, 
it isn't used yet and is safe to ignore. I have a largish outstanding
patch to it anyhow.

I'll have to look at this patch in detail after we branch for 2.2. 
It probably overlaps with the "saving window positions" patch at least
a little bit, also.

So, no possibility of seeing this in 2.2?  This is a pretty big
upgrade for metacity, and it would be nice if my biggest metacity
gripe (bug 86682, an oft-reported bug) could be fixed for 2.2.

The patch is just too big for 2.2 IMO. We already shipped a release
candidate.

OK.  It's probably better to give it more time to be tested before
release.

Regardless, here's another patch that fixes a minor bug I discovered
and removes some extra debugging output from the logfile that I forgot
to remove.

Created attachment 13513 [details] [review]
per-xinerama workareas minor bugfix

This version of the patch (sorry for the patch proliferation) fixes
another bug and cleans up the code a bit.

I've got an idea of how to make panels on internal edges work well
with the window manager with this patch, which I don't think would be
that hard to implement.  But I was under the impression that by Gnome
2.2 panels wouldn't be allowed on internal edges, so I'm not sure if I
want to bother with that.  Do you think that it would be worth it to
try to deal with "internal struts"?

Basically the idea is that if the size of a strut area is larger than
the size of the xinerama whose edge is being used to compute the strut
distance, that the strut would be ignored for the purposes of
computing the overall screen work area, as distinct from the
individual xinerama work areas, which would use that strut.  An
example:  if you have two 1280x1024 displays side by side, with a
strut on the left side of the right monitor, that strut would set
left_strut to something like 1332.  Since this is greater than 1280,
the overall screen work area would be 0,0 2560x1024 and the xinerama
workareas would be 0,0 1280x1024 and 1332,0 1228x1024.  Of course this
is a horrible ugly kludge, but I'm fairly confident it would work.

-Rob

Created attachment 13517 [details] [review]
another bug fix, some cleanups to per-xinerama work areas

Created attachment 13769 [details] [review]
update patch for current CVS head

have we officially branched for 2.2 yet?  I notice that a number of
distributions seems to have official 2.2 packages available, but I
haven't seen a 2.2 announcement yet.  I assume this means that some
programs have branched and others haven't.

Could we start to look at this patch?

There is a HEAD branch, but I don't have time to look at the patch
probably for the next couple weeks. Be sure all the patch bugs are
marked priority=High and PATCH keyword, and I'll get to them as soon
as I can. 

ping

Soon ;-) too many Red Hat Linux bugs.

Created attachment 14522 [details] [review]
Updating patch for current CVS head

Is there a change where the patch modifies the placement of transient 
children, or is that just a whitespace glitch?

I mean the hunk starting with:

       if (parent)
-        {
-          int w;
-
-          meta_window_get_position (parent, &x, &y);

I would like to break up the function get_work_area_for_xinerama 
into get_work_area_for_current_xinerama(),
get_work_area_for_xinerama(), and get_work_area_for_all_xineramas(), 
I think that would make the code a lot more readable. 

The "fluff" placement patch sounds sensible to me, and the xinerama
change here makes sense. I still need to go through the details of the 
patch but it looks good in big picture.

That must be a whitespace glitch -- the section that it's replaced
with looks identical at least to me.   I must have reindented that
section of code at some point for some reason, since some spaces seem
to have turned into tabs.  And I certainly didn't intend to do
anything to transient placement, since so far as I know that's related
only to the parent position and not to work areas.  I think you can
ignore that hunk, if you like.

And I think that you're right about breaking up the functions a bit. 
I'll go ahead and do that and submit a new patch.

Created attachment 14533 [details] [review]
code cleanup -- simplify function semantics for meta_window_get_work_area*

I'm merging this patch now, making nitpick changes as I go.

What is the problem solved by:
 +      /* don't try to place on the edge of a dock */
+      if (META_WINDOW_DOCK != w->type)
+       {

?

The problem here is panels on the top or left of the screen look good
edges to place along otherwise, and prevent the fluff from being
applied, since the placement algorithm sees an edge and goes with it.

Wasn't a problem before since windows were always put at 0,0 anyway.

Created attachment 14540 [details] [review]
current version

Attached patch with lots of nitpicks changed. I also dropped the 
part from move_resize_internal() because at first I didn't realize
this patch also covered #86682; I need to put that back in some form.

As noted in a comment, I'm not sure using the all_xineramas work area
is ever right; I may purge that. I want to do a bit more hacking 
on the patch before commit.

Some nitpicks for future reference:
 
- space between operators and args "a * b" not "a*b"

- spaces before parens, foo () not foo()

- multiline comments have a * on every line

- no "gint", just "int"
 
- function args should line up neatly (google 
  for egtk-format-protos if you use emacs)

- try to include only smallest possible change per patch, 
  e.g. here splitting placements changes from #86682 would 
  make it a lot easier to review the patch

- try not to make formatting/whitespace changes in the same 
  patch as functional changes

- spaces not tabs

- if body of a conditional is one line, no braces:

    if (foo)
      bar ();

Anyway, I'll finish looking at the more substantive details
tomorrow hopefully and commit.

old habits die hard :-)

This change was what was needed for a number of problems, and I tried
to fix as many as was convenient without adding too much code.  Sorry
to make it harder for you.

At least I showed some restraint on 93596 and 105270 :-)

The move_resize_internal stuff handles the case where a panel is moved
from one xinerama to another without any change in struts -- this can
be accomplished by dragging if you're careful or in the properties
dialog of the panel.

Created attachment 14547 [details] [review]
Checking in

OK, committed. I disabled the fix for bug #86682 for now while keeping 
most of the code (since having workspace->work_areas is clearly right, 
but I'm pretty unconvinced that we know the right way to compute 
them). Let's continue working on a fix for that on the other bug
and wm-spec-list.

Thanks! Let me know if I totally broke it making changes. 
i.e. it might be nice to review the final patch.