[PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface

Bryce Harrington bryce at osg.samsung.com
Fri Jun 24 00:32:50 UTC 2016


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.

> > +}
> > +
> > +/** 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


More information about the wayland-devel mailing list