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

Fri Jul 1 09:26:55 UTC 2016

On Thu, 30 Jun 2016 12:45:51 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Thu, Jun 30, 2016 at 02:13:45PM +0300, Pekka Paalanen wrote:

> > 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.

Hi Bryce,

there is a reason to tinker with it, and I explained it below, but only
if you insist on tracking surface activeness in weston core.

> > 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".

The patch has your name in it, and no-one else's.

> > 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.

Active per seat is a good idea, because of its close relationship to
keyboard focus.

It has nothing to do with tracking input activity.

I also cannot see why idle-inhibit would need any tracking per seat
whatsoever.

> > 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.

There is the activate_signal. That needs to become per-seat, if you
track the active surface per seat.

> > 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.

I believe you have the right to quote private discussions in public if
you see it as appropriate.

I have given you a suggestion on how to land the idle-inhibit. Please
use it. I reiterate that suggestion below.

> 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

This is where we disagree.

In my opinion, both focused and unfocused surfaces (or active vs.
inactive) should be able to inhibit on the same terms. Therefore I do
not see a reason to track "activeness" in weston core, as I have
repeatedly said. When you don't need to care about "activeness", we
don't need to fight over what it means either, and you can leave
activate_signal as is.

Backgrounded (occluded?) or minimized is a completely different thing.
Minimized surfaces are not in view_list, which means they will not be
considered at all during output repaints or in your
weston_output_inhibited_outputs(), which means they automatically lose
their ability to inhibit. You don't have to add anything special to
make that happen.

If backgrounded means "completely occluded", that we do not have
readily available, and shells do not know it either. That information
must be extracted in the output repaint path. I think that can be left
for later.

Using the view_list to find inhibiting surface already automatically
covers most cases, where you don't want a surface with an inhibitor to
inhibit: windows that are minimized, hidden, on a non-visible virtual
desktop, etc. Furthermore, using the view->output_mask like you did
will also automatically exclude all surfaces that are outside of any
outputs. The only thing not handled by this is occlusions, which must
be computed from the scenegraph with the help of opaque regions.

Your patches to implement idle-inhibit are almost ready. You only need
to drop the unneeded patches!

> 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.

I believe we can get idle-inhibit in its basic form landed pretty
easily, if you just drop the "active" stuff completely.

Remove the use of weston_surface::active, and drop the patches adding
it.

I still stand by my claim, that you asked how to track active surfaces
and I explained exactly that to you. You had *already* decided you need
it for something, and I didn't care what you needed it for. I just
explained the proper way to get it in my opinion.

> > 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

Are you sure you were not asking in the context of "activeness"?

"Active" is a property that is visible to the client somehow (keyboard
focus and shell-specific protocols), therefore it must be on the
surface.

A flag that only considers idle-inhibiting would be per-view, because
you might have different kind of views that are not all equal wrt.
inhibiting.

> 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).

A tangible scenario: live thumbnail views. But Weston does not do that
itself, you can perfectly leave that fight for the time when someone
needs it. There are more things to do to have nice live thumbnail
views anyway. You can ignore that for now.

You don't need to reimplement everything. Just drop the things we don't
need, we can get the feature landed, and practically all users will be
happy using it.

> 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*.

In that case, please do as I suggested: drop all uses of any new flags,
activeness or alike.

That is why I have been recently stressing so hard on the question of
why do you need activeness at all. We've been fighting over a thing you
don't need, but rather than just flat out saying "drop that shit, it's
not useful", I gave you the opportunity to explain why you want it,
in case I just didn't understand. You ignored the question and
continued on the details of how, assuming idle-inhibit cannot live
without inspecting "active".

But now it seems there really is no need for activeness tracking in
weston core at all, so just drop that part of the series and be happy
you don't have to fight with it anymore. The work lost is lost, now cut
your losses and get the idle-inhibit feature in a shape where I can
land it.

Please.

I would really like to land it.

Now that I read again my reviews, it seems there were more questions
about why do something at all (patch 1). I do not understand why you
carry patches adding things you never use.

How about you keep patch 3, the protocol implementation from patch 6,
and patches 7 and 8, and drop all the rest for a start? Addressing the
review comments for what remains shouldn't be anyway near as hard as
this.

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/20160701/73e215ad/attachment.sig>