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

[Yong Bakos]

Hi,

On Jun 3, 2016, at 4:04 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> 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>
>>> +

Shouldn't there be a "create" request so clients can obtain the manager object?


>>> +    <request name="destroy" type="destructor">
>>> +      <description summary="destroy the idle inhibitor object">

destroy the idle inhibit manager


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

Agreed w/ pq. But, is the inhibit manager a /singleton/ global?


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

Does it make sense to associate inhibitors to surfaces, rather than clients?
See below.


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

I don't believe this is a good choice. Imagine the case of a surface-less
'inhibitor daemon.' There may be no visible surface (is my thinking out
of scope here?). Imagine another case, that of a "caffeine" widget. This
widget's surface would be hidden when another app is fullscreen.

Furthermore, I don't believe that inhibition should be coupled to outputs.
See below.


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

I don't believe that inhibition should /only/ be coupled to outputs.
In the case of a "caffeine" widget or daemon, the use case is to
prevent /all/ outputs from idling, regardless of what output the widget's
surface resides upon.


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

Forgive me if I didn't quite grok prior conversations about this protocol.

Thank you,
yong