If I go ahead and redock the laptop I notice that not only is the laptop not suspended the session is also not locked.

So presumably it just doesn't know the laptop lid is closed at all and just thinks an external monitor was detached.

I was able to reproduce this without a dock as well by using wake on LAN for step 4 above.

I was also able to verify that /proc/acpi/button/lid/LID/state displays the correct state. It correctly states that the lid is closed when it is. I verified in the Display panel that the LVDS is still on when the lid is closed.

So perhaps we could just add code to g-s-d to rescan the outputs and turn off the LVDS when the lid is closed?

Created attachment 200448 [details] [review]
Disable the laptop's RANDR output when the lid is closed

This is a tentative patch.  It makes the power plugin disable the laptop's RANDR output when the lid is closed but the laptop is not suspended.

The rest of the code does g_warning()s when bad things happen, so this probably needs the "NULL-GError" removed and taken care of with a g_warning().

I'm not sure if we need the opposite code for do_lid_open_action(), to turn the output back on.

Created attachment 200451 [details] [review]
Patch with fixed typo

Oops, fix a typo in the last patch.

There seems to be some consternation about faulty reporting of the lid state. However, unless I'm missing something I can't see how any of the solutions are worse than what we have now.

I'd propose that we:
 * Equate lid closed with LVDS off, except:
  * Explicitly booting or resuming the system (dock power, wake on LAN) when there is only one output and it is an LVDS. In that case, just keep it on. The fringe case of a user booting with a closed lid won't really harm much.

 * Scan outputs at system boot and resume and apply above rule.

 * Rescan outputs when running laptop lid is opened again. Turning LVDS on if it isn't already.

 * Log / report instances where we force LVDS on with a message similar to: "Forcing closed LVDS to on. Possibly broken hardware."

 * Display text in the Display settings that describes the LVDS output as not only Off but Closed. This is will help the user understand that it is off because it is detected to be closed (whether faulty or not).

 * Allow the user to explicitly turn on a closed display (with a suitable warning). This will help workaround faulty switch detection.

Created attachment 200457 [details] [review]
Disable the laptop's output when the lid is closed, for gsd-xrandr-manager

OK, this obsoletes the previous patch for gsd-power-manager.

This patch is for gsd-xrandr-manager.  Whenever it turns outputs on, it won't turn on a laptop's display if the lid is closed.

It doesn't monitor lid events yet.  For that, gsd_xrandr_manager_start() needs to connect to the "changed" signal on the priv->upower_client, and check the lid's state there.

I didn't implement this part because I'd like to know the policy for this.  Say you were docked with the lid closed, and you open the lid.  Do you expect the display to come up cloned, or xinerama-ed, etc?  Should this be the same policy as what we use for "another display got hotplugged", which is to extend the display to it?

As a final thing, right now gsd-power-manager changes the screen's DPMS state.  We should probably do this in gsd-xrandr-manager instead, now that it would be handling power-related things as well.

Another thing - Jon proposed in comment #6 that the xrandr plugin should leave the laptop's output on, regardless of lid state, if it is the only connected output.  The patch doesn't do this yet.

Also, we may still apply stored configurations as before, but we don't sanitize them to remove the laptop's output if the lid is closed.

Sanitizing stored configurations needs to happen in apply_configuration_from_filename() - that's the only place that tries to match the current screen's configuration to what is stored on disk.

(In reply to comment #7)
> I didn't implement this part because I'd like to know the policy for this.  Say
> you were docked with the lid closed, and you open the lid.  Do you expect the
> display to come up cloned, or xinerama-ed, etc?  Should this be the same policy
> as what we use for "another display got hotplugged", which is to extend the
> display to it?

Sounds right to me. Mirroring is rarely a good default and moving all windows to this (likely smaller) output seems undesirable unless the externals are disconnected.

(In reply to comment #11)
> Sounds right to me. Mirroring is rarely a good default and moving all windows
> to this (likely smaller) output seems undesirable unless the externals are
> disconnected.

One problem with this approach is that if the laptop's display is normally the leftmost one, then making it the rightmost when you open the lid will feel wrong.

By the way, I just noticed that make_xinerama_setup() and auto_configure_outputs() don't really do the same thing - the former puts the laptop on the left and activates everything else in order from left to right, and the latter is more complicated (it tries to preserve the ordering of outputs that remain on).

I'm happy to replace auto_configure_outputs() with the simpler make_xinerama_setup(), which is essentially "lay out things from left to right"; it doesn't make sense to have two policies.

Jon, I'd love some help in thinking through the implications of the policies we choose.  I don't have a docking station to test how this actually feels :)

(Also, on a hotplug event, we auto_configure_outputs() *only* if there wasn't any stored configuration that matches the new set of displays.  So you can't assume that you'll get left-to-right always; rather, you may get one of the stored configurations, which get stored only when you do Apply in the control center's display panel - this is likely broken; I'd rather have "save always" or "save never", but not "save only if you click this button rather than use Fn-F7 or whatever".)

The key thing is really the location of the primary display since that is where the OS chrome lives.

I think the natural (default) place for the primary on a laptop or tablet is on the directly attached / internal display.

When that display is not available for some reason (due to lid, etc) or is turned off explicitly then the leftmost external display should be used. The OS chrome works best as the leftmost display (partly due to the hotcorner, partly due to LTR locale, ...)

If the internal display becomes available and is the leftmost display (which it will be in automatic arrangement, or can be if configured by the user) then it should get the primary back.

If the internal was explicitly arranged to be not-leftmost then when it becomes available it may not become primary.

Whether windows move between displays in any of the above scenarios is orthogonal I think.

As discussed on IRC, I'll also play with saving the current configuration after every Fn-F7 change.  Right now we only do that when you hit the "Apply" button in the control center's display panel.  In turn, we made Fn-F7 *not* use any saved configurations in its sequence, because then the sequence felt unpredictable - now that I think of it, it may have been the disparity between saving in one place but not in the other.

(I.e. "I set this configuration; remember it regardless of how I set it".)

OK, I'm doing this in the "docking-stations" branch of gnome-settings-daemon.  So far it does these:

- In the make_xxx_setup() functions, don't turn on the laptop's display if the lid is closed.

- When auto-configuring outputs, simply use a "xinerama" configuration where all available displays are turned on, left to right; this is instead of having a custom algorithm for auto-configuration.

- Monitor the laptop's lid state, and turn the display off or xinerama depending on the state.

It's missing these:

- When using a saved configuration, turn off the laptop's display if the lid is off.  As mentioned before, this should be done in apply_configuration_from_filename().

- Save the configuration after generating/setting it.  I think we can centralize this in apply_configuration().

I'm hoping that upower's up_client_get_lid_is_closed() does the right thing (return false) on a desktop machine, i.e. when up_client_get_lid_is_present() would return false.  I never call the latter function; only the former.

I don't know the implications of having my own UpClient instead of sharing one with the power plugin for g-s-d - similarly to how they share a single GnomeRRScreen.  In any case, sharing a client is trivial if needed; we just put the give-me-the-singleton function in the g-s-d's common module.

I'd appreciate some testing of this branch, as I don't have a docking station to test a closed lid properly :)

*** Bug 663840 has been marked as a duplicate of this bug. ***

As far as I can tell, the current state of the docking-stations branch now has all the features we've discussed.  Testing is much appreciated :)

If anyone has a chance to make a package for F16 I would really like to try this out.

Now that Takashi Iwai gave me a tip for how to test this with my (dockless) laptop and RTC alarm wakeups, I've committed a bunch of fixes.

Things work much better than before right now:

- The daemon responds to opening and closing the lid.

- The primary output is kept on the laptop if possible.

One last tweak we may want to make is to ensure the laptop's display is turned on if you open the lid *but* you had a saved configuration that turns off the laptop display.  For example:

1. Close the lid.  Your external monitor gets used.

2. Run the control center's display panel; fiddle with the external monitor; keep the laptop's display as off, or press Fn-F7 so that something gets saved.

3. Open the lid.  Since you have a saved configuration, we don't autoconfigure (and thus we don't enable the laptop's display), and we keep on using the configuration you had.

We have a turn_off_laptop_display(); maybe we need a corresponding turn_on_laptop_display() as well, instead of the logic above.

Note that my work doesn't deal at all with step 7 from the opening comment.  Undocking a laptop that has the lid closed, per Jon's comment, should cause the laptop to be suspended.

How do we detect dock/undock events?  (Is this the responsibility of the power plugin in g-s-d?)

This doesn't compile for me because of a change in 5352c1aa116f8ed0b35368cbaefe27eb96cbd874

@@ -1554,7 +1557,7 @@ handle_rotate_windows (GsdXrandrManager *mgr,
 
         gnome_rr_output_info_set_rotation (rotatable_output_info, next_rotation);
 
-        success = apply_configuration (mgr, current, timestamp, show_error);
+        success = apply_configuration (mgr, current, timestamp, show_error, TRUE, TRUE);
         if (success)
                 rotate_touchscreens (mgr, next_rotation);
 

apply_configuration only takes 5 arguments, not 6. Removing the last TRUE fixes compilation, dunno if it's the right fix

Yes, that is the right fix; I just pushed it.  Sorry for the trouble - I'm testing this on a backported version to g-s-d 3.0.3.

(In reply to comment #18)
> If anyone has a chance to make a package for F16 I would really like to try
> this out.

http://koji.fedoraproject.org/koji/taskinfo?taskID=3523415

(In reply to comment #20)
> Note that my work doesn't deal at all with step 7 from the opening comment. 
> Undocking a laptop that has the lid closed, per Jon's comment, should cause the
> laptop to be suspended.
> 
> How do we detect dock/undock events?  (Is this the responsibility of the power
> plugin in g-s-d?)

Actually I don't think it is necessarily an undock. It is the result of having no usable outputs. Once I have disconnected the only usable monitor (the external since the internal is closed and unusable) then the system should sleep.

Ok after testing the patch I found:

 * As noted above, the system does not suspend when there are no active monitors
 * The first time after installing the test package I noticed that the internal turned off when I connected the external. That was unexpected. It however did not reoccur after I manually fixed it using the Display settings panel.
 * Suspending the system, attaching an external monitor (or docking), and resuming the system (lid closed) correctly moves the primary display and windows to the external monitor and turns off the internal. YAY.
 * This exposes some kind of bug in the wallpaper used in the screen lock - it doesn't stretch to fill the screen in the above case. I think this is reported separately.
 * At one point it got into a mode where one monitor was still "off" when both were available and fn-f7 was switching between them. Not sure how it got confused about that. May be related to what I noticed in the first bullet point above.

This is already so much better. It will be very good if we can add the suspend when no displays are available bits.

Thanks!

Jon - kind of obvious question, but just to check.  You *did* set the lid-close-ac-action on your box to "nothing", right?  Otherwise you wouldn't have been able to test this?

Oh, never mind, I just saw the is_docked() inside do_lid_closed_action() in the power plugin.

(In reply to comment #26)
> Jon - kind of obvious question, but just to check.  You *did* set the
> lid-close-ac-action on your box to "nothing", right?  Otherwise you wouldn't
> have been able to test this?

No, I didn't change that.


Additional details:

When I resumed my suspended laptop in a dock in the office just now the laptop internal display was not turned off (and was still primary) and the external monitor was placed on top of it. Screenshots to follow.

Created attachment 201839 [details]
screenshot showing screen after unlocking after resuming in dock

Created attachment 201840 [details]
screenshot of display settings after unlocking after resuming in dock

Screen 0: minimum 320 x 200, current 1680 x 1050, maximum 8192 x 8192
LVDS1 connected 1366x768+0+0 (normal left inverted right x axis y axis) 277mm x 156mm
   1366x768       60.0*+
   1024x768       60.0  
   800x600        60.3     56.2  
   640x480        59.9  
VGA1 connected 1680x1050+0+0 (normal left inverted right x axis y axis) 433mm x 270mm
   1680x1050      60.0*+
   1280x1024      75.0     60.0  
   1152x864       75.0  
   1024x768       75.1     60.0  
   800x600        75.0     60.3  
   640x480        75.0     60.0  
   720x400        70.1

That's very odd.  I don't see how docking and resuming should give you *that*, except for having a stored configuration with that setup (but since we don't let you overlap displays in the capplet, that shouldn't happen).

I just noticed that make_xinerama_setup() should explicitly turn off the laptop's display if the lid is closed, but even this shouldn't give you that bug.

I've pushed another set of unbuilt/untested changes to the docking-stations branch.  This makes the monitoring of the lid's state only happen in the power plugin, and in turn it will notify the randr plugin that the lid's state has changed.

This avoids both plugins racing for the lid notification.  It also keeps the when-to-suspend policy in the power plugin, and the turn-off-the-laptop's-display policy in the randr plugin.

Testing is appreciated!   Feel free to push fixes for syntax errors/etc.

Please forgive me if this is off-topic but I am watching this bug for a while and now I am getting confused because in bug 595531 comment 20 Ray says it shouldn't make a difference if the second display is attached to the docking or directly. My problem is with gdm, the greeter is displayed on the laptop display even if the lid is closed and regardless of how the second display is attached or whether I'm on AC or not.

So I wonder if it makes sense to introduce fixes for docked and undocked scenarios when gnome-settings-daemon doesn't set any external display as primary and even tries to use a display that is turned off.

(In reply to comment #33)
> My problem is with gdm, the greeter is displayed on the laptop
> display even if the lid is closed and regardless of how the second display is
> attached or whether I'm on AC or not.

As far as I can tell this will work properly now on the docking-stations branch.  We simply don't use the laptop's output if the lid is closed.

I've pushed another set of fixes to the docking-stations branch.  As far as I can tell, this is ready for merging to master.

What is the easiest way for me to test it? Build from git?

Created attachment 202914 [details] [review]
gnome-settings-daemon-docking-stations-3.2.diff

Chris, this patch is against gnome-settings-daemon 3.2.1 - it's what I'm using to test the patchset in openSUSE with a stable version of Gnome.

To test against git master, you'll have to get a diff from git.

WTF - sorry about the greenwoodglobal "title"; I pasted from the wrong clipboard.  That attachment is supposed to be named gnome-settings-daemon-docking-stations-3.2.diff

Thanks for all the work on this Federico.

Review of attachment 202914 [details] [review]:

I really don't like the gnome-settings-session.[ch] changes. Can't gnome-desktop provide a way to get the default GnomeRRScreen for an X11 screen itself?

I could not find the code that would trigger the power plugin to go through its policy again when a screen is unplugged.

::: plugins/power/gsd-power-manager.c
@@ +2205,2 @@
         /* perform policy action */
+        if (non_laptop_outputs_are_all_disconnected (manager->priv->x11_screen)) {

That should be something that the XRandR plugin tracks and can tell us, no?

::: plugins/xrandr/gsd-xrandr-manager.c
@@ +97,3 @@
 "    </method>"
 "  </interface>"
+"  <interface name='org.gnome.SettingsDaemon.XRANDR.Internal'>"

This should instead be the XRandR plugin listening to a signal from the power plugin.

@@ +1077,3 @@
+        }
+
+ */

Add them to a list using g_list_insert_sorted(), cleaner, and no need to go through the list twice.

(In reply to comment #40)

Oops!  I committed some more changes (mainly to remove the internal DBus interface) and *then* saw your review.  Anyway, let me respond.

> I really don't like the gnome-settings-session.[ch] changes. Can't
> gnome-desktop provide a way to get the default GnomeRRScreen for an X11 screen
> itself?

I'd say, "yes", but...

The stuff I just committed also adds a gnome_settings_session_get_upower_client(), so that the power and xrandr plugins can both share the same UpClient instance.  I need this so that they can catch the client's "changed" signal in the correct order: the xrandr plugin needs to go first and so it does g_signal_connect(); the power plugin needs to go last and so it does g_signal_connect_after().

For now, I'm happy with g-s-d being the one that provides the "give me a singleton" service.  It has special needs from the underlying GnomeRRScreen and UpClient.  As soon as other apps find the need to have singletons, we can move that code to the libraries.  (GnomeRRScreen is not for public consumption, anyway, although it has begun to be used elsewhere.)

(I don't care where the singleton lives; just that the plugins can acquire it :)

> I could not find the code that would trigger the power plugin to go through its
> policy again when a screen is unplugged.

Good point.

On a related note, I just added code that makes the randr plugin re-probe the outputs when the laptop's lid is opened/closed.  My laptop seems to detect plugging the VGA output, but not unplugging it.  This lets things work correctly if I start with a closed laptop with an external monitor, and then I unplug the monitor and open the laptop - it properly re-probes, reconfigures, and remains unsuspended.

However, if I made the power plugin run its logic as soon as a screen gets unplugged, the case above would lead it to suspend as soon as I unplug the monitor, and resume as soon as I open the lid.  The end result would be what I wanted it to be, but with a cumbersome suspend in the middle.  I'm not sure what to do about that - we could have a timeout so that the closed laptop won't suspend immediately after removing all displays, but only after a few seconds if you don't open the lid / plug another display / etc.  That seems a bit hairy to implement without some communication between both plugins...

> ::: plugins/power/gsd-power-manager.c
> @@ +2205,2 @@
>          /* perform policy action */
> +        if (non_laptop_outputs_are_all_disconnected
> (manager->priv->x11_screen)) {
> 
> That should be something that the XRandR plugin tracks and can tell us, no?

Initially I also felt uncomfortable about both plugins looking at each other's state - the xrandr one looks at the laptop lid's state via UpClient, and the power one looks at the RANDR outputs and frobs the backlight via GnomeRRScreen.

However, the screens and power states are evidently connected and you can't separate them from each other.  So I'm fine with having this reflected in the code.

> ::: plugins/xrandr/gsd-xrandr-manager.c
> @@ +97,3 @@
>  "    </method>"
>  "  </interface>"
> +"  <interface name='org.gnome.SettingsDaemon.XRANDR.Internal'>"
> 
> This should instead be the XRandR plugin listening to a signal from the power
> plugin.

This DBus interface is gone now; see above about a normal and an _after signal connection.

> 
> @@ +1077,3 @@
> +        }
> +
> + */
> 
> Add them to a list using g_list_insert_sorted(), cleaner, and no need to go
> through the list twice.

Nah, this uses fewer allocations than using a list, and doesn't use insertion sort.

I'm ready to merge this to master; it works very well on my laptop.

I've made gnome_rr_screen_new() return a singleton itself; I noticed that up_client_new() already does that.  Thus, I removed gnome_settings_session_get_screen() and _get_upower_client().

I've merged the docking-stations branch into master; it's pushed now as 6e17bc78.

One last thing is to consider whether to suspend the laptop if you close it and *then* unplug the external monitor.  We probably need some sort of timer so that you can plug/unplug/close/open without a bunch of suspends/unsuspends in the process.

For now I'll mark the bug as FIXED; any further refinements can go in other bugs.  Feel free to assign them to me!