Closed Bug 156333 Opened 22 years ago Closed 1 year ago

GetZOrderDOMWindowEnumerator is broken on Linux (new message count not updated when multiple mail windows are present)

Categories

(Core :: Widget: Gtk, defect, P4)

All
Linux
defect

Tracking

()

RESOLVED INACTIVE

People

(Reporter: colin, Unassigned)

References

()

Details

(Keywords: helpwanted, platform-parity, Whiteboard: tpi:+)

Attachments

(4 files, 1 obsolete file)

This is with Mozilla 1.0 and 1.1 Alpha on OpenVMS. Using POP3.

Biff works just fine so long as I only have one mail window open. But when I
open a second window, the count of the number of messages in my Inbox never gets
updated, even when I know there is new mail waiting to be fetched. If I close
the second window, then the count will (after a short while) get updated. If I
manually click on "Get New Messages" the count gets updated.

Using the debugger I can see that even with two mail windows, the biff code is
still running, but with POP3 protocol tracing enabled, there's no activity. 

Stepping through the nsPop3IncomingServer::PerformBiff() code its the call to
nsMsgMailSession::GetTopmostMsgWindow that's failing.

In GetTopmostMsgWindow we get the count of the number of windows (2) and then
drop into the code which will try to find the top most window.
GetZOrderDOMWindowEnumerator appears to succeed (rv=0) but the first call to
HasMoreElement fails (because mCurrentPosition is zero). This is why
GetTopmostMsgWindow returns with NS_ERROR_FAILURE (which is then silently ignored).

Anyone any ideas on what else I should be looking at here?
Great news (for me). I can reproduce this on RedHat.

The easiest way to see the problem is:

- export NSPR_LOG_MODULES=POP3
- start mozilla
- go into mail/news
- set auto-fetch on a POP3 account to 1 minute
- observe the POP3 messages on the console every 60 seconds
- create a second mail window
- notice that the POP3 messages on the console stop
- close the second window and they'll come back again
OS: OpenVMS → Linux
I can't believe no one else is annoyed/affected by this bug. Its easy to
double-click on the inbox and accidently leave that window open (usually hidden
behind some other window), and then you don't receive any more mail. Eventually
you realise because its TOO quiet, you find the window, close it, and boom,
you've suddenly got 20 new mails.
Product: Browser → Seamonkey
The bug in getZOrderDOMWindowEnumerator affects me too. Sadness.
Assignee: mscott → mail
QA Contact: stephend
This bug is misassigned, right?

/be
Assignee: mail → adamlock
Component: MailNews: Notification → Embedding: APIs
Product: Mozilla Application Suite → Core
Hardware: DEC → All
This is more likely to be a widgetry issue...  Colin, what widget set is in use here?  GTK1 or GTK2?
GTK1.
Assignee: adamlock → nobody
Component: Embedding: APIs → Widget: Gtk
Keywords: helpwanted
QA Contact: gtk
Gtk1/xlib widget code has been removed on trunk.

Does this bug also occur in a recent gtk2 trunk build?
http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-trunk/
Whiteboard: [gtk1-only?]
As far as this bug is about getZOrderDOMWindowEnumerator not working with GTK, getZOrderDOMWindowEnumerator still doesn't work with the latest gtk2 trunk build.  
If this problem only occurs when you open a second mail window and you don't want  this second window, you could probably use the No New Window on Double Click extension.
I am not sure if it works out of the box for mozilla or seamonkey, but it works like a charm for thunderbird:
https://addons.mozilla.org/en-US/thunderbird/addon/150
Whiteboard: [gtk1-only?]
This patch makes the zOrderEnumerators work on Linux. It uses the same method as is used for cocoa windows on Mac (see http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsCocoaWindow.mm SendSetZLevelEvent method). Whenever a window gets focussed, it sends a SetZLevel event, so the windows zlevel gets registered by a nsIWindowMediator. 

There is not a native gtkwidget method to get the z-Index of a window, so I think this must be the way to go.

Since according to lxr DipatchGotFocusEvent is not used by any other widget, and this patch calls DispatchGotFocusEvent and DispatchSetZLevelEvent at the same time, maybe it is better if those two functions would merge?
Attachment #280020 - Flags: superreview?
Attachment #280020 - Flags: review?
Attachment #280020 - Flags: superreview?(roc)
Attachment #280020 - Flags: superreview?
Attachment #280020 - Flags: review?(roc)
Attachment #280020 - Flags: review?
Cleaner patch: there is no need to set refPoints to the event.
Attachment #280020 - Attachment is obsolete: true
Attachment #280058 - Flags: superreview?(roc)
Attachment #280058 - Flags: review?(roc)
Attachment #280020 - Flags: superreview?(roc)
Attachment #280020 - Flags: review?(roc)
doesn't this break things for X users who use focus-follows-mouse and no autoraise?
(In reply to comment #12)
> doesn't this break things for X users who use focus-follows-mouse and no
> autoraise?
> 
Considering nsIWindowMediator::GetZOrder(DOM|XUL)WindowEnumerator does not work at all, in this case there is nothing to break really ;).

But you're right, I didn't think of that specific case. But then again: currently getMostRecentWindow does also not return the topmost window when the mouse is (or was last) over a lower-stacked window, while the spec says: "This is a shortcut for simply fetching the first window in front to back order." (it works with a timestamp for focusing).

I couldn't find any reference for getting the topmost window in the GTK spec, therefor I think this is the way to go, to fix z-ordering on Linux.

The other way to fix this bug is to rewrite nsMsgMailSession::GetTopmostMsgWindow,  but that won't prevent/fix problems with other calls to the z-order enumerators.

 
CCing my favourite Linux/X bug buddies. Michael, Karl, what do you think here?
I was just about to post that I wouldn't have a clue either, but then after a bit of searching I found this; maybe it will help?

http://library.gnome.org/devel/gdk/unstable/GdkScreen.html#gdk-screen-get-window-stack

If the current GdkScreen is stored somewhere, we could pass that into the function and get the stack of windows in Z-order, at least according to the documentation and some searching. However, this is a fairly recent API so we should guard against "undefined symbol", and if the window manager doesn't support this it will return null.

I'll have a better look later when I get home.
I don't think I'll be able to make that API work properly for our situation, since it returns every window on the desktop and I can't think of a way to distinguish Mozilla windows. I think Teune's patch is the best way to go for now.
(In reply to comment #13)
> (In reply to comment #12)
> > doesn't this break things for X users who use focus-follows-mouse and no
> > autoraise?
> > 
> [...]
> But then again:
> currently getMostRecentWindow does also not return the topmost window when the
> mouse is (or was last) over a lower-stacked window, while the spec says: "This
> is a shortcut for simply fetching the first window in front to back order."

I can see that the documentation in nsIWindowMediator.idl doesn't seem to have
been written with focus-follows-mouse behaviour in mind.

What I think we want here though is the window most recently "selected" by the
user.

The problem with using focus events is that the user is likely to end up
selecting other mostly hidden windows when the pointer is dragged across
their edge.

> (it works with a timestamp for focusing).

I don't know exactly when nsWindowMediator::UpdateWindowTimeStamp is called,
but a method based on this would be better if the timestamp were only updated
on input, window-creation, and window-raise events (and maybe a few others).

The window-manager's stack is probably going to give us what the user would
expect much of the time.

It would also be nice to prefer unminimized windows on _NET_SHOWING_DESKTOP, but I'm not sure that fits directly into nsWindowMediator.
(In reply to comment #15)
http://library.gnome.org/devel/gdk/unstable/GdkScreen.html#gdk-screen-get-window-stack
> 
> If the current GdkScreen is stored somewhere,

I think we normally just use something like gdk_screen_get_default, which should give the screen specified by the -display arg or DISPLAY env var.
(Can we have windows on more than one screen from one process?)

> However, this is a fairly recent API so we
> should guard against "undefined symbol".

We now require gtk+-2.10, so we can link against this symbol.
(http://wiki.mozilla.org/Linux/Runtime_Requirements)
(In reply to comment #16)
> it returns every window on the desktop and I can't think of a way to
> distinguish Mozilla windows.

gtk_window_list_toplevels or gdk_screen_get_toplevel_windows should give a list of windows in the same process (and is probably easier than using WM_CLASS and _NET_WM_PID properties).

What is likely better though is to use the CirculateNotify event from X.
(See man XCirculateEvent.)  It can give us PlaceOnTop or PlaceOnBottom notifications.  If we detect this event we can use it to change the Z order or change the timestamp or both.

gdk doesn't provide us with this event, so I guess we'd need gdk_window_add_filter to collect the event before gdk drops it.
> We now require gtk+-2.10

"We" == "mozilla.org binaries".  It's not the case that the source should unconditionally require them.  See the part about compile-time switches on the page you cite.
(In reply to comment #19)
> What is likely better though is to use the CirculateNotify event from X.

Actually, this would need to be combined with ConfigureNotify events and probably some others, and interpreting the "above" field of the XConfigureEvent (not present in GdkEventConfigure) would be difficult.

gdk-screen-get-window-stack is probably going to be easier (if available at compile time).
(In reply to comment #16)
> it returns every window on the desktop and I can't think of a way to
> distinguish Mozilla windows.
get_window_for_gdk_window() or get_owning_window_for_gdk_window() seem to do the trick.

However, when I tested gdk_screen_get_window_stack(), everytime it returned every window except for the two topmost windows on the screen. 

There is another problem with gdk_screen_get_window_stack(): You can call it when a window gets focussed, but when the focussed window is not top stacked, you have to be notified when the user after focussing the window raises the window. In that case you are back where you were: there is a need for a notification when a window gets raised.

Apparently, the CirculateNotify event is only sent when an application calls XCirculateSubwindows(), XCirculateSubwindowsUp(), or XCirculateSubwindowsDown(): http://tronche.com/gui/x/xlib/events/window-state-change/circulate.html, so this    seems not to be an option.
(In reply to comment #22)

Thanks for trying these out.  Sorry they weren't helpful.
A few more ideas that may (or may not) be helpful...

> However, when I tested gdk_screen_get_window_stack(), everytime it returned
> every window except for the two topmost windows on the screen. 

That's inconvenient!
Does "xprop -root | grep \^_NET_CLIENT_LIST_STACKING" give all the windows?
(You can also check it changes appropriately with "xprop -spy -root".)

> 
> There is another problem with gdk_screen_get_window_stack(): You can call it
> when a window gets focussed, but when the focussed window is not top stacked,
> you have to be notified when the user after focussing the window raises the
> window. In that case you are back where you were: there is a need for a
> notification when a window gets raised.

XSelectInput with PropertyChangeMask on the root window is tempting but I don't think we want to wake up Mozilla every time anything else changes.

> Apparently, the CirculateNotify event is only sent when an application calls
> XCirculateSubwindows(), XCirculateSubwindowsUp(), or
> XCirculateSubwindowsDown():
> http://tronche.com/gui/x/xlib/events/window-state-change/circulate.html, so
> this    seems not to be an option.

Not the whole answer at least anyway.
Most cases are apparently meant to be handled by ConfigureNotify events:

http://tronche.com/gui/x/xlib/events/window-state-change/configure.html

However, I'm only seeing these events on raise not lower.  Raise events may be enough for us, but the lack of lower events makes me wonder whether we can rely on raise events.

I'm not sure whether gtk registers for events on the frame window or whether we'd have to do that.

% xwininfo -children -frame

xwininfo: Please select the window about which you
          would like information by clicking the
          mouse in that window.

xwininfo: Window id: 0x127fb5e (has no name)

  Root window id: 0x3d (the root window) (has no name)
  Parent window id: 0x3d (the root window) (has no name)
     2 children:
     0x127fb5f (has no name): ()  1123x724+4+25  +153+25
     0x127fb6a "kwin": ("kwin" "Kwin")  1131x753+0+0  +149+0

% xev -id 0x127fb5e

...

ConfigureNotify event, serial 15, synthetic NO, window 0x127fb5e,
    event 0x127fb5e, window 0x127fb5e, (149,0), width 1131, height 753,
    border_width 0, above 0x120003d, override NO

VisibilityNotify event, serial 15, synthetic NO, window 0x127fb5e,
    state VisibilityUnobscured

VisibilityNotify event, serial 15, synthetic NO, window 0x127fb5e,
    state VisibilityPartiallyObscured
I wonder whether a most-to-least-recent iterator corresponding to getMostRecentWindow is what is really wanted, but just needs UpdateWindowTimeStamp called at the right time?

I don't feel comfortable sending NS_SETZLEVEL events.  nsGUIEvent.h says these are "top-level window z-level change request" events, which doesn't seem to be what we want to do.

_NET_WM_USER_TIME is almost the right kind of timestamp but isn't updated (by gdk) on raise.
(In reply to comment #24)
> I wonder whether a most-to-least-recent iterator corresponding to
> getMostRecentWindow is what is really wanted, but just needs
> UpdateWindowTimeStamp called at the right time?
> 
> I don't feel comfortable sending NS_SETZLEVEL events.  nsGUIEvent.h says these
> are "top-level window z-level change request" events, which doesn't seem to be
> what we want to do.

AFAIK, this event is used on MS Windows for the same purpose [1][2][3], but in that case it also handles lower z-levels. So, if I am right, this is the only way a window is registered to the windowMediators zLevel enumerators, currently.

> Does "xprop -root | grep \^_NET_CLIENT_LIST_STACKING" give all the windows?
> (You can also check it changes appropriately with "xprop -spy -root".)

This does list every window, including the panels on the top and bottom of my gnome desktop. So actually the 4 topmost windows are missing from gdk_screen_get_window_stack(), but I guess the panels are correctly filtered away.

> However, I'm only seeing these events on raise not lower.  Raise events may be
> enough for us, but the lack of lower events makes me wonder whether we can rely
> on raise events.

If I add a the filter for those events to gtk nsWindow.cpp like this:
  gdk_window_add_filter(NULL, notify_zlevel_filter_func, this);
I never get a useful event, when with focus-follows-mouse after focusing a lower stacked window, I raise the window by clicking on it. Maybe only the expose event and the clicking event self.

1] http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#4651
[2] http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#3553
[3] http://msdn2.microsoft.com/en-us/library/ms632653.aspx
(In reply to comment #25)
> (In reply to comment #24)
> > I don't feel comfortable sending NS_SETZLEVEL events.  nsGUIEvent.h says
> > these are "top-level window z-level change request" events, which doesn't 
> > seem to be what we want to do.
> 
> AFAIK, this event is used on MS Windows for the same purpose [1][2][3], but in
> that case it also handles lower z-levels.

It serves a similar purpose but also serves a different purpose.

On MS Windows, the application does its own window management but under X11, the window manager does this.

The references you give are for WM_WINDOWPOSCHANGING which corresponds to the ConfigureRequest received by the X11 window manager.  On X11 the application (frame) only receives the ConfigureNotify, which corresponds to WM_WINDOWPOSCHANGED.

WM_WINDOWPOSCHANGING ConfigureRequest and NS_SETZLEVEL are currently requests that may be modified, and may be interpreted to move other windows also.  This should not happen with ConfigureNotify events.

> So, if I am right, this is the only way a window is registered to the
> windowMediators zLevel enumerators, currently.

Yes, _currently_ it is.

A separate NS_ZLEVELCHANGED event would be nice for doing the mediator->SetZPosition separately on WM_WINDOWPOSCHANGED or ConfigureNotify, but maybe the existing method can be hijacked if careful and things are documented well (and provided it is a zlevel change not a focus change).

The main issue is that nsXULWindow::ConstrainToZLevel moves other windows around even if aImmediate is not set.  This may be appropriate behaviour on MS Windows but on X11 I don't think we should be doing this in response to ConfigureNotify. (The window manager does this sort of thing.)

Currently we are fortunate in that nsWindow::PlaceBehind just returns NS_ERROR_NOT_IMPLEMENTED with gtk, but, if this were to be implemented in the future, then the danger is that we end up fighting the window manager in a endless loop.

Maybe we can ifdef this out for MOZ_X11:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/appshell/src/nsXULWindow.cpp&rev=1.176&mark=1873-1882#1870
(In reply to comment #25)
> > Does "xprop -root | grep \^_NET_CLIENT_LIST_STACKING" give all the windows?
> > (You can also check it changes appropriately with "xprop -spy -root".)
> 
> This does list every window, including the panels on the top and bottom of my
> gnome desktop.

Great.  So it would be possible to get these properties ourselves rather than through gdk.

> So actually the 4 topmost windows are missing from
> gdk_screen_get_window_stack(), but I guess the panels are correctly filtered
> away.

Maybe gdk's filtering is doing too much ????
> Does "xprop -root | grep \^_NET_CLIENT_LIST_STACKING" give all the windows?

For what it's worth, over here it gives absolutely nothing.  But then again, I'm not running either GNOME or KDE, if that matters.
(In reply to comment #28)
> > Does "xprop -root | grep \^_NET_CLIENT_LIST_STACKING" give all the windows?
> 
> For what it's worth, over here it gives absolutely nothing.  But then again,
> I'm not running either GNOME or KDE, if that matters.

We don't need to make this work well for people running AfterStep version 1 ;), but we should handle the missing property reasonably nicely.

(gdk_window_cache_filter in gtk+-2.10.14/gdk/x11/gdkdnd-x11.c maintains a list of children of the root window but I don't think we want to take that approach.)
(In reply to comment #25)
> (In reply to comment #23)
> > However, I'm only seeing these events on raise not lower.
> > Raise events may be enough for us, but the lack of lower events makes me
> > wonder whether we can rely on raise events.

I now see that on lower, other windows are getting raise events.  (The window
manager is raising them rather than lowering the window I ask it to lower.)
So we can get enough notifications here.

> 
> If I add a the filter for those events to gtk nsWindow.cpp like this:
>   gdk_window_add_filter(NULL, notify_zlevel_filter_func, this);
> I never get a useful event, when with focus-follows-mouse after focusing a
> lower stacked window, I raise the window by clicking on it. Maybe only the
> expose event and the clicking event self.

I assume you get the events with xev on the frame window?

I guess the frame window really belongs to the window manager so I'm not
surprised that gtk doesn't register for events on the frame window.  We'd have
to do that ourselves.

XSelectInput with StructureNotifyMask or similar use of gdk_window_set_events
can register for the events, but getting the frame window is a little more
difficult.

gdk_window_get_frame_extents in gtk+-2.10.14/gdk/x11/gdkwindow-x11.c has implementation for getting the frame window (but we can't copy it).

Things may get complicated though as "a window does not have a frame until the
window manager decorates it":

  http://doc.trolltech.com/4.3/geometry.html

ReparentNotify events may be useful for detecting when the frame window changes.
(It may be worth checking out QWidget::frameGeometry too.)

(In reply to comment #26)
> A separate NS_ZLEVELCHANGED event would be nice for doing the
> mediator->SetZPosition separately on WM_WINDOWPOSCHANGED or ConfigureNotify,
> but maybe the existing method can be hijacked if careful and things are
> documented well (and provided it is a zlevel change not a focus change).
> (...)
> Currently we are fortunate in that nsWindow::PlaceBehind just returns
> NS_ERROR_NOT_IMPLEMENTED with gtk, but, if this were to be implemented in the
> future, then the danger is that we end up fighting the window manager in a
> endless loop.
 
Maybe you didn't see this, or maybe is that not your point, but as long as mediator->CalculateZPosition never succeeds because mActualBelow on the NS_SETZLEVEL event is not set, the part you want to ifdef out never gets called.

This also means that, if I am correct, mediator->GetZLevel is the only thing needed to register the window at the zlevel enumerators? And that the last window which calls that method will be registered as being on top? 

(In reply to comment #30)
> I assume you get the events with xev on the frame window?
ah, yes
(In reply to comment #30)
> ReparentNotify events may be useful for detecting when the frame window
> changes.

xevent->xreparent.parent will give you the frame actually. 

> XSelectInput with StructureNotifyMask or similar use of gdk_window_set_events
> can register for the events, but getting the frame window is a little more
> difficult.

this seems to work:

if (xevent->type == ReparentNotify) {
   GdkWindow *window = gdk_window_foreign_new(xevent->xreparent.parent);
   gdk_window_set_events(window, GDK_STRUCTURE_MASK);
   gdk_window_add_filter(window, aGdkFilterFunc, (nsWindow *)data);
}

   

This patch is how far I came with using ConfigureNotify Raise events. (With ConfigureNotify Raise events, I mean ConfigureNotify events, with the above property set)
The patch is by far not material which is ready for check in, but it is only here to see for others what code is causing the problems listed below.

There are a few problems when listening to the ConfigureNotify Raise event:

When there are for example three windows open and you close one window, the ConfigureNotify event is sent for the topmost window first and then for the lower stacked window. This means that when _NET_CLIENT_LIST_STACKING is not available, you can not register the windows in the correct stacking order.

When there is a popup on a window when you close the window like "Are you sure you want to close all tabs", and you press OK, a ConfigureNotify Raise event is sent for the closing window because the popup just closed. If you then want to access properties of that window, the application crashes because the window has been destroyed. 

Note also that when you move a window around, a lot of ConfigureNotify Raise events are fired (it may be slowing down the dragging). 

Maybe there is a way to check if a window has been destroyed from a pointer to that window, without trying to access the window itself. But especially the first problem mentioned above makes it as far as I'm concerned not worth using ConfigureNotify to register the z-level of windows.
Summary: new message count not updated when multiple mail windows are present → GetZOrderDOMWindowEnumerator is broken on Linux (new message count not updated when multiple mail windows are present)
(In reply to comment #11)
> Created an attachment (id=280058) [details]
> register/update zIndex on window focussing

I too am running in to the problem of getZOrderXULWindowEnumerator not working.  I tried out this initial simple patch, and it works fine (I don't use focus-follows-mouse).

Is there a reason why this patch was never reviewed?  Was it because of the focus-follows-mouse issue roc brought up in comment 12?  Like Teune pointed out in comment 13, the whole thing was broken and with the patch now just part of it is broken for some users (i.e. no worse than it was previously).

I would like to see a fix for this make its way in to the trunk, but only if it's not going to cause more problems.  I've not done any gtk programming, so I'm not qualified to know whether the patch is acceptable or not.
Karl, do you think we should go with the earlier patch?
(In reply to comment #31)
> (In reply to comment #26)
> > A separate NS_ZLEVELCHANGED event would be nice for doing the
> > mediator->SetZPosition separately on WM_WINDOWPOSCHANGED or ConfigureNotify,
> > but maybe the existing method can be hijacked if careful and things are
> > documented well (and provided it is a zlevel change not a focus change).
> > (...)
> > Currently we are fortunate in that nsWindow::PlaceBehind just returns
> > NS_ERROR_NOT_IMPLEMENTED with gtk, but, if this were to be implemented in the
> > future, then the danger is that we end up fighting the window manager in a
> > endless loop.
> 
> Maybe you didn't see this, or maybe is that not your point, but as long as
> mediator->CalculateZPosition never succeeds because mActualBelow on the
> NS_SETZLEVEL event is not set, the part you want to ifdef out never gets
> called.

I can't see why mediator->CalculateZPosition should never succeed.
nsIWidget **outBelow is the address of an output parameter and I would expect to be set to &zEvent->mActualBelow.

If it didn't succeed, then mediator->SetZPosition would not be called.

> This also means that, if I am correct, mediator->GetZLevel is the only thing
> needed to register the window at the zlevel enumerators?

If GetZLevel records change in position in the enumerators, then something is wrong.
(In reply to comment #35)
> Karl, do you think we should go with the earlier patch?

No.  Mostly because it is sending a NS_SETZLEVEL event requesting a change in window zlevel, when what we want is an event informing that the window zlevel has already changed.  (Comment 26)

Also the event is being sent when focus is changed, not when the zlevel is changed.

That's too much confusion for me.

For a simple way to improve the current behavior, I'd suggest investigating why GetZOrderDOMWindowEnumerator() succeeds but HasMoreElements() fails. (Comment 0)

If GetZOrderDOMWindowEnumerator() failed then nsMsgMailSession::GetTopmostMsgWindow could fall back to a different enumerator.
Or GetZOrderDOMWindowEnumerator() could still succeed so long as HasMoreElements() still iterates through all the windows (even if the order would not be correct).
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P4
Blocks: 462222
Attached patch PatchSplinter Review
This seems to work well by using the GDK API for getting the window stack, and setting the window position based on that instead of just raising to the top every time. I tried to take an approach that would ensure the order was kept up-to-date but didn't have to needlessly update the list.
Attachment #356888 - Flags: superreview?(roc)
Attachment #356888 - Flags: review?(roc)
This doesn't seem to address Karl's issues in comment #37.
Comment on attachment 280058 [details] [diff] [review]
register/update zIndex on window focussing

minusing due to Karl's comments
Attachment #280058 - Flags: superreview?(roc)
Attachment #280058 - Flags: superreview-
Attachment #280058 - Flags: review?(roc)
Attachment #280058 - Flags: review-
Comment on attachment 356888 [details] [diff] [review]
Patch

minusing, see comment #39
Attachment #356888 - Flags: superreview?(roc)
Attachment #356888 - Flags: superreview-
Attachment #356888 - Flags: review?(roc)
Attachment #356888 - Flags: review-
I'm going to try and work on this bug again.

Reading through the relevant code, it seems that ZLEVEL events do in fact provide a simple notify, provided the event is set as non-immediate.

If a different window's Z level is set later (nsWindowMediator::SetZLevel), then the linked list is not only resorted followed by calls to PlaceBehind on all windows. But I suppose that's expected behaviour and is what is considered the "non-immediate" path I guess.

The reason I chose focus events is because they are sent far less often than configure events, yet I cannot think of a common way window order changes other than focus.
I suppose what I can do is cache the "above" member of the configure event, and only request the window stack and sent events if that member changes.
The event is set to non-immediate by default. So the event in the old patch was never requesting a change in window Z level at all.

I guess this was why I was confused by your comment all the way back then, karl. Because this is precisely the approach that windows and mac use to update the Z level list.
Ah, I just recalled another reason I used the focus handler, that I reproduced while updating this patch running on Fedora 15.

I don't get ConfigureNotify events when the window stacking order changes, only on the other stuff like when I move or resize the window.
And I don't seem to get CirculateNotify events at all.

Hm, this complicates things a lot...
Does X rely on the window manager to send these circulation events? If so that might explain the inconsistency.

Also karl, if you're worried about semantics I could split the Z level events into two, a SET and NOTIFY, and just remove the immediate field. Not sure if this could break addons, though.
Blocks: 455694
(In reply to comment #45)
> Does X rely on the window manager to send these circulation events? If so
> that might explain the inconsistency.

The window manager need only send ConfigureNotify on move (and real
ConfigureNotify events are received from the server on resize) but I don't
know of any requirement for the window manager to notify of stacking order
change.  It may be possible to watch for ConfigureNotify (and perhaps
CirculateNotify) on all windows between the client window and the root window,
but I fear it could be difficult to manage.  (The intermediate windows may
exist only temporarily.)

I think _NET_CLIENT_LIST_STACKING is going to be our simplest option if we
really want to track Z-order.
http://standards.freedesktop.org/wm-spec/wm-spec-1.4.html#id2505795

Even though it is possible to lower a window while it is not active, it may be
close enough to only watch the root window property changes while the app has
the active window, to avoid waking up the app when it not being used.

> Also karl, if you're worried about semantics I could split the Z level
> events into two, a SET and NOTIFY, and just remove the immediate field. Not
> sure if this could break addons, though.

I don't mind using the same event structure if the SET/NOTIFY distinction is
clear.

In comment 26 I thought other windows were restacked even when aImmediate is
not set.  So isn't mImmediate indicating something different from the
SET/NOTIFY distinction?

(In reply to comment #42)
> If a different window's Z level is set later (nsWindowMediator::SetZLevel),
> then the linked list is not only resorted followed by calls to PlaceBehind
> on all windows. But I suppose that's expected behaviour and is what is
> considered the "non-immediate" path I guess.

Perhaps you have already picked up from my comments that I didn't really
understand what you were saying here.

> The reason I chose focus events is because they are sent far less often than
> configure events, yet I cannot think of a common way window order changes
> other than focus.

Comment 22 notes that there is no focus event when an already-focused window
is raised.  A typical arrangement might be that mousing over a window gives it
focus, but clicking on its decorations raises it.
I'll continue to question whether z-order is really what is wanted here, and
so I wonder whether it is worth the effort.  I guess it depends on the
particular consumer.

If a window has been viewed and is then lowered to the bottom of the stack,
then it may be the preferred window to use for getMostRecentWindow.
(Also, if a window is minimized, it can still be at the top of the z-order stack, even after another window has been used, but we can still choose how to treat minimized windows internally.)

Perhaps using focus for getMostRecentWindow is appropriate (and the
documentation needs updating).  I was concerned about accidentally focusing
windows in comment 17, but focus may be a good approximation.  If a window got
focus, then at least it was not minimized at that time.

I'm much less happy about using focus to send ZLEVEL events or to affect
getZOrderDOMWindowEnumerator.  I think it would be better to make
getZOrderDOMWindowEnumerator fail when it is not available, and add a new
method for a recency-sorted list, if a list is needed.

Something else to consider is making getZOrderDOMWindowEnumerator invoke a widget method to get the list of windows, perhaps just for the X11 ports, so that we don't need to get the information unless/until it is needed.
(In reply to comment #46)
> Perhaps you have already picked up from my comments that I didn't really
> understand what you were saying here.

Bah, I typed that incorrectly.
When the event is sent: if mImmediate is false, only the linked list of windows is updated. If it is true, the linked list is updated and a widget function is called to change the Z level of the window.

(In reply to comment #47)
> I'll continue to question whether z-order is really what is wanted here, and
> so I wonder whether it is worth the effort.  I guess it depends on the
> particular consumer.

Apparently the tab animations patch really requires this, and asked me to fix this bug.

> Something else to consider is making getZOrderDOMWindowEnumerator invoke a
> widget method to get the list of windows, perhaps just for the X11 ports, so
> that we don't need to get the information unless/until it is needed.

This sounds like a good idea. I'll look into this.
So, a simple way of doing this would be, after calling the new method, to send Z level events for every window we recognize. We seem to use events rather than functions because of the many different type of windows there are.

The code in the window mediator is just an absolute pain to read and understand. I still don't get it.
Here's an update which should explain why this is taking so long. I'm struggling to find a clean way to implement this, since the only entry point from outside widget code where this function can be put is nsIWidget. This needs an implementation in nsBaseWidget, but it can't make any calls to nsWindow because that causes a circular dependency.

This would be easier if there were such a thing as a static virtual method.
Attached patch New WIPSplinter Review
This won't compile on non-GTK2, I'm still working on that but have a general plan. I'd like some feedback from karl on the general direction of this patch before I move down to semantics and fixups.
Attachment #551449 - Flags: feedback?(karlt)
(In reply to Michael Ventnor from comment #48)
> When the event is sent: if mImmediate is false, only the linked list of
> windows is updated.

It looks like PlaceWindowLayersBehind() can be called irrespective of the setting of mImmediate.
Comment on attachment 551449 [details] [diff] [review]
New WIP

(In reply to Michael Ventnor from comment #49)
> So, a simple way of doing this would be, after calling the new method, to
> send Z level events for every window we recognize.

I'd really like avoid dispatching events from a getter method, if at all possible.  A client is not likely to expect the possible side-effects that might result, and I imagine we'd need something to prevent recursion.

> We seem to use events
> rather than functions because of the many different type of windows there
> are.

I assume nsIBaseWindow::GetMainWidget() provides a mapping to nsIWidget.
I don't know whether there is an existing mapping the other way.

> The code in the window mediator is just an absolute pain to read and
> understand. I still don't get it.

Looks like Josh felt similarly: Bug 450576 comment 19.

He seemed to think that the solution on Mac would be to "change the window mediator to get a z-ordered list of windows from widget code instead of caching such a list and keeping it updated", so that is looking like the right approach for GTK too.  (Bug 450576 comment 9)

(In reply to Michael Ventnor from comment #50)
> Here's an update which should explain why this is taking so long. I'm
> struggling to find a clean way to implement this, since the only entry point
> from outside widget code where this function can be put is nsIWidget. This
> needs an implementation in nsBaseWidget, but it can't make any calls to
> nsWindow because that causes a circular dependency.

The nsBaseWidget implementation can simply return NS_ERROR_NOT_IMPLEMENTED.

> This would be easier if there were such a thing as a static virtual method.

Think about whether a new XPCOM service would be appropriate.
Attachment #551449 - Flags: feedback?(karlt) → feedback-
When this is fixed, we should also get rid of BROKEN_WM_Z_ORDER (see also bug 891194).
Whiteboard: tpi:+
Blocks: 1372434
No longer blocks: 1372434
Depends on: 1489833
No longer blocks: 1335457
Severity: normal → S3
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: