Created attachment 12339 [details] [review]
Patch to extend libwnck with multi-head support

Thanks, I think this is a reasonable idea as panels are per-monitor.
I don't think it's necessary to make it an option right away; we could
try just doing it this way by default, and if that fails be even more
clever and do it this way if there's a tasklist on each monitor, 
and do it the old way if there's only a single tasklist. That way 
stuff just works.

A couple changes I would make to the patch:

 - "multihead" normally refers to multiple screens, not to xinerama. 
   the xinerama heads are "monitors" and the "real" heads are 
   "screens" (where "screen" means "separate root window"). 
   Anyhow I think an s/head/monitor/ would clarify things
   for future maintenance.

 - the Xinerama checks and usage are Linux-specific, you have to do 
   more complicated configure.in stuff, see GDK or metacity. However
   the easy course of action is to just use GDK 2.1.x for this, 
   i.e. gdk_screen_get_num_monitors() and so on. IMO that's the 
   way to go, then it will automatically work on Solaris etc.

Thanks again

I wanted to do it without an option but I couldn't figure out how to 
get the current monitor of the widget. Perhaps you have me a clue on 
this?

Sorry for the wrong name, the reason is the DualHead feature of 
Matrox - since then I call it head.

Thanks for the tip regarding the Linux-specificity, I'll try to 
implement it using GDK 2.1.

The simplest way to get the current monitor is probably to 
use gdk_screen_get_monitor_at_point() or whatever it's called.

My problem is how to get the point on the screen within libwnck, not 
how to get the right monitor for a defined point. Perhaps that's a 
trivial question but I'm new to GTK programming as I've stated 
before.

The hacky thing is gdk_window_get_origin(tasklist->window) probably,
though that requires the applet to be realized and in-place before 
it works obviously.

A nicer solution might be to have the panel tell applets somehow 
what monitor the panel is on, then have the applet pass the 
monitor number to libwnck. That would probably be more robust 
and less annoying to implement.

I've now changed the patch as proposed. It refers to monitors 
instead of heads and it doesn't rely anymore on Linux/Xinerama.

The monitor number has still to be passed as an argument, as I can't 
figure a way out of this. But in my gnome-panel patch (Bug 98700), I 
found a solution, how to recognize the active monitor. This 
information gets passed to libwnck.

Created attachment 14155 [details] [review]
Patch to provide multimonitor support in libwnck

Would be nice if someone could review my patch and if it's ok, 
commit it.

I've improved my patch. Now it's got an automatic monitor detection,
so there is no need for the gnome-panel patch anymore.
Also updated to apply cleanly on libwnck-2.3.1.

Created attachment 17166 [details] [review]
Patch against libwnck-2.3.1 w/automatic monitor detection

Havoc, me again ;) Would be nice to see a comment here, too.

*ping*

I've updated my patch. It now applies cleanly on libwnck-2.5.1 and cvs
head.

Would be nice to get some feedback.

Created attachment 22708 [details] [review]
Patch against libwnck-2.5.1

Are you leaking a region object here?

on_active_monitor = gdk_region_point_in (gdk_region_rectangle (
+                                 &tasklist->priv->monitor_geometry),
+                                 x + w / 2, y + h / 2);


Does the patch work properly if you drag the tasklist applet from one
monitor to another? it looks like monitor_num may not be updated in
that case.

Thanks

I've fixed the region leak; I didn't look at my old code well enough...

You were right that it didn't work properly when dragging the tasklist
applet from one monitor to another. I had only tested the dragging
with the test-tasklist application, there it worked because the test
application window does not skip the taskbar.

I've now implemented an additional check and now applet dragging works
properly. I hope the implementation is acceptable, I haven't got a
better idea.

Created attachment 22711 [details] [review]
Patch with leak fix and correct applet dragging behaviour

Re: the region code, why create a region at all now that I think about
it; that's somewhat expensive. Something like 

#define POINT_IN_RECT(xcoord, ycoord, rect) \
 ((xcoord) >= (rect).x &&                   \
  (xcoord) <  ((rect).x + (rect).width) &&  \
  (ycoord) >= (rect).y &&                   \
  (ycoord) <  ((rect).y + (rect).height))

Usage of gdk_screen_get_monitor_at_window() should be avoided if we
can, as it will be a round-trip network request to the X server. It
would be better to use cached positions of things on the client side.
gdk_window_get_position() uses the client-side position without a
round trip.

I'm not sure I understand why the tasklist being skip_tasklist
matters, what am I missing?

problem though, gdk_window_get_position doesn't return the position in
root window coordinates. so the round trip may be needed, or else some
relatively elaborate solution...

POINT_IN_RECT: Sure, will be more efficient. There is no
gdk_rectangle_point_in() but a gdk_region_point_in(), so I thought
that's the way to go. I've changed that.
Unfortunately it can't be used in wnck_tasklist_include_window. I had
to replace my buggy code there with gdk_screen_get_monitor_at_point()
(no round trip), the reason is that windows could get lost if their
center is not on any monitor.

I don't know a possibility to avoid gdk_screen_get_monitor_at_window()
but I have limited its use in wnck_tasklist_window_changed_geometry()
by an additional condition. But gdk_screen_get_monitor_at_window()
does only use a round trip to get the window coordinates and caches
the xinerama monitors, at least not more than one round trip per call.

tasklist and skip_tasklist: Let's have a look at the moment when the
tasklist changes monitor in two situations
- The simple case of test-tasklist (no relevant skip_tasklist): Before
the monitor change the test-tasklist itself is shown on the tasklist.
During the monitor change libwnck gets a geometry_changed signal with
win_task != NULL but !show (because test-tasklist is off-monitor), so
it will re-generate the task list.
- The case of gnome-panel (has skip_tasklist): Before the monitor
change the panel / tasklist itself is not shown on the tasklist.
During the monitor change libwnck gets a geometry_changed signal with
win_task == NULL and !show, so it _won't_ re-generate the task list.
But if we integrate my extra check, it will correctly re-generate the
task list.

Created attachment 22718 [details] [review]
Patch with fix for off-monitor window centers and _slightly_ enhanced speed

This looks like it should be looked at again for 2.7.x.

Would be nice to get this finally in (or at least a reason why not). The
supplied patch still applies as of libwnck 2.6.0.1.

2.7.x is now open. Perhaps now is the time to merge this patch?

Comment on attachment 22718 [details] [review]
Patch with fix for off-monitor window centers and _slightly_ enhanced speed

The latest patch looks good to me. Mark is it OK to commit with current freeze
rules?

I'm cc'ing Mark so that he will see Havoc's question in the last comment.

Its not really a feature, there's no impact on docs or translators and it looks
like a relatively safe patch. No problems with comitting it...

As this hasn't been committed before 2.8 even though you wanted to, could you
please commit it now, so this gets in at last for 2.9/2.10?

Jürg: Mark and Havoc both said it'd be okay and wouldn't break freezes.  Do you
have cvs commit access?  If not, I'll go ahead and commit it for you on the
stable branch so it'll be in Gnome 2.8.1.

Created attachment 31657 [details] [review]
Rediff against current cvs, added changelog entry

I don't have cvs commit access, so it'd be nice if you could commit the patch.
I've rediffed against current cvs and added a short changelog entry. Thx

committed.

This change appears to cause some problems, outlined in
http://bugs.debian.org/274890 .

Yes, same issue was pointed out in bug 154040.  Havoc, Mark: Should we revert
until Jurg can provide an updated patch that fixes all cases?  

No, I don't think its a huge issue - one that I'd like to see fixed, but there's
no real need to revert.

This change is really annoying when using twinview on nvidia cards. Please
create an option so you can switch back to the old behaviour.

What mode are you using twinview in? Just like regular xinerama, or some sort of
zoom/clone/app-exclusive/etc. setup?

An option isn't as good as just detecting the active setup and doing the right
thing (for example basing behavior on number of tasklist applets active)

I'm using plain Nvidia-Twinview with XineramaInfo.
I.e.:

    Option "TwinView"
    Option "ConnectedMonitor" "CRT, CRT"
    Option "TwinViewOrientation" "RightOf"
    Option "SecondMonitorHorizSync" "30-96"
    Option "SecondMonitorVertRefresh" "50-160"
    Option "MetaModes" "1400x1050, 1400x1050"

That's it.

The right thing(tm) is to be consisten in the first place.
As I already mentioned in Bug 154040, Window List is the
only applet which distinguishes monitors of the same screen.
This simply doesn't feels right. I can't set different backgrounds
on different monitors but I've to run two panels to see all
windows?
If you don't want to present additional options, then you should
revert this patch.

For a while now I've been noticing that sometime windows seem to disappear from
my taskbar applet.  It wasn't until a few moments ago I realized it was now only
showing me the windows from one of the xinerama screens.

Frankly, I can't imagine why I would want that.  Now I'd have to add a panel on
the other xinerama just so I can have those windows show up.

I could almost imagine this being a preference, which is perhaps smart enough to
only appear if you actually have a real xinerama setup, but really, it seems to
me this should simply be reverted.

There's a patch in bug 154041 to fix the "slowly disappearing" windows aspect. 
That is supposed to make it so that the system immediately only shows windows
from one of the xinerama screens.

I don't use Xinerama myself (though I really wish I had the money for more
monitors so I could use it...), but I have to agree with Rob.  If the default is
to only have one panel (as I've been led to believe from what I've read/heard),
why do we keep a patch that breaks that default?  Users are unlikely to realize
that they have to add another panel on the other screen, but if they don't do so
then they can't make use of them other than manually.  In constrast, reverting
this patch provides no more problems than having windows listed once on each
monitor.

(See also bug 154040)