[PATCH wayland-protocols v3] Add screensaver idle inhibitor protocol

Fri Jun 3 09:04:30 UTC 2016

On Fri, 3 Jun 2016 09:26:24 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Thu, Jun 02, 2016 at 02:24:20PM -0700, Bryce Harrington wrote:
> > This interface allows disabling of screensaver/screenblanking on a
> > per-surface basis.  As long as the surface remains visible and
> > non-occluded it blocks the screensaver, etc. from activating on the
> > output(s) that the surface is visible on.
> > 
> > To uninhibit, simply destroy the inhibitor object.
> > 
> > Signed-off-by: Bryce Harrington <bryce at bryceharrington.org>
> > ---
> > 
> > v2:
> >  + Rename protocol to idle-inhibit
> > v3:
> >  + Added a destructor for the idle manager
> >  + Changed sub-surface behavior to inherit idle from parent surface
> >  + Various wording changes suggested by pq
> >   

Hi,

you posted the last one marked as v3, so this would have been v4.

> Hi,
> 
> I think this looks sane, I only have a couple of questions that might
> need clarification.
> 
> > 
> >  Makefile.am                                        |  1 +
> >  unstable/idle-inhibit/README                       |  4 ++
> >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 84 ++++++++++++++++++++++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 unstable/idle-inhibit/README
> >  create mode 100644 unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 71d2632..de691db 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -8,6 +8,7 @@ unstable_protocols =								\
> >  	unstable/relative-pointer/relative-pointer-unstable-v1.xml		\
> >  	unstable/pointer-constraints/pointer-constraints-unstable-v1.xml	\
> >  	unstable/tablet/tablet-unstable-v1.xml			                \
> > +	unstable/idle-inhibit/idle-inhibit-unstable-v1.xml			\
> >  	$(NULL)
> >  
> >  stable_protocols =								\
> > diff --git a/unstable/idle-inhibit/README b/unstable/idle-inhibit/README
> > new file mode 100644
> > index 0000000..396e871
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/README
> > @@ -0,0 +1,4 @@
> > +Screensaver inhibition protocol
> > +
> > +Maintainers:
> > +Bryce Harrington <bryce at osg.samsung.com>
> > diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > new file mode 100644
> > index 0000000..af3a911
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > @@ -0,0 +1,84 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="idle_inhibit_unstable_v1">
> > +
> > +  <copyright>
> > +    Copyright © 2015 Samsung Electronics Co., Ltd
> > +
> > +    Permission is hereby granted, free of charge, to any person obtaining a
> > +    copy of this software and associated documentation files (the "Software"),
> > +    to deal in the Software without restriction, including without limitation
> > +    the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > +    and/or sell copies of the Software, and to permit persons to whom the
> > +    Software is furnished to do so, subject to the following conditions:
> > +
> > +    The above copyright notice and this permission notice (including the next
> > +    paragraph) shall be included in all copies or substantial portions of the
> > +    Software.
> > +
> > +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > +    DEALINGS IN THE SOFTWARE.
> > +  </copyright>
> > +
> > +  <interface name="zwp_idle_inhibit_manager_v1" version="1">
> > +    <description summary="control behavior when display idles">
> > +      This interface permits inhibiting the idle behavior such as screen
> > +      blanking, locking, and screensaving.  The client binds the idle manager
> > +      globally, then creates idle-inhibitor objects for each surface.  
> 
> + for each surface that should inhibit the idle behavior. ?

Yeah, makes sense.

> > +
> > +      Warning! The protocol described in this file is experimental and
> > +      backward incompatible changes may be made. Backward compatible changes
> > +      may be added together with the corresponding interface version bump.
> > +      Backward incompatible changes are done by bumping the version number in
> > +      the protocol and interface names and resetting the interface version.
> > +      Once the protocol is to be declared stable, the 'z' prefix and the
> > +      version number in the protocol and interface names are removed and the
> > +      interface version number is reset.
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy the idle inhibitor object">
> > +	This destroys the inhibit manager.

Good addition. Should probably say how destroying the manager affects
the inhibitors created from it (no effect at all)?

> > +      </description>
> > +    </request>
> > +
> > +    <request name="create_inhibitor">
> > +      <description summary="create a new inhibitor object">
> > +	Create a new inhibitor object associated with the given surface.
> > +      </description>
> > +      <arg name="id" type="new_id" interface="zwp_idle_inhibitor_v1"/>
> > +      <arg name="surface" type="object" interface="wl_surface"
> > +	   summary="the surface that inhibits the idle behavior"/>
> > +    </request>
> > +
> > +  </interface>
> > +
> > +  <interface name="zwp_idle_inhibitor_v1" version="1">
> > +    <description summary="context object for inhibiting idle behavior">
> > +      An idle inhibitor prevents the output that the associated surface is
> > +      visible on from being blanked, dimmed, locked, set to power save, or
> > +      otherwise obscured due to lack of user interaction.  Any active
> > +      screensaver processes are also temporarily blocked from displaying.
> > +
> > +      If the surface is destroyed, unmapped, becomes occluded or otherwise
> > +      loses visibility, the screen behavior is restored to normal; if the
> > +      surface subsequently regains visibility the inhibitor takes effect once
> > +      again.
> > +
> > +      Note that in the case of a compound window (a surface with
> > +      sub-surfaces), the inhibitor effects should be inherited from the top
> > +      level surface.  
> 
> Does this mean that if we have a subsurface with an inhibitor object,
> and for example an xdg_surface without one, the subsurface would still
> not inhibit the idle behavior, as it'd inherit the behavior?
> 
> If it should work like this, maybe we should make it an error to create
> an idle inhibitor on a subsurface. If it should inhibit, I think it
> needs to be clarified.

Right, I was thinking something like: Sub-surfaces that do not have an
inhibitor associated inherit the inhibition behaviour from their
parent. This applies recursively.

To clarify what I mean, lets consider an artificial case like this:

wl_surface A is a top-level, only on output 1.
wl_surface B is a sub-surface to A, only on output 2.
wl_surface C is a sub-surface to B, only on output 3.

wl_surface B has an inhibitor associated. All surfaces are completely
visible etc. on their own outputs.

Output 1 will go to powersave: no surface is inhibiting it.

Output 2 does not go to powersave: B inhibitor is in effect.

Output 3 does not go to powersave: C inherits B's inhibitor behaviour.

This raises a corner-case: If surface B becomes completely occluded so
that the compositor considers it is no longer inhibiting, what happens
with output 3? Should it powersave or not?

That is a strange use case but still possible. I'm not sure what should
happen. Should it behave like C had its own inhibitor, or should the
inheritance take also the effectiveness of the inhibitor on B?

I suppose you could pick either way.

> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy the idle inhibitor object">
> > +	This removes the inhibitor effect from the associated wl_surface.
> > +      </description>
> > +    </request>
> > +
> > +  </interface>
> > +</protocol>
> > -- 

Ok, this looks quite good, just a couple clarifications would be nice.

If you agree with my "inherited from parent" idea instead of from
top-level, you get:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

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/20160603/434ae641/attachment-0001.sig>