[PATCH weston v6 0/6] Implement screensaver/idle inhibition

[Quentin Glidic]

On 9/9/16 4:31 AM, Bryce Harrington wrote:
> Updated to include review feedback from Quentin on the v5.  That
> involves refinements to the destructor behavior, reorganizing patches a
> bit, and noting that if the idle manager is destroyed by the client, the
> individual inhibitor objects remain active.  The libweston-desktop API
> is renamed from surface_drop_idle_inhibitor to just drop_idle_inhibitor.

I’ve been trying to make up my mind of this series for quite some time 
now, and I’m sad that we hit v6 and still not have something usable.

Let’s start with global patches comments:

1. I merged it because it was nice and standalone, so we can stop 
worrying about it.
2.
3. This one seems odd to me. It patches stuff added in 2, that would be 
better squashed directly. And the protocol handling code looks too 
different from what it usually is (e.g. emitting the signal in the 
destroy request, not in the resource destroy handler).
4. As I said in v5, this patch is wrong. It will not work if you add an 
inhibitor to a popup, it will just crash (didn’t test that, but I’m 
pretty confident).
5. Mostly good, except for the libweston-desktop stuff.
6. It should allow to test for popup inhibition, and subsurface too.


To get the implementation right, we first need to define what it should do.

The first question is: when is an inhibitor “active”? The answer from 
the protocol file is:
“If the surface is destroyed, unmapped, becomes occluded, loses 
visibility, or otherwise becomes not visually relevant for the user, the 
idle inhibitor will not be honored by the compositor”
This means only visibility is taken into account. The shell has no 
checks to add to the inhibitor.

In libweston, this effectively means we can just check the 
weston_surface and ignore role-specific structures.
So all the *inhibitor* related code should be hidden in libweston.


Now, we want the compositor (the shell in weston’s case) to drive the 
fade in/out animation.

That means we need the shell to know when an *output* is being inhibited 
or not. Not individual surfaces.



I suggest a few things for the next round.
First, implement the client directly, this way we can merge it quickly 
to help other compositors to run tests. (And implement subsurface/popup 
testing.)

Then, reworking the output code to something like that (in pseudo-code):

output_set_on(output) {
   if output.compositor.inhibitor_handler != NULL
     output.compositor.inhibitor_handler(output, ON)
   else
     output.set_dpms(ON)
}

output_set_off(output) {
   if output.inhibited || output.compositor.idle
     return

   if output.compositor.inhibitor_handler != NULL
     output.compositor.inhibitor_handler(output, OFF)
   else
     output.set_dpms(OFF)
}

idle_handler(compositor) {
   for output in compositor.outputs {
     if compositor.idle
       output_set_off(output)
     else
       output_set_on(output)
   }
}

output_check_inhibit(output) {
   was_inhibited = output.inhibited
   output.inhibited = false
   for v in output.views {
     if v.surface.inhibit {
       output.inhibited = true
       break
     }
   }
   if was_inhibited
     output_set_off(output)
}

protocol_create_inhibitor(surface) {
   surface.inhibit = true
   for v in surface.views {
     output_check_inhibit(v.output)
   }
}

protocol_destroy_inhibitor(surface) {
   surface.inhibit = false
   for v in surface.views {
     output_check_inhibit(v.output)
   }
}

The shell/compositor would set the compositor.inhibitor_handler to drive 
fade in/out.
Does that seem sensible to you?

Cheers,

-- 

Quentin “Sardem FF7” Glidic