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

[Bryce Harrington]

On Wed, May 18, 2016 at 04:11:39PM +0300, Pekka Paalanen wrote:
> On Thu, 24 Mar 2016 11:14:33 -0700
> Bryce Harrington <bryce at osg.samsung.com> 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.
> 
> Hi,
> 
> ok, that sounds good.
> 
> > Signed-off-by: Bryce Harrington <bryce at bryceharrington.org>
> > ---
> > v2: Rename protocol to idle-inhibit
> >  + Use "idle" rather than "screensaver".  People likely to be interested
> >    in this protocol would know the difference between "blanking", "dpms",
> >    and literal "screensavers", so don't lead them to think that this only
> >    deals with the latter.  The protocol addresses all manner of idle
> >    behaviors, so is a more accurate name.
> >  + Standardize nomenclature to "idle-manager" and "inhibitor" for clarity.
> >    "inhibition_inhibit" was just too cumbersome.
> 
> Alright!
> 
> > 
> >  Makefile.am                                        |  1 +
> >  unstable/idle-inhibit/README                       |  4 ++
> >  unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 78 ++++++++++++++++++++++
> >  3 files changed, 83 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 5926a41..ac497d8 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -5,6 +5,7 @@ unstable_protocols =								\
> >  	unstable/text-input/text-input-unstable-v1.xml				\
> >  	unstable/input-method/input-method-unstable-v1.xml			\
> >  	unstable/xdg-shell/xdg-shell-unstable-v5.xml				\
> > +	unstable/idle-inhibit/idle-inhibit-unstable-v1.xml	\
> >  	$(NULL)
> >  
> >  nobase_dist_pkgdata_DATA =							\
> > 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..a8c8a85
> > --- /dev/null
> > +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> > @@ -0,1 +1,78 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="idle_inhibit_unstable_v1">
> > +
> > +  <copyright>
> > +    Copyright © 2015-2016 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.
> > +
> > +      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="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>
> 
> This interface is missing a destructor. It is the only way the
> compositor could destroy the wl_resource before the client disconnects.
> 
> All interfaces need a way to destroy the object.
> 
> > +  </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 surface is visible on
> 
> "the associated surface", to be crystal clear.
> 
> > +      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, disabled, becomes occluded or otherwise
> 
> What does "disabled" mean? Are you looking for "unmapped"?
> 
> Should we say "completely occluded"?

I am not certain that we should limit it to that, in case a compositor
wanted to have a partial occlusion state be reason for not honoring
inhibition requests.  But it's hard to conceptualize the potential use
cases here.

> > +      loses visibility, the screen behavior is restored to normal; if the
> 
> I'd change "the screen behavior is restored to normal" to "the
> inhibitor becomes ineffective". Restoring to normal is a bit vague, and
> might not even be true if there are other inhibitors in effect.

Hmm, how about "the inhibition request will not be honored by the
compositor?"

> > +      surface subsequently regains visibility or is unoccluded, the inhibitor
> > +      takes effect once again.
> 
> Strike "or is unoccluded", it's already covered by "regains
> visibility", right?
> 
> > +
> > +      Note that in the case of a compound window (a surface with
> > +      sub-surfaces), a separate inhibitor object may be needed for each
> > +      wl_surface.
> 
> In case of sub-surfaces, inheriting inhibitor effects would be much
> better than having to explicitly set every wl_surface, as pointed out
> in earlier reviews. I should have given that comment the first time.
> 
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy the idle inhibitor object">
> > +	This restores screensaver/blanking/et al behaviors to their normal
> > +	state and destructs the idle inhibitor object.
> 
> Or just: "This removes the inhibitor effect from the associated
> wl_surface."
> 
> > +      </description>
> > +    </request>
> > +
> > +  </interface>
> > +</protocol>
> 
> Otherwise just wording nitpicks, but two things I would like to see
> corrected:
> - the missing destructor
> - interaction with sub-surfaces

Apart from the questions above I've incorporated the other feedback in
the v3 patch.

Thanks,
Bryce

> Thanks,
> pq

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIVAwUBVzxqFyNf5bQRqqqnAQjX/A/9GCVWmQOJfmL70+s/K/778gOvAjPeYZX0
> wkCr7uNswvV5yh4Hv4Jb/gFPEzGOvvdNkZsItvY+g3VaxB9lAxblny5cEGehfOVf
> ZTkwUfuBktaCn7QKyxzKaf157X4mu4auJ4NumVIx9gonRtyCHPHs66LOYdxmOwcI
> fE1n0rP0RhCTfyF/wEBxy3SYRGnTDChQExH/0zjeIbOibfiE+tnF+K+pVQdpseLh
> QFZW6l8I3VQ6uxtlHcAizMLxuqgqrXGzsaHlH/3lQQhhzCjChKHleQIFAaT9BrWl
> e4In7JGkaRy/KuwpdCgUbzBh+1qqyxtn48TDUpxFzKCG0XM0cNLJpcPvQTdPuz+p
> Pue/xr0P9Exi5Rd8uQdYfm8ToKBRq2gv6EWQYVT/XslcV2aH6jSYRpxLqihTf+s2
> JNA3g9TyuEc0EESOv214W2g9fPvHkkeB/CDxQuSVn8GURQwMDhT6sysJlAaE9k/B
> gJUylulmxnygMi2QD/Ie/yEMiqveHPTajRQzww1uMMG/uuEC2eKIiLiIhzVY0lDu
> ZjYEloXqItWB8ihUPuHPPbC9SQtvZLaMH5swHxiXpyMYVy2CeMtef33xuMe067Af
> HcKe8GEg1cp5s9Hze1tiCFQzcRWuA2twBJKbMpxoeEWwC4R7aLZSdu1ZvxoFhVfj
> /s01GBo0gyM=
> =ayxG
> -----END PGP SIGNATURE-----