Re: [compiz] Focus problem for shaded windows
Hi, > > > > 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. > > We do select for FocusChange events on the frame window at > window.c:1974. The frame window will be added to the window list just as > all other windows and we'll then select for FocusChange events, which is > why we don't have to do this when creating the window. Oh, yes, you are right ... I overlooked that addWindow is called for frame windows as well. Sorry. > > > 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) > > sure. > > > - findWindowAtScreen returns the frame window CompWindow struct and sets > > lastFoundWindow to that > > yes. > > > - if findTopLevelWindowAtScreen is called after that for the window the > > frame window belongs to, the frame window is returned > > No, how can that happen? lastFoundWindow->id will not match the id > passed to findTopLevelWindowAtScreen so it will walk the window list > until it finds the appropriate window with a matching id and return it. My point is valid - my explanation is not (dunno what I thought when I wrote that :-/ ). What I wanted to say: - findWindowAtDisplay (some_frame_window) will return some_frame_window - findTopLevelWindowAtDisplay (some_frame_window) will return some_frame_window after that although it should return the main window belonging to it. But it doesn't matter, as ... > I think I just found the bug that is causing the problem. > findWindowAtScreen and findTopLevelWindowAtScreen are fine. However, > findTopLevelWindowAtDisplay is broken. It shouldn't check > lastFoundWindow. I pushed out changes that will fix this problem and > clean up findTopLevelWindowAtScreen so it will be a bit easier to > understand how lastFoundWindow is used. > > Let me know if this fixed your problem. ... this change fixes the problem as findTopLevelWindowAtDisplay no longer immediately returns lastFoundWindow. Thanks. Regards, Danny ___ compiz mailing list compiz@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/compiz
Re: [compiz] Focus problem for shaded windows
On Tue, 2007-08-07 at 20:44 +0200, Danny Baumann wrote: > 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. We do select for FocusChange events on the frame window at window.c:1974. The frame window will be added to the window list just as all other windows and we'll then select for FocusChange events, which is why we don't have to do this when creating the 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) sure. > - findWindowAtScreen returns the frame window CompWindow struct and sets > lastFoundWindow to that yes. > - if findTopLevelWindowAtScreen is called after that for the window the > frame window belongs to, the frame window is returned No, how can that happen? lastFoundWindow->id will not match the id passed to findTopLevelWindowAtScreen so it will walk the window list until it finds the appropriate window with a matching id and return it. > - if lastFoundWindow would have been unset, the window itself would have > been returned by findTopLevelWindowAtScreen. lastFoundWindow being unset doesn't change anything. lastFoundWindow is only a cache of the last found window. That's all. lastFoundWindow is only used when lastFoundWindow->id match the id we're searching for. > > 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). We could keep a separate cache for top-level windows but they are not searched for as often so I don't think it will make much of a difference. I think I just found the bug that is causing the problem. findWindowAtScreen and findTopLevelWindowAtScreen are fine. However, findTopLevelWindowAtDisplay is broken. It shouldn't check lastFoundWindow. I pushed out changes that will fix this problem and clean up findTopLevelWindowAt
Re: [compiz] Focus problem for shaded windows
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 @@ findTopLe
Re: [compiz] Focus problem for shaded windows
On Mon, 2007-08-06 at 10:15 +0200, Danny Baumann wrote: > >> 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. 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. -David ___ compiz mailing list compiz@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/compiz
Re: [compiz] Focus problem for shaded windows
>> 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?
Regards,
Danny
diff --git a/src/screen.c b/src/screen.c
index 15458f7..f8bd082 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -2198,7 +2198,12 @@ findWindowAtScreen (CompScreen *s,
for (w = s->windows; w; w = w->next)
if (w->id == id)
- return (lastFoundWindow = w);
+ {
+ /* FIXME: not sure if this is correct */
+ if (w->managed)
+ lastFoundWindow = w;
+ return w;
+ }
}
return 0;
@@ -,7 +2227,7 @@ findTopLevelWindowAtScreen (CompScreen *s,
{
if (w->id == id)
{
- found = (lastFoundWindow = w);
+ found = w;
break;
}
}
@@ -2240,12 +2245,16 @@ findTopLevelWindowAtScreen (CompScreen *s,
for (w = s->windows; w; w = w->next)
if (w->frame == id)
+ {
+ lastFoundWindow = w;
return w;
+ }
}
return NULL;
}
+lastFoundWindow = 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
Re: [compiz] Focus problem for shaded windows
On Mon, 2007-06-25 at 15:35 +0200, Danny Baumann wrote: > 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... -David ___ compiz mailing list compiz@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/compiz
[compiz] Focus problem for shaded windows
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? Regards, Danny ___ compiz mailing list compiz@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/compiz
