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

[Manuel Bachmann]

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 ?

"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 ?).

"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.

"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.
---
(btw, as you pushed master, you'll want to take a look at my 2 no-brainer
patches which fix building it :
http://lists.freedesktop.org/archives/wayland-devel/2015-March/020920.html
- http://lists.freedesktop.org/archives/wayland-devel/2015-March/020921.html
)

Regards,

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
>
>


-- 
Regards,



*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150328/6db91f5c/attachment-0001.html>