On Fri, 3 Jun 2016 10:39:24 -0500 Yong Bakos <[email protected]> wrote:
> Hi, > > On Jun 3, 2016, at 4:04 AM, Pekka Paalanen <[email protected]> wrote: > > > > On Fri, 3 Jun 2016 09:26:24 +0800 > > Jonas Ådahl <[email protected]> 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 <[email protected]> > >>> --- > >>> > >>> 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 <[email protected]> > >>> 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? Hi, that request is wl_registry.bind, as for all globals. > >>> + <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? Yes. > >>> + </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. Yes, they must be associated to surfaces. > >>> + > >>> + </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.' An ordinary client must not be able to do that. > 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. If you cannot see the widget anyway, it must not be able to affect screen saving. Therefore by definition, surfaceless clients must not be able to inhibit. > Furthermore, I don't believe that inhibition should be coupled to outputs. > See below. It should work per-output. If there is important info on just one output, why should the other outputs also stay on for no good reason? > >>> + > >>> + 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. That is explicitly not what we want to enable. What is this "caffeine" you talk about? I've never heard of it. Maybe I never had to fight against screen saving. > > 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 <[email protected]> > > > > > > Thanks, > > pq > > Forgive me if I didn't quite grok prior conversations about this protocol. It seems like your mindset is tuned to fighting against bad screensaving policies in a compositor, while Bryce's goal with the extension is to allow implementing proper and good screensaving policies *in the first place*, so that no fighting is needed. We want the compositor to behave correctly, not offer tools to override the compositor behaviour. Bugs should be fixed at their source, not worked around at every other place. Thanks, pq
pgpKvXFeJBcM4.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
