On Thu, Jun 30, 2016 at 02:13:45PM +0300, Pekka Paalanen wrote: > On Wed, 29 Jun 2016 19:04:07 -0700 > Bryce Harrington <[email protected]> wrote: > > > Place it with the other weston_seat functions. > > > > Signed-off-by: Bryce Harrington <[email protected]> > > --- > > 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----- _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
