<div dir="ltr"><div>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." </div><div>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?" </div><div>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?" </div><div>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?" </div><div>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. --- </div><div>(btw, as you pushed master, you'll want to take a look at my 2 no-brainer patches which fix building it : <a href="http://lists.freedesktop.org/archives/wayland-devel/2015-March/020920.html">http://lists.freedesktop.org/archives/wayland-devel/2015-March/020920.html</a> - <a href="http://lists.freedesktop.org/archives/wayland-devel/2015-March/020921.html">http://lists.freedesktop.org/archives/wayland-devel/2015-March/020921.html</a>) </div><div>Regards,</div></div><div class="gmail_extra"> <div class="gmail_quote">2015-03-27 15:22 GMT+01: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"><div class="HOEnZb"><div class="h5">On Mon, 16 Feb 2015 11:52:47 +0100 Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> wrote: > From: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">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">manuel.bachmann@open.eurogiciel.org</a>> > Author: Nicolas Guyomard <<a href="mailto:nicolas.guyomard@open.eurogiciel.org">nicolas.guyomard@open.eurogiciel.org</a>> > Signed-off-by: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">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)); > + } > } </div></div>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 </blockquote></div> -- <div class="gmail_signature"><div dir="ltr">Regards, Manuel BACHMANN Tizen Project VANNES-FR </div></div> </div>