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*
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
