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 < [email protected]>: > 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 <[email protected]>: > >> On Sat, 28 Mar 2015 07:29:48 +0100 >> Manuel Bachmann <[email protected]> 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 <[email protected]>: >> > >> > > On Mon, 16 Feb 2015 11:52:47 +0100 >> > > Manuel Bachmann <[email protected]> wrote: >> > > >> > > > From: Manuel Bachmann <[email protected]> >> > > > >> > > > 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 <[email protected]> >> > > > Author: Nicolas Guyomard <[email protected]> >> > > > Signed-off-by: Manuel Bachmann <[email protected] >> > >> > > > --- >> > > > 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] -> [email protected]_minimized() >> > > [2030091.737] -> [email protected]() >> > > [2030091.768] -> [email protected]() >> > > [2030091.805] -> [email protected]() >> > > [2030091.892] -> [email protected]() >> > > [2030092.413] [email protected]_id(21) >> > > [2030092.469] [email protected]_id(25) >> > > [2030092.503] [email protected]_id(29) >> > > [2030092.537] [email protected]_id(28) >> > > [2030092.570] [email protected](315, wl_surface@14) >> > > [2030092.633] [email protected](1024, 600, array, 316) >> > > [2030092.722] -> [email protected]_configure(316) >> > > [2030092.762] -> [email protected]_title("pq@erebos:~/git/weston") >> > > [2030092.798] [email protected](317, nil) >> > > [2030092.853] [email protected](318, 0, 0, 0, 0) >> > > [2030093.019] [email protected](318, nil, 69.000000, 109.000000) >> > > [2030093.154] [email protected](319) >> > > [2030093.195] -> [email protected](319) >> > > [2030093.265] [email protected](320, nil) >> > > [2030093.322] -> [email protected]_region(new id wl_region@28) >> > > [2030093.372] -> [email protected]_title("pq@erebos:~/git/weston") >> > > [2030093.409] -> [email protected]_region(new id wl_region@29) >> > > [2030093.455] -> [email protected](0, 0, 1024, 600) >> > > [2030093.553] -> [email protected](0, 0, 1024, 600) >> > > [2030093.642] -> [email protected](new id wl_callback@25) >> > > [2030098.044] -> [email protected]_opaque_region(wl_region@28) >> > > [2030098.142] -> [email protected]() >> > > [2030098.170] -> [email protected]_input_region(wl_region@29) >> > > [2030098.208] -> [email protected]() >> > > [2030098.291] -> [email protected](wl_buffer@24, 0, 0) >> > > [2030098.379] -> [email protected](0, 0, 1024, 600) >> > > [2030098.436] -> [email protected]() >> > > [2030103.646] [email protected]_id(28) >> > > [2030103.727] [email protected]_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*
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
