[PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface
Bryce Harrington
bryce at osg.samsung.com
Wed Jun 29 23:57:31 UTC 2016
On Tue, Jun 28, 2016 at 12:15:49PM +0300, Pekka Paalanen wrote:
> 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?
Yes
> 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.
I can see you're confused, yeah. Amusingly this is something you more
or less suggested yourself. It's just that a couple separate things are
being done here, and you're conflating it into something bigger I guess,
and then wondering why there isn't more to this.
The old weston_surface_activate() actually had nothing to do with
surface activation - it handles keyboard focus assignment; thus it is
misnamed (as you pointed out yourself early on). So first change is a
rename; you didn't like weston_surface_assign_keyboard and suggested
weston_seat_set_keyboard_focus instead. Second, the old implementation
had been in input.c since despite the name it was mostly keyboard
functionality, but it's now moving to live with the other seat
functionality. Third, with the old function renamed we're recycling the
old 'weston_surface_activate' to be used for something that actually
"sets active on a surface".
> Would it be possible to have the old and the new code existing in
> parallel, like we did with the backend config migration?
No, that'd be way too much work for something that is actually pretty
straightforward.
Instead of what you suggest I'm going to split the rename and move into
its own patch. Then it'll be clearer we're just renaming existing
functionality and then recycling the freed up function name for
something different, but more spoon-fed to the reviewer.
> 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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIVAwUBV3JATCNf5bQRqqqnAQh8BxAAkK0NCT3VzlxFc2Vj7N7kvKNAlF+EEcNI
> 3Zuz810qVyc3DpP50TwaimYjcqCf0FV9xov4tc4CmzrFohqiW9ObrET3xIkhRhXu
> PY2PAs55XW/2Du22OobQ13VJ4QDpShD7leYSMX3iQWKeOzQXglXdz81uWv2OLIa5
> QTSjRkq2aK+dA8ESnMYAbTe2WBwV3UinWMd12LoL5dYfUeCzbYNUJJKAoaEtV9aH
> ho3Bf2BQE/OurN6dx6DviXOMiMnkcHRMIOrAQB+iirevgP8UOUSfdTd0ry9PLUxS
> iFgjijmmmwLpyUGDAT/amyCVNxkt/eFONohZyIiVCKQkp1gIBhxOy54ZVMBs+b5p
> ++z+qzvKhccsTvvCR8uD2LeZ5ZQRyQwK3qhAq0PdCu2ArLV150tYRa5vNbiTmuoo
> qFnPx7ljQI44RgA5mPipf4Zn4XDQCoZILLY1jUSKtTbHo3nAfgo7qFnB3mJwqbMS
> uqZunRgrDrGhvVotI8x3C2rSXE8bmLUlXWOQCjku8BLBkuM57hwb8M7QALnWak7N
> d3OJlI9GrWtfFU5oEV+3tBNJpU7e8PshETisWa8TNs7WCnNsn+BCAuFUonSb3Vmp
> VRtPmdH4nN3usSNJ5TOVx2nzETfRGcISkluKWXhJtZ20Rg+WSxBti9RPZOXXNkD+
> UjCbl1VwSzo=
> =R35M
> -----END PGP SIGNATURE-----
More information about the wayland-devel
mailing list