Created attachment 232736 [details]
a diagram

Some irc dicussion between Bastien, Allan, Giovanni and me led to this diagram.
It is not quite perfect - e.g it misses the 'shield down, but not locked yet' state, which Bastien wants.

As user-visible settings, we have a proposal for

Brightness [slider] (in the power panel)
Dim Screen when inactive [switch] (in the power panel)
Lock Screen After [timeout combo]
Require pin/password After [timeout combo] (in the privacy panel)

Bastien would prefer just tie 'dim screen when inactive' to ac/battery
Allan would prefer to not have 'Lock Screen After'

Additional considerations for the dimming part:

- the dim timeout should depend on battery supply (ie be more aggressive on low battery)

- we should set a 'nominal brightness' depending on ac/battery. The brightness slider should adjust the nominal value, so it is possible to override.

Additional considerations for blanking:

- The slow fade-and-blank should happen before the shield. If you wiggle while the fade is going, you get back to the session. If you wiggle after the fade is complete, you see the shield. This only on battery. On ac, the shield would come down first, with blanking happening later.

- We need a separate (more aggressive) blanking timeout when the shield is up, to save power.

As mentioned on IRC, and as a comment on Allan's upcoming diagram:
- not having the shield coming down as an animation makes it harder for the user to understand the shield metaphor
- the diagram doesn't include a separate shield/locking timeout (eg. allow to only have to enter a password if the shield has been down for a certain amount of time)

Created attachment 232773 [details]
another diagram

Here's an updated version of the diagram. For me, there are four relevant user visible settings that should be at play here:

 Power:
  - Brightness [slider]
  - Dim Screen when Inactive [switch]

 Privacy:
  - Lock Screen After [timeout combo]

 Users
  - Automatic Login [switch]

There are also two timeouts which aren't user preferences:

 - dim screen timeout
 - screen blank timeout

Both of these can be a predefined time that is adjusted according to the battery level.

Here is the behaviours that I currently see:

gsd:

the power plugin sets up 3 idle watches:

- idle-dim-time: after this time, and depending on the idle-dim-ac/battery setting, it sets the brightness to idle-brightness

- sleep-display-ac/battery: after this time, it turns the primary monitor and the keyboard backlight off

- sleep-inactive-ac/battery-timeout: after this time, the machine is suspended

There's complicated interactions where it enforces that the sleep-inactive timeout is at least as large as the sleep-display timeout (why ?), where it also factors in some hardcoded delay for the fade-to-off that happens when the screensaver is activated.


gnome-shell:

The screen shield code lowers the shield as soon as the user goes idle. It respects the lock-delay key and only requires unlocking after that delay.

So gsds dimming and blanking does not really work as intended at all, since the shield is already down by the time it kicks in.


Initial suggestions:

- move the turning off of the monitor(s) to the shell - we want this to happen shortly after the fade which is done by the shell, and trying to synchronize the idle watches between the shell and gsd is error-prone and pointless

- remove the dim timeout on the gsd side - instead, dim the brightness right away when the user goes idle, and turn off the keyboard backlight.

- add a timeout on the shell side before the shield comes down. Without that, the dimming is pointless

- remove the interaction between the sleep-display and sleep-inactive timeouts in gsd

Alternatively, if you prefer screen blanking to remain in gsd, we should tie it to the org.gnome.ScreenSaver.ActiveChanged signal. Still have to factor in the fade time then, since the shell emits ActiveChanged before the fade. It would still be easier than syncing idle timers

ok, giovanni cleared up one point where I was confused here.

the shield code lowers the shield when the session status changes to idle, which effectively means it uses the idle-delay setting as the 'blank timeout', relative to the last input. and the dim-timeout and various other timeouts that are used in g-s-d are all relative to the last input as well. I was confused and thought they were relative to the session status change to idle, which would have had the consequences I described above.

In the light of this clarification, I'm retracting the recommendations in comment 6 and 7.

From what I understand we could implement this with:
- Dimming. This is quite simple, we'd dim the screen after a short idle delay (dim-timeout). This would only be effective when the screensaver is not active (shield down) as we want to blank the screen without going through dimming when the shield is down. Implemented in g-s-d.
- Blanking. This will happen after a short idle delay when the shield is down (blank-timeout). We wouldn't blank when the screensaver is not active. This happens in g-s-d as well and the timeout will be aggressive (10, 20 secs?)
- Suspend/hibernate: This is implemented similarly to blanking, bound to idle as well.
- gnome-shell will pop the shield down when the session is idle. The animation is visible because the screen might (at worst) have dimmed, or would be on. This works already.
- gnome-shell will handle the idle timeout to activate the password/pin, if locking is requested (otherwise it would just show the shield without a lock).

Does that sound correct?

(In reply to comment #10)
> From what I understand we could implement this with:
> - Dimming. This is quite simple, we'd dim the screen after a short idle delay
> (dim-timeout). This would only be effective when the screensaver is not active
> (shield down) as we want to blank the screen without going through dimming when
> the shield is down. Implemented in g-s-d.
> - Blanking. This will happen after a short idle delay when the shield is down
> (blank-timeout). We wouldn't blank when the screensaver is not active. This
> happens in g-s-d as well and the timeout will be aggressive (10, 20 secs?)
> - Suspend/hibernate: This is implemented similarly to blanking, bound to idle
> as well.
> - gnome-shell will pop the shield down when the session is idle. The animation
> is visible because the screen might (at worst) have dimmed, or would be on.
> This works already.
>
> - gnome-shell will handle the idle timeout to activate the password/pin, if
> locking is requested (otherwise it would just show the shield without a lock).

Sounds all fine so far. There's a question of what smarts to apply to the non-user settable timeouts.

A patch to add a 'Dim Screen when Inactive' switch is in bug 691582

Reading up on recent changes to the timeout machinery in bug 668703, I can now confirm that things work ok as long as

session.idle-delay == power.sleep-display-ac == power.sleep-display-battery

In this case, gnome-shell lowers the shield after idle-delay, and then gsd blanks the screen after sleep-display-* + FADE.

Created attachment 233286 [details] [review]
power: Don't reconfigure idle timeouts when the user goes idle

This seems unnecessary, and can have the unfortunate side-effect
of making the timeouts larger than configured, causing them to
be out of sync with what gnome-shell is doing.

Created attachment 233287 [details] [review]
Don't double/quadruple/... timeouts

This code is meant to handle a very unlikely situation that login
takes longer than the blank timeout. But it can have unintended
side-effects where it causes timeouts to become much longer than
configured, just because an idle_configure call was triggered
when the system was already idle.
Remove this code on the basis that a system where login takes
longer than 600 seconds (the default blank timeout) is not suitable
for running GNOME in the first place.

Created attachment 233288 [details] [review]
power: Don't dim when idle is inhibited

We expect movie players and similar to inhibit idle as a way to
prevent the screensaver from kicking in. We may even get firefox
to do this for HTML5 video. But constant screen dimming is just as
disruptive to the movie experience, so I think we should not do it
when idle is inhibited.

Created attachment 233289 [details] [review]
power: update idle config when plugging in or out

The idle configuration depends on the ac/battery status - we
choose different settings for some things. Therefore, we need
to refresh the configuration when the battery status changes.

Created attachment 233292 [details] [review]
power: Aggressive blank timeout when the shield is down

When the screen shield is down (as indicated by the screensaver
being active), blank the screen more aggressively, to conserve
power. To not contribute to the over-configurability of the
power plugin, the timeout value of 60 seconds is fixed.

Review of attachment 233292 [details] [review]:

> To not contribute to the over-configurability of the
> power plugin, the timeout value of 60 seconds is fixed.

The code says 30 though :)
Which a good amount IMO.

::: plugins/power/gsd-power-manager.c
@@ +3343,3 @@
+                       gpointer user_data)
+{
+        GsdPowerManager *manager = GSD_POWER_MANAGER (user_data);

Put GsdPowerManager directly in the arguments.

@@ +3372,3 @@
+        }
+        g_signal_connect (manager->priv->screensaver_proxy, "g-signal",
+                          G_CALLBACK (screensaver_signal_cb), manager);

You should initialise screensaver_active here, in case gnome-settings-daemon restarts in the session while the screensaver is active.

Review of attachment 233286 [details] [review]:

Well, we actually don't want to use that code because gnome-shell will tell us when the session goes idle *and* the screensaver is down.
So while the patch is correct, I don't think the commit message is very helpful.

Review of attachment 233289 [details] [review]:

How did that ever work...

::: plugins/power/gsd-power-manager.c
@@ +3992,3 @@
         g_signal_connect_after (manager->priv->up_client, "changed",
                                 G_CALLBACK (up_client_changed_cb), manager);
+        g_signal_connect (manager->priv->up_client, "notify::on-battery",

I don't see the on-battery property in the API docs for UPClient.
Using upower-0.9.18-2.fc18.x86_64

Review of attachment 233288 [details] [review]:

This shouldn't have been necessary. You shouldn't be able to get to an idle mode < NORMAL when the session is inhibited, as per idle_configure()'s current code:
        /* are we inhibited from going idle */
        is_idle_inhibited = idle_is_session_inhibited (manager,
                                                       SESSION_INHIBIT_MASK_IDLE);
        if (is_idle_inhibited) {
                g_debug ("inhibited, so using normal state");
                idle_set_mode (manager, GSD_POWER_IDLE_MODE_NORMAL);
...

Review of attachment 233287 [details] [review]:

There's 2 separate fixes in that patch, one for the case you mention in the commit message. The other one is to remove the now mostly empty idle_adjust_timeout_blank().

Note that all of those patches would need to test cases attached to them. Now that we have a test suite, it would be pretty bad to end up regressing on this.

(In reply to comment #22)
> Review of attachment 233288 [details] [review]:
> 
> This shouldn't have been necessary. You shouldn't be able to get to an idle
> mode < NORMAL when the session is inhibited, as per idle_configure()'s current
> code:
>         /* are we inhibited from going idle */
>         is_idle_inhibited = idle_is_session_inhibited (manager,
>                                                       
> SESSION_INHIBIT_MASK_IDLE);
>         if (is_idle_inhibited) {
>                 g_debug ("inhibited, so using normal state");
>                 idle_set_mode (manager, GSD_POWER_IDLE_MODE_NORMAL);
> ...

It does remove the blank and sleep timeouts, but it does not remove the dim timeout, which will still kick in and set the idle mode to 'dim'.

Created attachment 233410 [details] [review]
power: Don't reconfigure idle timeouts when the user goes idle

Created attachment 233411 [details] [review]
Don't double/quadruple/... timeouts

Created attachment 233412 [details] [review]
power: Don't dim when idle is inhibited

Created attachment 233413 [details] [review]
power: Don't reconfigure idle timeouts when the user goes idle

Created attachment 233414 [details] [review]
Don't double/quadruple/... timeouts

Created attachment 233415 [details] [review]
power: Aggressive blank timeout when the shield is down

(In reply to comment #25)
> (In reply to comment #22)
> > Review of attachment 233288 [details] [review] [details]:
> > 
> > This shouldn't have been necessary. You shouldn't be able to get to an idle
> > mode < NORMAL when the session is inhibited, as per idle_configure()'s current
> > code:
> >         /* are we inhibited from going idle */
> >         is_idle_inhibited = idle_is_session_inhibited (manager,
> >                                                       
> > SESSION_INHIBIT_MASK_IDLE);
> >         if (is_idle_inhibited) {
> >                 g_debug ("inhibited, so using normal state");
> >                 idle_set_mode (manager, GSD_POWER_IDLE_MODE_NORMAL);
> > ...
> 
> It does remove the blank and sleep timeouts, but it does not remove the dim
> timeout, which will still kick in and set the idle mode to 'dim'.

Can't we remove/set it up in idle_configure() as well then?

Review of attachment 233415 [details] [review]:

Looks good otherwise.

::: plugins/power/gsd-power-manager.c
@@ +3198,3 @@
         on_battery = up_client_get_on_battery (manager->priv->up_client);
+        if (manager->priv->screensaver_active) {
+                timeout_blank = 30;

One of the commit message or the code above is wrong. Is it 30 or 60 seconds?

Review of attachment 233414 [details] [review]:

That's the same patch as the old one. Please split it up.

Review of attachment 233413 [details] [review]:

++

Review of attachment 233412 [details] [review]:

As mentioned previously, I'd like to see this be done in idle_configure() instead.

(In reply to comment #34)
> Review of attachment 233414 [details] [review]:
> 
> That's the same patch as the old one. Please split it up.

Oh, I didn't get that thats what you wanted. I just beefed up the commit message here. Will do

I've pushed all the remaining patches, fixing the timeout in the "aggressive blank" patch.

I'm leaving this opened to rework the "don't dim when idle is inhibited" patch though.

Comment on attachment 233414 [details] [review]
Don't double/quadruple/... timeouts

Committed in a single patch. It's harder to separate than if it had been done during coding, and the changes are obviously right.

Comment on attachment 233415 [details] [review]
power: Aggressive blank timeout when the shield is down

Changed the commit message to mention 30 seconds instead of 60.

Created attachment 233598 [details] [review]
power: Handle dim idle the same way as other idles

And unsure that we do not dim the keyboard or the screen when
when the screen lock is up (as we already turn everything off
pretty aggressively when the lock is up)

Created attachment 233599 [details] [review]
power: Don't add SCREENSAVER_FADE_TIME twice

Created attachment 233600 [details] [review]
power: Use session's idle-delay to blank the screen

The blanking will only work as designed if:
session.idle-delay ==
	power.sleep-display-ac ==
	power.sleep-display-battery

So nuke the sleep-display* configuration keys, and rely solely
on the idle-delay to blank the screen.

Created attachment 233602 [details] [review]
power: Use session's idle-delay to blank the screen

The blanking will only work as designed if:
session.idle-delay ==
	power.sleep-display-ac ==
	power.sleep-display-battery

So nuke the sleep-display* configuration keys, and rely solely
on the idle-delay to blank the screen.

Created attachment 233603 [details] [review]
power: Handle dim idle the same way as other idles

And unsure that we do not dim the keyboard or the screen when
when the screen lock is up (as we already turn everything off
pretty aggressively when the lock is up)

Created attachment 233604 [details] [review]
power: Use session's idle-delay to blank the screen

The blanking will only work as designed if:
session.idle-delay ==
	power.sleep-display-ac ==
	power.sleep-display-battery

So nuke the sleep-display* configuration keys, and rely solely
on the idle-delay to blank the screen.

Created attachment 233605 [details] [review]
power: Handle dim idle the same way as other idles

And unsure that we do not dim the keyboard or the screen when
when the screen lock is up (as we already turn everything off
pretty aggressively when the lock is up)

Created attachment 233606 [details] [review]
power: Don't add SCREENSAVER_FADE_TIME twice

Created attachment 233607 [details] [review]
power: Use session's idle-delay to blank the screen

The blanking will only work as designed if:
session.idle-delay ==
	power.sleep-display-ac ==
	power.sleep-display-battery

So nuke the sleep-display* configuration keys, and rely solely
on the idle-delay to blank the screen.

(In reply to comment #45)
> Created an attachment (id=233603) [details] [review]
> power: Handle dim idle the same way as other idles
> 
> And unsure that we do not dim the keyboard or the screen when

*ensure*

Review of attachment 233605 [details] [review]:

Looks good to me.

Review of attachment 233606 [details] [review]:

Thanks for catching that, looks right.

Review of attachment 233607 [details] [review]:

sure, looks good.

Attachment 233605 [details] pushed as 29286fe - power: Handle dim idle the same way as other idles
Attachment 233606 [details] pushed as e3c9fbc - power: Don't add SCREENSAVER_FADE_TIME twice
Attachment 233607 [details] pushed as 3fd7826 - power: Use session's idle-delay to blank the screen

I pushed that, and just now realised that I should have gone through the commit messages before committing.

Hopefully the code itself is clear enough.

commit bd3b8beeae9a3d88483aa0d33fd10042593735a6
Author: Bastien Nocera <hadess@hadess.net>
Date:   Wed Jan 16 20:01:37 2013 +0100

    power: Remove idle-dim-time
    
    And make the default time for the dimming be based upon the idle timeout.
    If there is no idle timeout, dim at one minute of idle.
    If there is an idle timeout, ensure that the dim time is at least 10
    seconds, otherwise a third of the idle-delay.

commit 82af1816f32a26f28027ea7ce8edc79cd833bc76
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Jan 17 09:54:45 2013 +0100

    power: Remember whether battery is low or UPS discharging
    
    And reconfigure the idle timeouts when that changes.