[PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface
Pekka Paalanen
ppaalanen at gmail.com
Tue Jun 28 09:15:49 UTC 2016
On Thu, 23 Jun 2016 17:32:50 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> On Thu, May 26, 2016 at 06:01:27PM +0300, Pekka Paalanen wrote:
> > On Thu, 7 Apr 2016 16:44:17 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> >
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >
> > 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.
Hi Bryce,
you're right, it's not a new signal, but right now in master it is
being emitted from weston_surface_activate().
Can you explain why it does not belong in weston_surface_activate()?
Is the signal misnamed?
If it is misnamed, just explaining what it actually is in the commit
message would be a perfect fix for this review comment.
>
> > > +}
> > > +
> > > +/** 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.
I suppose I was confused because this patch is already patching up all
the shells, but only doing half the job.
Would it be possible to have the old and the new code existing in
parallel, like we did with the backend config migration?
First you'd add weston_seat_set_keyboard_focus(), and since
weston_surface_activate() is a name already taken, add
weston_surface_{set,unset}_active() for example. The new name is
warranted, because you allow having multiple surfaces active at the
same time regardless of seats. Then you can convert each shell in its
own patch, which would be nice to review as the conversion is completed
in one go for each shell. Finally, you would delete
weston_surface_activate() as unused.
In fact, as weston_seat_set_keyboard_focus() is identical to the old
weston_surface_activate(), the initial implementation of
weston_seat_set_keyboard_focus() could simply just call
weston_surface_activate(). Then in the final step,
weston_surface_activate() would get essentially renamed to
weston_seat_get_keyboard_focus().
I followed a similar approach in my series for converting tests to use
create_shm_buffer_a8r8g8b8(), and Armin used the approach in reworking
the surface/view mappedness tracking, which I am yet to review and
merge.
But before you do that work, write down a justification for the
activeness tracking feature this patch is adding, please. It definitely
belongs in the commit message, and if you ever explained it to me, I
have long forgotten. Even more so, because your patch series does not
add even a single user of weston_surface_is_active() - oh wait, you
used the flag directly in patch 3.
*If* using weston_surface_is_active() is a good thing to do, using it
already in patch 3 makes use of known broken state until all the
patches to fix the shells.
> > > static void
> > > subsurface_set_position(struct wl_client *client,
> > > struct wl_resource *resource, int32_t x, int32_t y)
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160628/4ee7ad9d/attachment-0001.sig>
More information about the wayland-devel
mailing list