[PATCH weston 2/2] desktop-shell: fix minimization of fullscreen surfaces, pointer focus

[Pekka Paalanen]

On Sat, 28 Mar 2015 07:29:48 +0100
Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:

> Hi Pekka, thanks a lot for getting this on the rails !
> 
> "I'm not so sure about the pointer_set_focus here. Shuffling surfaces is
> supposed to trigger a repick, which will reassign the pointer focus.
> However, it seems something is off."
> 
> The "pointer_set_focus(... 0, 0)" thing was added for a client (Chromium)
> whch privilegies pointer focus over keyboard focus, It considers itself
> still focused when minimized ; mostly because it disregards keyboard focus
> and, last time I looked in desktop-shell, even xdg-shell activate() events
> were mostly linked to keyboard focus. So adding this call for pointer_leave
> fixed it. What is your opinon on this ?

It is the wrong way to fix it.

Not wl_pointer nor wl_keyboard foci are interchangeable with the
concept of "active". It simply just happens that an active window will
usually have the kbd focus if a kbd exists, because users expect that.
From a purely protocol-mechanical point of view there is no connection.
Giving keyboard focus to an active window is compositor internal policy.

The app must be fixed to honour the activate events if it uses
xdg_shell. In xdg_shell, window activeness is notified via configure
events' flags. The spec says:
      <entry name="activated" value="4">
        Client window decorations should be painted as if the window is
        active. Do not assume this means that the window actually has
        keyboard or pointer focus.
      </entry>

If it uses wl_shell, then you have to rely on kbd focus, because
wl_shell doesn't have anything better. This means that in a
keyboard-less system, wl_shell cannot even communicate activeness. Maybe
that's the reason why someone might abuse pointer focus as window
activation? But when he does that, the window will switch to an
inactive look by just a pointer leave, while the user could still be
typing into the window. That would be confusing.

If this is some tablet use case, then you wouldn't have either
wl_pointer not wl_keyboard, you'd only have wl_touch. And touch "focus"
disappears the moment the last finger goes up. Note, that virtual
keyboards do not communicate via wl_keyboard. So in a tablet case, the
window activeness is completely separated from any foci.

Pointer focus is good for highlighting the widget under the pointer
kind of things, but neither pointer not keyboard foci are meant to
signify activeness.

> "assume the first wl_pointer.leave(nil) is for the popup surface, so
> that's ok. The enter for nil looks very suspicious though. Maybe that
> is caused by this patch?"
> 
> It clearly is ; I interpret this as the pointer leaving the main terminal
> surface when it minimizes, which seems logical, but maybe I'm misleaded
> (btw, how do you get all these logs ?).

You can get them with WAYLAND_DEBUG=client env var, when running the
client.

Entering a NULL surface makes no sense to a client, it can only ignore
it. It already received a leave, which means that wl_pointer is not on
any of its own surfaces. Saying "the pointer is still not on any of
your surfaces" is redundant.

These NULL arguments should happen only in race conditions. For
instance, the compositor assigned pointer focus to a surface, that the
client has already destroyed but the compositor has not processed the
destruction yet. If the compositor *has* processed the destruction,
there is no reason to send enter for NULL.

> "Just curious, how can the surface go off-center? Some output
> hot-unplugging involved?"
> 
> The call to "unset_fullscreen()" will reposition the surface at its former
> non-fullscreen position (old_x, old_y), which is good. When the surface
> gets focused again later, fullscreen state gets triggered again because the
> hint is stll too, which is good, too ; but the x,y is now old_x, old_y, so
> off-center.

Hmm, I'm not sure if that's clever or abuse... it's certainly
unexpected and requires effort to see in the code, which calls a for a
big comment to explain what is happening.

> "When wouldn't there be a black_view?"
> 
> When you refocus a minimized surface, black_view has been destroyed by
> "unset_fullscreen()". It gets created later again, but after this part,
> where it is NULL and thus crashes.

Alright.


Thanks,
pq

> 2015-03-27 15:22 GMT+01:00 Pekka Paalanen <ppaalanen at gmail.com>:
> 
> > On Mon, 16 Feb 2015 11:52:47 +0100
> > Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:
> >
> > > From: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> > >
> > > 2 things were wrong with the minimization code :
> > > - if the minimized surface was fullscreen, the shell would keep
> > >   its fullscreen mode on and only display a black background ;
> > > - keyboard focus was unset, but pointer focus was not.
> > >
> > > On the other hand, make sure a fullscreen surface gets centered
> > > again when refocused with Mod+Tab.
> > >
> > > Author: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> > > Author: Nicolas Guyomard <nicolas.guyomard at open.eurogiciel.org>
> > > Signed-off-by: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> > > ---
> > >  desktop-shell/shell.c |   16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > > index 5900c4d..830c25d 100644
> > > --- a/desktop-shell/shell.c
> > > +++ b/desktop-shell/shell.c
> > > @@ -2657,13 +2657,20 @@ set_minimized(struct weston_surface *surface)
> > >       weston_layer_entry_remove(&view->layer_link);
> > >
> >  weston_layer_entry_insert(&shsurf->shell->minimized_layer.view_list,
> > &view->layer_link);
> > >
> > > +     if (shsurf->state.fullscreen)
> > > +             unset_fullscreen(shsurf);
> > > +
> > >       drop_focus_state(shsurf->shell, current_ws, view->surface);
> > >       wl_list_for_each(seat, &shsurf->shell->compositor->seat_list,
> > link) {
> > >               if (!seat->keyboard)
> > >                       continue;
> > >               focus =
> > weston_surface_get_main_surface(seat->keyboard->focus);
> > > -             if (focus == view->surface)
> > > +             if (focus == view->surface) {
> > >                       weston_keyboard_set_focus(seat->keyboard, NULL);
> > > +                     weston_pointer_set_focus(seat->pointer, NULL,
> > > +                                              wl_fixed_from_int(0),
> > > +                                              wl_fixed_from_int(0));
> > > +             }
> > >       }
> >
> > Hi,
> >
> > I'm not so sure about the pointer_set_focus here. Shuffling surfaces is
> > supposed to trigger a repick, which will reassign the pointer focus.
> > However, it seems something is off. The keyboards focus is different to
> > pointer focus, because keyboard focus is "sticky" and needs to be
> > explicitly managed. Pointer focus generally follows picking unless the
> > pointer is grabbed.
> >
> > With this patch applied:
> >
> > [2030091.702]  -> xdg_surface at 15.set_minimized()
> > [2030091.737]  -> xdg_popup at 21.destroy()
> > [2030091.768]  -> wl_surface at 25.destroy()
> > [2030091.805]  -> wl_buffer at 29.destroy()
> > [2030091.892]  -> wl_shm_pool at 28.destroy()
> > [2030092.413] wl_display at 1.delete_id(21)
> > [2030092.469] wl_display at 1.delete_id(25)
> > [2030092.503] wl_display at 1.delete_id(29)
> > [2030092.537] wl_display at 1.delete_id(28)
> > [2030092.570] wl_keyboard at 16.leave(315, wl_surface at 14)
> > [2030092.633] xdg_surface at 15.configure(1024, 600, array, 316)
> > [2030092.722]  -> xdg_surface at 15.ack_configure(316)
> > [2030092.762]  -> xdg_surface at 15.set_title("pq at erebos:~/git/weston")
> > [2030092.798] wl_pointer at 17.leave(317, nil)
> > [2030092.853] wl_keyboard at 16.modifiers(318, 0, 0, 0, 0)
> > [2030093.019] wl_pointer at 17.enter(318, nil, 69.000000, 109.000000)
> > [2030093.154] xdg_shell at 12.ping(319)
> > [2030093.195]  -> xdg_shell at 12.pong(319)
> > [2030093.265] wl_pointer at 17.leave(320, nil)
> > [2030093.322]  -> wl_compositor at 4.create_region(new id wl_region at 28)
> > [2030093.372]  -> xdg_surface at 15.set_title("pq at erebos:~/git/weston")
> > [2030093.409]  -> wl_compositor at 4.create_region(new id wl_region at 29)
> > [2030093.455]  -> wl_region at 29.add(0, 0, 1024, 600)
> > [2030093.553]  -> wl_region at 28.add(0, 0, 1024, 600)
> > [2030093.642]  -> wl_surface at 14.frame(new id wl_callback at 25)
> > [2030098.044]  -> wl_surface at 14.set_opaque_region(wl_region at 28)
> > [2030098.142]  -> wl_region at 28.destroy()
> > [2030098.170]  -> wl_surface at 14.set_input_region(wl_region at 29)
> > [2030098.208]  -> wl_region at 29.destroy()
> > [2030098.291]  -> wl_surface at 14.attach(wl_buffer at 24, 0, 0)
> > [2030098.379]  -> wl_surface at 14.damage(0, 0, 1024, 600)
> > [2030098.436]  -> wl_surface at 14.commit()
> > [2030103.646] wl_display at 1.delete_id(28)
> > [2030103.727] wl_display at 1.delete_id(29)
> >
> > This was made with weston-terminal patched to have a "minimize" option
> > in the context menu. I just pushed that patch to master, btw. so
> > everyone can test.
> >
> > I assume the first wl_pointer.leave(nil) is for the popup surface, so
> > that's ok. The enter for nil looks very suspicious though. Maybe that
> > is caused by this patch?
> >
> > My opinion is that this looks okay apart from the pointer focus.
> >
> > >
> > >       shell_surface_update_child_surface_layers(shsurf);
> > > @@ -5937,8 +5944,11 @@ switcher_next(struct switcher *switcher)
> > >               view->alpha = 1.0;
> > >
> > >       shsurf = get_shell_surface(switcher->current);
> > > -     if (shsurf && shsurf->state.fullscreen)
> > > -             shsurf->fullscreen.black_view->alpha = 1.0;
> > > +     if (shsurf && shsurf->state.fullscreen) {
> > > +             center_on_output(shsurf->view, shsurf->fullscreen_output);
> > > +             if (shsurf->fullscreen.black_view)
> > > +                     shsurf->fullscreen.black_view->alpha = 1.0;
> >
> > Just curious, how can the surface go off-center? Some output
> > hot-unplugging involved?
> >
> > When wouldn't there be a black_view?
> >
> > > +     }
> > >  }
> > >
> > >  static void
> >
> > Thanks,
> > pq
> >
> >
> 
>