[PATCH weston 2/2] desktop-shell: fix minimization of fullscreen surfaces, pointer focus
Manuel Bachmann
manuel.bachmann at open.eurogiciel.org
Mon Mar 30 14:42:14 PDT 2015
Now re-sending this patch, modified, under the new name "desktop-shell: fix
minimization of fullscreen surfaces".
2015-03-30 18:26 GMT+02:00 Manuel Bachmann <
manuel.bachmann at open.eurogiciel.org>:
> 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*
>
--
Regards,
*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150330/74f022c2/attachment-0001.html>
More information about the wayland-devel
mailing list