[PATCH weston 2/2] desktop-shell: fix minimization of fullscreen surfaces, pointer focus
Manuel Bachmann
manuel.bachmann at open.eurogiciel.org
Mon Mar 30 09:26:13 PDT 2015
Hi Pekka,
"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."
This makes sense, thank you. I am thus going to remove the call to
"pointer_set_focus(.. 0, 0)". By the way, I have a WIP patch which will
allow focus to switch when you minimize a surface, just as when one gets
destroyed currently. This seems to be what most of the other window
managers do, and is likely to address this problem in a better way :-).
"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."
Practically, it works just nice ; but I agree, I will add a multi-line
comment on top of this, to explain why this is required.
Regards,
Manuel
2015-03-30 8:59 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> 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
> > >
> > >
> >
> >
>
>
--
Regards,
*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150330/abdb411a/attachment-0001.html>
More information about the wayland-devel
mailing list