Hi,
> > >> today, I noticed a problem in focus handling for shaded windows which is
> > >> pretty easy to reproduce:
> > >>
> > >> - Disable "Click to focus"
> > >> - Shade a window
> > >> - Hover another window
> > >> - Hover back to the window frame of the shaded window
> > >> - Press Ctrl+Alt+S
> > >>
> > >> Expected behaviour would be that the shaded window is unshaded. What
> > >> happens is that the last active window is shaded.
> > >>
> > >> I investigated this a bit and found the cause for this behaviour, but
> > >> wasn't able to work out a proper fix for that:
> > >> - The function shade() uses d->activeWindow to determine the window to
> > >> (un)shade via the "window" option.
> > >> - d->activeWindow is set from the FocusIn event handler, but only if
> > >> w->managed is set for the window which is hovered or whose frame is
> > >> hovered
> > >> - As w->managed is unset in the UnmapNotify handler (and so after a
> > >> window is shaded), hovering a shaded window frame does not set
> > >> d->activeWindow to that window.
> > >>
> > >> Any idea regarding that one?
> >
> > > w->managed should not be unset when getting an UnmapNotify event cause
> > > by shading. The window is still managed by compiz while being shaded.
> > > w->pendingUnmaps should be greater than 1 when a window is shaded but I
> > > guess that's broken somehow...
> >
> > You are right. This also works as intended. I walked a false path there,
> > sorry.
> > The real problem is that we don't select for FocusChange events for frame
> > windows and thus never get FocusIn events for them.
> > Correcting this revealed another problem: The lastFoundWindow variable was
> > (IMO incorrectly) sometimes also set to frame windows.
> > The attached patch seems to do the trick - any comments?
>
> I applied the FocusChange change just before I realized that it
> shouldn't be needed as we'll select for focus change events when the
> window gets added using the addWindow function and this change shouldn't
> make a difference.
It does make a difference. We're currently selecting for FocusChange
events of the _window_, not the _frame_.
The problem is that although we correctly set the input focus to the
frame window if the window is shaded (window.c:2870), we never get a
FocusIn event in response and thus never will set the shaded window
active (where findTopLevelWindowAtDisplay will retrieve the main window
for the focussed frame). That's why we IMO need to select for
FocusChange events also for the frame window.
> lastFoundWindow is just an optimization to avoid walking the list of
> windows when a window is looked up multiple times.
>
> The way lastFoundWindow is updated right now is more correct than what
> your patch will do.
I disagree. lastFoundWindow is used for both findWindowAtDisplay/Screen
and findTopLevelWindowAtDisplay/Screen. This has the side effect that
the behaviour of findTopLevelWindow depends on which window was
processed by findWindow before:
- Assumption: findWindowAtScreen is called for a frame window (which is
a valid scenario)
- findWindowAtScreen returns the frame window CompWindow struct and sets
lastFoundWindow to that
- if findTopLevelWindowAtScreen is called after that for the window the
frame window belongs to, the frame window is returned
- if lastFoundWindow would have been unset, the window itself would have
been returned by findTopLevelWindowAtScreen.
That's a behaviour change which shouldn't be caused by an optimization. Maybe a
better fix for that would be to track the last found top level window and last
found window separately (that's what the attached patch does).
Regards,
Danny
diff --git a/include/compiz.h b/include/compiz.h
index e870662..451d5e1 100644
--- a/include/compiz.h
+++ b/include/compiz.h
@@ -214,6 +214,7 @@ extern Window currentRoot;
extern Bool shutDown;
extern Bool restartSignal;
extern CompWindow *lastFoundWindow;
+extern CompWindow *lastFoundTopLevelWindow;
extern CompWindow *lastDamagedWindow;
extern Bool replaceCurrentWm;
extern Bool indirectRendering;
diff --git a/src/display.c b/src/display.c
index ac9ba1e..7cd75fa 100644
--- a/src/display.c
+++ b/src/display.c
@@ -2690,9 +2690,9 @@ CompWindow *
findTopLevelWindowAtDisplay (CompDisplay *d,
Window id)
{
- if (lastFoundWindow && lastFoundWindow->id == id)
+ if (lastFoundTopLevelWindow && lastFoundTopLevelWindow->id == id)
{
- return lastFoundWindow;
+ return lastFoundTopLevelWindow;
}
else
{
diff --git a/src/main.c b/src/main.c
index a85c484..d1a8b66 100644
--- a/src/main.c
+++ b/src/main.c
@@ -54,6 +54,7 @@ Bool shutDown = FALSE;
Bool restartSignal = FALSE;
CompWindow *lastFoundWindow = 0;
+CompWindow *lastFoundTopLevelWindow = 0;
CompWindow *lastDamagedWindow = 0;
Bool replaceCurrentWm = FALSE;
diff --git a/src/screen.c b/src/screen.c
index 15458f7..eab372c 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -2210,9 +2210,9 @@ findTopLevelWindowAtScreen (CompScreen *s,
{
CompWindow *found = NULL;
- if (lastFoundWindow && lastFoundWindow->id == id)
+ if (lastFoundTopLevelWindow && lastFoundTopLevelWindow->id == id)
{
- found = lastFoundWindow;
+ found = lastFoundTopLevelWindow;
}
else
{
@@ -2222,7 +2222,7 @@ findTopLevelWindowAtScreen (CompScreen *s,
{
if (w->id == id)
{
- found = (lastFoundWindow = w);
+ found = w;
break;
}
}
@@ -2240,12 +2240,16 @@ findTopLevelWindowAtScreen (CompScreen *s,
for (w = s->windows; w; w = w->next)
if (w->frame == id)
+ {
+ lastFoundTopLevelWindow = w;
return w;
+ }
}
return NULL;
}
+ lastFoundTopLevelWindow = found;
return found;
}
diff --git a/src/window.c b/src/window.c
index ddfc123..f961b2c 100644
--- a/src/window.c
+++ b/src/window.c
@@ -1083,7 +1083,7 @@ updateFrameWindow (CompWindow *w)
XSetWindowAttributes attr;
XWindowChanges xwc;
- attr.event_mask = 0;
+ attr.event_mask = FocusChangeMask;
attr.override_redirect = TRUE;
w->frame = XCreateWindow (w->screen->display->display,
_______________________________________________
compiz mailing list
compiz@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/compiz
