[PATCH weston 2/2] input: Move weston_seat_set_keyboard_focus and document

[Bryce Harrington]

On Thu, Jun 30, 2016 at 02:13:45PM +0300, Pekka Paalanen wrote:
> On Wed, 29 Jun 2016 19:04:07 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
> 
> > Place it with the other weston_seat functions.
> > 
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > ---
> >  libweston/compositor.h |  7 ++++---
> >  libweston/input.c      | 34 +++++++++++++++++++---------------
> >  2 files changed, 23 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libweston/compositor.h b/libweston/compositor.h
> > index 5701a05..49ef063 100644
> > --- a/libweston/compositor.h
> > +++ b/libweston/compositor.h
> > @@ -1167,9 +1167,6 @@ int
> >  weston_spring_done(struct weston_spring *spring);
> >  
> >  void
> > -weston_seat_set_keyboard_focus(struct weston_seat *seat,
> > -			       struct weston_surface *surface);
> > -void
> >  notify_motion(struct weston_seat *seat, uint32_t time,
> >  	      struct weston_pointer_motion_event *event);
> >  void
> > @@ -1717,6 +1714,10 @@ weston_seat_get_pointer(struct weston_seat *seat);
> >  struct weston_touch *
> >  weston_seat_get_touch(struct weston_seat *seat);
> >  
> > +void
> > +weston_seat_set_keyboard_focus(struct weston_seat *seat,
> > +			       struct weston_surface *surface);
> > +
> >  #ifdef  __cplusplus
> >  }
> >  #endif
> > diff --git a/libweston/input.c b/libweston/input.c
> > index e8c060e..8f46698 100644
> > --- a/libweston/input.c
> > +++ b/libweston/input.c
> > @@ -1297,21 +1297,6 @@ notify_motion_absolute(struct weston_seat *seat,
> >  }
> >  
> >  WL_EXPORT void
> > -weston_seat_set_keyboard_focus(struct weston_seat *seat,
> > -			       struct weston_surface *surface)
> > -{
> > -	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);
> > -}
> > -
> > -WL_EXPORT void
> >  notify_button(struct weston_seat *seat, uint32_t time, int32_t button,
> >  	      enum wl_pointer_button_state state)
> >  {
> > @@ -2763,3 +2748,22 @@ weston_seat_get_touch(struct weston_seat *seat)
> >  
> >  	return NULL;
> >  }
> > +
> > +/** Sets the keyboard focus to the given surface
> > + *
> > + * \param seat The seat to query
> > + */
> > +WL_EXPORT void
> > +weston_seat_set_keyboard_focus(struct weston_seat *seat,
> > +			       struct weston_surface *surface)
> > +{
> > +	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);
> > +}
> 
> Hi Bryce,
> 
> both pushed:
>    a5bb91d..24f917e  master -> master
> 
> 
> The activate_signal will be a lot more difficult to handle. The only
> in-tree user of it is xwayland's weston_wm_window_activate() which
> assumes there can only be one surface active at a time, and it's not
> even per seat, it's really just one. There might also be other users
> out there I do not know of.

I was hoping in breaking this patch out separately that you'd see that
I'm not introducing this signal or using it for inhibition; it's
existing code, and doesn't need tinkered with beyond this rename.

Yes, the signal is mis-named but that's a legacy issue unrelated to this
patch.  Weston is rife with poorly chosen names already, so this is
nothing particularly out of the ordinary.  Ideally there'd be some
documentation explaining what it is, but again, its lack of
documentation is hardly unique either.  And unless there's a clear
*technical* reason why I'd need to tinker with it as part of the
inhibition work, I don't want to scope creep this patchset to deal with
either of those problems.

> If we go with your plan to be able to have arbitrary many surfaces
> active at the same time, without changes XWM would raise and activate
> the most recent surface set active, regardless of what is already
> active. Is that desireable, I do not know.

This is the second time you've asserted that this is "my plan".

> To be able to support any other kind of behaviour, we'd need signals for
> both activate and deactivate. Or, we have to decide that you can only
> have one surface active per seat,

I have no problem with limiting it to one active surface per seat.
Indeed I was leaning that way to begin with by tracking input activity
on a per-seat basis but you've balked at that.  I still think that's the
better way to go.

> in which case we'd have a per-seat
> activate_signal, and the active surface would be tracked just like
> keyboard focus except in struct weston_seat, not in struct
> weston_keyboard.

I would need better convincing that signals need to be involved.  To me
this seems like not needing to be so sophisticated a production.  I'm
not opposed to using signals here but this is not that elaborate of a
feature that it needs to be turned into a major production just to get
implemented.  And none of this is protocol, it can be refined later
after the basics have landed.

> Looks like all of weston_keyboard, weston_pointer, and weston_touch
> have their own focus_signals, too, which are naturally conditional on
> having the capability in the seat.
> 
> These are the design questions we need to solve to add activeness
> tracking in Weston core. If we put "active" as a thing in the core,
> how should it behave?
> 
> Therefore I'd suggest to reconsider "activeness" for idle-inhibit. If
> you just want a shell-controllable surface flag for "may inhibit idle
> triggers", then we should not talk about active because active is
> something else.

I would quote you as being the one that directed me on this path to
begin with, but that was a private discussion.  Needless to say, I'd
like to finish this implementation and get it landed rather than start
over from scratch yet again.

If you can suggest a better name than "active" I'm all ears, but there
needs to be a way to distinguish between surfaces the user is actually
interacting with (and thus allowed to manipulate the screensaver), and
backgrounded/minimized/unfocused surfaces that shouldn't.  The design
I'm implementing is the one you suggested to me, and as the
implementation is mostly done (IMHO) suggestions to rethink that part of
the design is going to require tossing out a lot of time investment and
starting over, which obviously I'd like to avoid.  Again, a lot of this
is just internal implementation details that can be refined later; I
would like to just get this landed.

> Then I'd also ask why is it a surface and not a view.

Coincidentally (and evidently you've forgotten) but I asked that exact
same question of you at the inception of this; surfaces were the result
of that discussion.  Unless there's a particular *tangible* scenario
where inhibition should be different for one view of a client's surface
vs. another, changing the design at this point just causes excessive
work to reimplement everything.  I'm not opposed to making it view-based
instead of surface-based if a compelling case could be made for why to
do it that way, but surface-based is going to cover all use cases I can
think of and will be simpler (and thus less error-prone).

Changing from surface to view later on would yes, unfortunately be a
protocol breakage, but the idle inhibit protocol is intentionally being
described as unstable and versioned so we're already giving ourselves
latitude for changing that later if needed.

Pekka, I appreciate your review attention and thought to making this
design better, but at this stage what I really need is support in
getting this *landed*.

Bryce

> 
> Thanks,
> pq

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBV3T+6SNf5bQRqqqnAQiBSxAAkEO8XhpZ2szCGQwu6oe6Ua81NUIy8jwS
> yigueLNDEWv4mPS6TjUppMC3TXCghqaYTy2hkVleZSCLyNTE26G9K/YN7WcTnvEM
> uN2HgfvAJ8jTS7D7uetVOC56lEHkv/NL4jUqWULG7XNljAyrcTnnEYsSpBZN70E4
> IE+vF6DXj74xumNizSKw75IHscp8hIxarIZsnvWn5RGuDfvxE1Q+M5+xJBiNWfqq
> 1N6fkVooEKlH3QTbAE/fktfpKBosjFWeNYhVhinZOwPQUkeCPfpFSn5W3JDa55Ru
> wq7SZDr5CK23crmFU6jmOxq3AiH/zlVPehXqBersJpiMIBMbSkNVE7TFqhKo7rKX
> h77aV1J6yzZa0DI5B3C6qkOlVDBhPMYWAKrDmRfDbdRz4bzvRD3ipXhgagxQ0pul
> /WRnQzXzfmOWopY5I80EeVjeXvADGkqUxp0UIt568lXUZo32+A3OldkTmHEk5XR4
> c8zk0TzsSH5w13lEdC0wb2aLv6ereA7swfZe/1E110jkUHeGMSKMQJA5LqRE+wLt
> tFRPpInGWui4wvlePiqHOh0SCJwpZwodG+bINSoCASVr6Ys4Evujn1Q7OvDgJM++
> YvXwCxk5NrjLTUcP54PeCg9IjdOyt0kZQmsRn30TAuFmYY6fsVoGT2Empcfxv4nY
> DZGUP1BIZQg=
> =TiPd
> -----END PGP SIGNATURE-----