[compiz] Focus problem for shaded windows

[David Reveman]

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 findTopLevelWindowAtScreen so it will be a bit easier to
understand how lastFoundWindow is used.

Let me know if this fixed your problem.

-David