Why track window activeness? (Re: [PATCH weston v3 2/8] compositor: Track inhibition state in weston_surface)
Pekka Paalanen
ppaalanen at gmail.com
Thu Jun 30 08:49:47 UTC 2016
On Wed, 29 Jun 2016 16:57:31 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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.
> > > >
> > > > > 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".
Hi Bryce,
very good! That explanation belongs in the commit message which you had
left completely empty. Normally that would have been reason enough to
just outright send the patch back without even looking further, but I
wanted to be nice and review it anyway, thinking maybe I'd figure it
out while reading the code - obviously I didn't. The patch title had
nothing to with the patch content, so that didn't help either.
> > 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.
I don't see how it would be any really different amount of work, doing
what I outlined below.
The fact that we are even having this conversation is proof that it is
not straightforward. Or maybe it would have been, if you had actually
written the commit message.
> 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.
Ok, let's see how that'll look like. Please mind, that changing the
meaning of an exported symbol without changing its signature will make
it much harder for external project developers to realize it has
changed, since there won't be even compiler warnings.
I cannot guess what you are up to, if you don't write good commit
messages.
Remember that I am continuously reviewing patches to almost all parts
of Weston when I can, so I am switching context by the hour, and I
usually cannot recall long past discussions. And that's not even my
main job either, I have other projects to care for, and I'm sure you do
too.
The nagging I am giving you here is what anyone would give you if they
would need to understand your patch without any prior discussions. That
anyone could even be you after a few months, bisecting to find the
cause for some regression no-one could have foreseen. Or, coming back
to revise a patch series that still hasn't been merged.
Bryce,
do not dismiss *the most important question* here:
Why is this activeness tracking needed?
I made this question in my comments to patch 3.
Again, I do remember you asking how to get the active surface(s) and I
explained, but I do not recall questioning what you needed it for.
Sorry, obviously I should have asked already back then.
If I remain unconvinced that activeness needs to be tracked and used,
and no-one else votes for your patch, the activeness tracking patches
will get rejected by me.
Are there any other use cases coming for it than idle-inhibit?
At this point, to get your idle-inhibit feature in, may I suggest to
not include any of the activeness tracking? Just drop the window
activeness check from the idle-inhibit path. We can get that landed,
and afterwards we can continue about the activeness tracking if
necessary.
> > 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.
> >
Sincerely,
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/20160630/5e115808/attachment-0001.sig>
More information about the wayland-devel
mailing list