On Thu, May 26, 2016 at 06:01:27PM +0300, Pekka Paalanen wrote:
> On Thu, 7 Apr 2016 16:44:17 -0700
> Bryce Harrington <[email protected]> wrote:
>
> > Signed-off-by: Bryce Harrington <[email protected]>
>
> Hi,
>
> the commit message could use an explanation of what the 'bool active'
> will actually be useful for. What do we need the "is in active use" for?
> The comment in the code does not really tell me either. This ties in
> with my comments on patch 3.
>
> The patch subject is misleading. There is no inhibition state tracking
> at all in this patch. Instead, this patch is about tracking surface
> activeness as defined by the shells.
>
> > ---
> > v3: Rename inhibit_screensaving to inhibit_idling
> >
> > desktop-shell/shell.c | 4 +++-
> > fullscreen-shell/fullscreen-shell.c | 25 +++++++++++++++-------
> > ivi-shell/ivi-shell.c | 4 +++-
> > src/compositor.c | 42
> > +++++++++++++++++++++++++++++++++++++
> > src/compositor.h | 27 +++++++++++++++++++++---
> > src/input.c | 15 -------------
> > tests/weston-test.c | 8 +++++--
> > 7 files changed, 96 insertions(+), 29 deletions(-)
> >
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 780902d..6e49076 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -5055,7 +5055,9 @@ activate(struct desktop_shell *shell, struct
> > weston_surface *es,
> > * Leave fullscreen surfaces on unrelated outputs alone. */
> > lower_fullscreen_layer(shell, shsurf->output);
> >
> > - weston_surface_activate(es, seat);
> > + weston_surface_assign_keyboard(es, seat);
> > + if (es != NULL)
>
> 'es' cannot be NULL.
>
> > + weston_surface_activate(es);
> >
> > state = ensure_focus_state(shell, seat);
> > if (state == NULL)
> > diff --git a/fullscreen-shell/fullscreen-shell.c
> > b/fullscreen-shell/fullscreen-shell.c
> > index abc4e84..e1f8a63 100644
> > --- a/fullscreen-shell/fullscreen-shell.c
> > +++ b/fullscreen-shell/fullscreen-shell.c
> > @@ -88,8 +88,11 @@ pointer_focus_changed(struct wl_listener *listener, void
> > *data)
> > {
> > struct weston_pointer *pointer = data;
> >
> > - if (pointer->focus && pointer->focus->surface->resource)
> > - weston_surface_activate(pointer->focus->surface, pointer->seat);
> > + if (pointer->focus && pointer->focus->surface->resource) {
> > + weston_surface_assign_keyboard(pointer->focus->surface,
> > pointer->seat);
> > + if (pointer->focus->surface != NULL)
>
> pointer->focus->surface cannot be NULL.
>
> > + weston_surface_activate(pointer->focus->surface);
> > + }
> > }
> >
> > static void
> > @@ -118,7 +121,9 @@ seat_caps_changed(struct wl_listener *l, void *data)
> > if (keyboard && keyboard->focus != NULL) {
> > wl_list_for_each(fsout, &listener->shell->output_list, link) {
> > if (fsout->surface) {
> > - weston_surface_activate(fsout->surface, seat);
> > + weston_surface_assign_keyboard(fsout->surface,
> > seat);
> > + if (fsout->surface != NULL)
>
> Cannot be NULL.
>
> > + weston_surface_activate(fsout->surface);
> > return;
> > }
> > }
> > @@ -703,8 +708,11 @@ fullscreen_shell_present_surface(struct wl_client
> > *client,
> > struct weston_keyboard *keyboard =
> > weston_seat_get_keyboard(seat);
> >
> > - if (keyboard && !keyboard->focus)
> > - weston_surface_activate(surface, seat);
> > + if (keyboard && !keyboard->focus) {
> > + weston_surface_assign_keyboard(surface, seat);
> > + if (surface != NULL)
>
> Cannot be NULL.
>
> > + weston_surface_activate(surface);
> > + }
> > }
> > }
> > }
> > @@ -754,8 +762,11 @@ fullscreen_shell_present_surface_for_mode(struct
> > wl_client *client,
> > struct weston_keyboard *keyboard =
> > weston_seat_get_keyboard(seat);
> >
> > - if (keyboard && !keyboard->focus)
> > - weston_surface_activate(surface, seat);
> > + if (keyboard && !keyboard->focus) {
> > + weston_surface_assign_keyboard(surface, seat);
> > + if (surface != NULL)
>
> Cannot be NULL.
>
> > + weston_surface_activate(surface);
> > + }
> > }
> > }
> >
> > diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> > index a767ccf..59f5656 100644
> > --- a/ivi-shell/ivi-shell.c
> > +++ b/ivi-shell/ivi-shell.c
> > @@ -425,7 +425,9 @@ activate_binding(struct weston_seat *seat,
> > if (get_ivi_shell_surface(main_surface) == NULL)
> > return;
> >
> > - weston_surface_activate(focus, seat);
> > + weston_surface_assign_keyboard(focus, seat);
> > + if (focus != NULL)
>
> Cannot be NULL.
>
> > + weston_surface_activate(focus);
> > }
> >
> > static void
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 83cabf7..9531a0a 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -612,6 +612,9 @@ weston_surface_create(struct weston_compositor
> > *compositor)
> > weston_matrix_init(&surface->buffer_to_surface_matrix);
> > weston_matrix_init(&surface->surface_to_buffer_matrix);
> >
> > + surface->active = false;
> > + surface->inhibit_idling = false;
> > +
> > return surface;
> > }
> >
> > @@ -3422,6 +3425,45 @@ weston_surface_copy_content(struct weston_surface
> > *surface,
> > src_x, src_y, width, height);
> > }
> >
> > +/** Sets the keyboard focus to the given surface
> > + */
> > +WL_EXPORT void
> > +weston_surface_assign_keyboard(struct weston_surface *surface,
> > + struct weston_seat *seat)
>
> This should be called weston_seat_set_keyboard_focus(seat, surface).
> Keyboard focus is a property of the seat.
>
> > +{
> > + struct weston_compositor *compositor = seat->compositor;
> > + struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> > +
> > + if (keyboard) {
> > + weston_keyboard_set_focus(keyboard, surface);
> > + wl_data_device_set_keyboard_focus(seat);
>
> I wonder if wl_data_device_set_keyboard_focus() is a misnomer. Its
> purpose is to offer the selection to the newly activated surface, so in
> that respect it should belong in weston_surface_activate() and not be
> tied to the keyboard in any way. But, as it currently is inherently
> tied to the keyboard, it needs to be here as it is.
>
> > + }
> > +
> > + wl_signal_emit(&compositor->activate_signal, surface);
>
> Why is sending the activate_signal here and not part of
> weston_surface_activate()?
No, it doesn't belong in weston_surface_activate(). This is not a new
signal added by this patch but a pre-existing one that gets triggered as
part of the keyboard focus assignment.
> > +}
> > +
> > +/** Set surface as considered 'active' by the shell.
> > + */
> > +WL_EXPORT void
> > +weston_surface_activate(struct weston_surface *surface)
> > +{
> > + surface->active = true;
> > +}
> > +
> > +/** Set surface as no longer considered 'active' by the shell.
> > + */
> > +WL_EXPORT void
> > +weston_surface_deactivate(struct weston_surface *surface)
> > +{
> > + surface->active = false;
> > +}
>
> There was not a single call to weston_surface_deactivate() in this
> patch. IOW, half of this patch is missing! See comments to patch 4.
>
> That is, if surface->active is meant to essentially be equivalent to
> keyboard focus if a keyboard existed?
>
> > +
> > +WL_EXPORT bool
> > +weston_surface_is_active(struct weston_surface *surface)
> > +{
> > + return surface->active;
> > +}
>
> This is also unused, but that's ok, I assume no-one is yet interested
> in it. But missing the deactivate is a real problem, because not using
> it will leave stale state all over.
At this point in the series nothing actually relies on that state being
coherent. But I suppose I could think about moving the
weston_surface_activate() calls out of this patch into a latter one if
it makes it clearer to you; technically it works the same either way.
The activate/deactivate is a shell-specific thing so the deactivate
calls do actually belong with those patches.
> > static void
> > subsurface_set_position(struct wl_client *client,
> > struct wl_resource *resource, int32_t x, int32_t y)
> > diff --git a/src/compositor.h b/src/compositor.h
> > index df8ef2d..d8d5368 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -1037,6 +1037,20 @@ struct weston_surface {
> > const char *role_name;
> >
> > struct weston_timeline_object timeline;
> > +
> > + /*
> > + * A shell-specific indicator that the surface is in immediate
> > + * use by the user. For example, the surface might have keyboard
> > + * focus.
> > + */
> > + bool active;
> > +
> > + /*
> > + * Indicates the surface prefers no screenblanking, screensaving,
> > + * or other automatic obscurement to kick in while the surface is
> > + * considered "active" by the shell.
> > + */
> > + bool inhibit_idling;
>
> This member is added, but not used at all, not even in any added unused
> functions. Therefore it does not belong in this patch, it belongs in the
> patch that starts using it.
>
> > };
> >
> > struct weston_subsurface {
> > @@ -1122,9 +1136,6 @@ int
> > weston_spring_done(struct weston_spring *spring);
> >
> > void
> > -weston_surface_activate(struct weston_surface *surface,
> > - struct weston_seat *seat);
> > -void
> > notify_motion(struct weston_seat *seat, uint32_t time,
> > struct weston_pointer_motion_event *event);
> > void
> > @@ -1402,6 +1413,16 @@ weston_surface_copy_content(struct weston_surface
> > *surface,
> > int src_x, int src_y,
> > int width, int height);
> >
> > +void
> > +weston_surface_assign_keyboard(struct weston_surface *surface,
> > + struct weston_seat *seat);
> > +void
> > +weston_surface_activate(struct weston_surface *surface);
> > +void
> > +weston_surface_deactivate(struct weston_surface *surface);
> > +bool
> > +weston_surface_is_active(struct weston_surface *surface);
> > +
> > struct weston_buffer *
> > weston_buffer_from_resource(struct wl_resource *resource);
> >
> > diff --git a/src/input.c b/src/input.c
> > index a3d982e..6e35105 100644
> > --- a/src/input.c
> > +++ b/src/input.c
> > @@ -1206,21 +1206,6 @@ notify_motion_absolute(struct weston_seat *seat,
> > }
> >
> > WL_EXPORT void
> > -weston_surface_activate(struct weston_surface *surface,
> > - struct weston_seat *seat)
> > -{
> > - struct weston_compositor *compositor = seat->compositor;
> > - struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat);
> > -
> > - if (keyboard) {
> > - weston_keyboard_set_focus(keyboard, surface);
> > - wl_data_device_set_keyboard_focus(seat);
> > - }
> > -
> > - wl_signal_emit(&compositor->activate_signal, surface);
> > -}
>
> It is a bit hard to see if this function was allowed to be called with
> NULL, so I just checked all the callsites replaced in this patch.
>
> > -
> > -WL_EXPORT void
> > notify_button(struct weston_seat *seat, uint32_t time, int32_t button,
> > enum wl_pointer_button_state state)
> > {
> > diff --git a/tests/weston-test.c b/tests/weston-test.c
> > index 03e2c54..15cd07f 100644
> > --- a/tests/weston-test.c
> > +++ b/tests/weston-test.c
> > @@ -181,13 +181,17 @@ activate_surface(struct wl_client *client, struct
> > wl_resource *resource,
> > seat = get_seat(test);
> > keyboard = weston_seat_get_keyboard(seat);
> > if (surface) {
> > - weston_surface_activate(surface, seat);
> > + weston_surface_assign_keyboard(surface, seat);
> > + if (surface != NULL)
> > + weston_surface_activate(surface);
>
> Redundant check, surface is always non-NULL.
>
> > notify_keyboard_focus_in(seat, &keyboard->keys,
> > STATE_UPDATE_AUTOMATIC);
> > }
> > else {
> > notify_keyboard_focus_out(seat);
> > - weston_surface_activate(surface, seat);
> > + weston_surface_assign_keyboard(surface, seat);
> > + if (surface != NULL)
> > + weston_surface_activate(surface);
>
> Redundant check, surface is always NULL.
>
> > }
> > }
> >
>
> Why is there no call to deactivate for the previously active surface
> here? What will call deactivate as necessary?
>
>
> Thanks,
> pq
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIVAwUBV0cPxyNf5bQRqqqnAQhmgA/+J5Or6rDCF4pVQZdYea6ErqarNFD61eG1
> 3Uhcw5CWMDJ86eS1pP6R6ovCRzdztNhwna1enk5s7zPbY4fVUKyRl5BMw+eIRuWY
> OpgaSnmP5ZlkkQWq+Ijc9mTzgaFy00nhIoCRnZ/TMJYAyLMeA9ZL7Ox28Vvmvg3N
> D5G5MGsoiVx3dQOQ9EFteP0aoqh+8gUKUiYKBo5GvWtR3nbI1QEGmvAtkBb8cKSK
> 1yuqomnIHeV9l1w0GEoDWYS+CR8+J2mqw/F5E/JY/KDAg9A7TlLazRcQI9ZHJi6e
> Kz7AttiTPuNHt+jlQLqSQD2BaXNjDyniWTYW61qD2yh+GzZ/RkODRm59XSRfC/sW
> 8RGhf5WkD3BBUUFCTldj4afoev90HCbYJG9Iz/hI13gkiji7ustkwFTU6Mx/3XdS
> mvKZ0Bz57x8Wp/YQCQ7Jat4e9pSWfsTkiP4y82QHV0Cz7EGeHHAF7N8y5LS04gbY
> NmABt3z6/rE/0RdLwxHD5D824IPOyfwURoxSOEFMHlDg4md3zDeFqoCpr+JLOiFQ
> WwytXoysD6xQRryd/PDKp2LY76UMQM81QbabKGDivvIOoe3vMwVJeNfy6Rp3dWHn
> 2Vd6jjDYxejMyBT0vclG21Brmmai6fJsWJmyaAT/nsq73IRJI/NBY8Z0YPqElHRW
> B/4ltfJylVE=
> =akz6
> -----END PGP SIGNATURE-----
Bryce
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
