<div dir="ltr">Now re-sending this patch, modified, under the new name "desktop-shell: fix minimization of fullscreen surfaces". </div><div class="gmail_extra"> <div class="gmail_quote">2015-03-30 18:26 GMT+02:00 Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Pekka, <div> "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 </div></div><div class="gmail_extra"><div><div class="h5"> <div class="gmail_quote">2015-03-30 8:59 GMT+02:00 Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, 28 Mar 2015 07:29:48 +0100 Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> 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 <div><div> > 2015-03-27 15:22 GMT+01:00 Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>>: > > > On Mon, 16 Feb 2015 11:52:47 +0100 > > Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> wrote: > > > > > From: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> > > > > > > 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 <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> > > > Author: Nicolas Guyomard <<a href="mailto:nicolas.guyomard@open.eurogiciel.org" target="_blank">nicolas.guyomard@open.eurogiciel.org</a>> > > > Signed-off-by: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> > > > --- > > > 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@15.set_minimized() > > [2030091.737] -> xdg_popup@21.destroy() > > [2030091.768] -> wl_surface@25.destroy() > > [2030091.805] -> wl_buffer@29.destroy() > > [2030091.892] -> wl_shm_pool@28.destroy() > > [2030092.413] wl_display@1.delete_id(21) > > [2030092.469] wl_display@1.delete_id(25) > > [2030092.503] wl_display@1.delete_id(29) > > [2030092.537] wl_display@1.delete_id(28) > > [2030092.570] wl_keyboard@16.leave(315, wl_surface@14) > > [2030092.633] xdg_surface@15.configure(1024, 600, array, 316) > > [2030092.722] -> xdg_surface@15.ack_configure(316) > > [2030092.762] -> xdg_surface@15.set_title("pq@erebos:~/git/weston") > > [2030092.798] wl_pointer@17.leave(317, nil) > > [2030092.853] wl_keyboard@16.modifiers(318, 0, 0, 0, 0) > > [2030093.019] wl_pointer@17.enter(318, nil, 69.000000, 109.000000) > > [2030093.154] xdg_shell@12.ping(319) > > [2030093.195] -> xdg_shell@12.pong(319) > > [2030093.265] wl_pointer@17.leave(320, nil) > > [2030093.322] -> wl_compositor@4.create_region(new id wl_region@28) > > [2030093.372] -> xdg_surface@15.set_title("pq@erebos:~/git/weston") > > [2030093.409] -> wl_compositor@4.create_region(new id wl_region@29) > > [2030093.455] -> wl_region@29.add(0, 0, 1024, 600) > > [2030093.553] -> wl_region@28.add(0, 0, 1024, 600) > > [2030093.642] -> wl_surface@14.frame(new id wl_callback@25) > > [2030098.044] -> wl_surface@14.set_opaque_region(wl_region@28) > > [2030098.142] -> wl_region@28.destroy() > > [2030098.170] -> wl_surface@14.set_input_region(wl_region@29) > > [2030098.208] -> wl_region@29.destroy() > > [2030098.291] -> wl_surface@14.attach(wl_buffer@24, 0, 0) > > [2030098.379] -> wl_surface@14.damage(0, 0, 1024, 600) > > [2030098.436] -> wl_surface@14.commit() > > [2030103.646] wl_display@1.delete_id(28) > > [2030103.727] wl_display@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 > > > > > > </div></div></blockquote></div> </div></div>-- <div><div dir="ltr">Regards, Manuel BACHMANN Tizen Project VANNES-FR </div></div> </div> </blockquote></div> -- <div class="gmail_signature"><div dir="ltr">Regards, Manuel BACHMANN Tizen Project VANNES-FR </div></div> </div>