After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 787629 - Settings for external monitor are deleted after reboot, suspension, log out
Settings for external monitor are deleted after reboot, suspension, log out
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 788249 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-09-13 14:19 UTC by Olivier Tilloy
Modified: 2017-10-02 20:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
monitors.xml (3.86 KB, text/xml)
2017-09-14 09:34 UTC, Olivier Tilloy
  Details
monitors-v1-backup.xml (4.56 KB, text/xml)
2017-09-14 09:34 UTC, Olivier Tilloy
  Details
monitor-config-store: Make monitor spec parsing/writing reusable (4.76 KB, patch)
2017-09-28 16:02 UTC, Jonas Ådahl
none Details | Review
monitor-config-store: Make monitor spec parsing/writing reusable (4.76 KB, patch)
2017-09-28 16:03 UTC, Jonas Ådahl
committed Details | Review
monitor-config: Keep track of disabled monitors for stored configs (19.05 KB, patch)
2017-09-28 16:03 UTC, Jonas Ådahl
none Details | Review
monitor-unit-tests: Add way for test case setup to specify output serial (1.92 KB, patch)
2017-09-28 16:03 UTC, Jonas Ådahl
committed Details | Review
monitor-unit-tests: Test configs with explicitly disabled monitors (9.67 KB, patch)
2017-09-28 16:03 UTC, Jonas Ådahl
committed Details | Review
renderer-native: Unset mode on disabled CRTCs (1.94 KB, patch)
2017-09-28 16:03 UTC, Jonas Ådahl
none Details | Review
monitor-config: Keep track of disabled monitors for stored configs (21.88 KB, patch)
2017-09-29 15:26 UTC, Jonas Ådahl
none Details | Review
renderer-native: Unset mode on disabled CRTCs (2.52 KB, patch)
2017-09-29 15:26 UTC, Jonas Ådahl
committed Details | Review
monitor-config: Keep track of disabled monitors for stored configs (23.75 KB, patch)
2017-09-29 19:44 UTC, Jonas Ådahl
committed Details | Review

Description Olivier Tilloy 2017-09-13 14:19:05 UTC
(initially reported for ubuntu as https://bugs.launchpad.net/ubuntu/+source/gnome-control-center/+bug/1716341)

Since Ubuntu 17.10 (the current development series) upgraded from 3.24.3 to 3.25.92, settings for external monitors are working but are lost after reboot/suspension/log-out log-in.

Example:
1- open Gnome Settings and select "Devices → Displays"
2- set the external monitor as "Single Display" and click on "Apply"
3- log-out

After log-in the laptop monitor will be set as default instead of the external monitor.
Comment 1 Rui Matos 2017-09-14 08:20:34 UTC
Which mutter version is this? Can you attach any ~/.config/monitors*.xml files you might have?
Comment 2 Olivier Tilloy 2017-09-14 09:33:42 UTC
osomon@bribon:~$ LANG=C apt policy mutter
mutter:
  Installed: 3.25.91+20170902~ce515c5-1ubuntu1
  Candidate: 3.25.91+20170902~ce515c5-1ubuntu1
  Version table:
 *** 3.25.91+20170902~ce515c5-1ubuntu1 500
        500 http://es.archive.ubuntu.com/ubuntu artful/main amd64 Packages
        100 /var/lib/dpkg/status


osomon@bribon:~$ ls -l ~/.config/monitors*.xml
-rw-rw-r-- 1 osomon osomon 4672 avril 18 11:31 /home/osomon/.config/monitors-v1-backup.xml
-rw-rw-r-- 1 osomon osomon 3950 sept. 14 07:02 /home/osomon/.config/monitors.xml

(attaching the two xml files)
Comment 3 Olivier Tilloy 2017-09-14 09:34:24 UTC
Created attachment 359770 [details]
monitors.xml
Comment 4 Olivier Tilloy 2017-09-14 09:34:51 UTC
Created attachment 359771 [details]
monitors-v1-backup.xml
Comment 5 Jonas Ådahl 2017-09-15 06:19:39 UTC
Could you also attach the output of these two commands:

command #1:

gdbus call -e -d org.gnome.Mutter.DisplayConfig -o /org/gnome/Mutter/DisplayConfig -m org.gnome.Mutter.DisplayConfig.GetCurrentState

command #2:

gdbus call -e -d org.gnome.Mutter.DisplayConfig -o /org/gnome/Mutter/DisplayConfig -m org.gnome.Mutter.DisplayConfig.GetResources

as well as check the journal (using journalctl) if there are any relevant messages from gnome-sell when you log in.
Comment 6 Olivier Tilloy 2017-09-15 07:56:53 UTC
osomon@bribon:~$ gdbus call -e -d org.gnome.Mutter.DisplayConfig -o /org/gnome/Mutter/DisplayConfig -m org.gnome.Mutter.DisplayConfig.GetCurrentState
(uint32 1, [(('HDMI-1', 'SAM', 'SyncMaster', 'H9MZ502297'), [('1920x1080@60.000495910644531', 1920, 1080, 60.000495910644531, 1.0, [1.0], {'is-current': <true>, 'is-preferred': <true>}), ('1680x1050@59.883750915527344', 1680, 1050, 59.883750915527344, 1.0, [1.0], {}), ('1600x1200@60.000499725341797', 1600, 1200, 60.000499725341797, 1.0, [1.0, 2.0], {}), ('1440x900@59.901962280273438', 1440, 900, 59.901962280273438, 1.0, [1.0], {}), ('1280x1024@60.020236968994141', 1280, 1024, 60.020236968994141, 1.0, [1.0], {}), ('1280x960@60.000499725341797', 1280, 960, 60.000499725341797, 1.0, [1.0], {}), ('1280x800@59.910045623779297', 1280, 800, 59.910045623779297, 1.0, [1.0], {}), ('1024x768@60.004344940185547', 1024, 768, 60.004344940185547, 1.0, [1.0], {}), ('800x600@60.317043304443359', 800, 600, 60.317043304443359, 1.0, [1.0], {}), ('800x600@56.250495910644531', 800, 600, 56.250495910644531, 1.0, [1.0], {}), ('640x480@59.940975189208984', 640, 480, 59.940975189208984, 1.0, [], {})], {'is-builtin': <false>, 'display-name': <'Samsung Electric Company 23"'>}), (('LVDS-1', 'LGD', '0x02d8', '0x00000000'), [('1366x768@60.019145965576172', 1366, 768, 60.019145965576172, 1.0, [1.0], {'is-preferred': <true>}), ('1280x720@59.855625152587891', 1280, 720, 59.855625152587891, 1.0, [1.0], {}), ('1024x768@59.920631408691406', 1024, 768, 59.920631408691406, 1.0, [1.0], {}), ('800x600@59.861904144287109', 800, 600, 59.861904144287109, 1.0, [1.0], {})], {'is-builtin': <true>, 'display-name': <'Affichage intégré'>})], [(0, 0, 1.0, uint32 0, true, [('HDMI-1', 'SAM', 'SyncMaster', 'H9MZ502297')], @a{sv} {})], {'layout-mode': <uint32 2>})


osomon@bribon:~$ gdbus call -e -d org.gnome.Mutter.DisplayConfig -o /org/gnome/Mutter/DisplayConfig -m org.gnome.Mutter.DisplayConfig.GetResources
(uint32 1, [(uint32 0, int64 32, 0, 0, 1920, 1080, 6, uint32 0, [uint32 0, 1, 2, 3, 4, 5, 6, 7], @a{sv} {}), (1, 39, 0, 0, 0, 0, -1, 0, [0, 1, 2, 3, 4, 5, 6, 7], {}), (2, 46, 0, 0, 0, 0, -1, 0, [0, 1, 2, 3, 4, 5, 6, 7], {})], [(uint32 0, int64 56, 0, [uint32 0, 1, 2], 'HDMI-1', [uint32 6, 0, 10, 5, 8, 7, 1, 11, 9, 4, 2], @au [], {'vendor': <'SAM'>, 'product': <'SyncMaster'>, 'serial': <'H9MZ502297'>, 'width-mm': <510>, 'height-mm': <290>, 'display-name': <'Samsung Electric Company 23"'>, 'backlight': <-1>, 'min-backlight-step': <-1>, 'primary': <true>, 'presentation': <false>, 'connector-type': <'HDMIA'>, 'underscanning': <false>, 'supports-underscanning': <false>, 'edid': <[byte 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x4c, 0x2d, 0x56, 0x06, 0x33, 0x32, 0x48, 0x57, 0x14, 0x14, 0x01, 0x03, 0x80, 0x33, 0x1d, 0x78, 0x2a, 0xe9, 0xe1, 0xa3, 0x54, 0x26, 0x0f, 0x4c, 0x99, 0x50, 0x54, 0x23, 0x08, 0x00, 0x81, 0x00, 0x81, 0x40, 0x81, 0x80, 0x95, 0x00, 0xa9, 0x40, 0xb3, 0x00, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38, 0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0xfe, 0x1f, 0x11, 0x00, 0x00, 0x1e, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x38, 0x3c, 0x1e, 0x51, 0x11, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, 0x00, 0x53, 0x79, 0x6e, 0x63, 0x4d, 0x61, 0x73, 0x74, 0x65, 0x72, 0x0a, 0x20, 0x20, 0x00, 0x00, 0x00, 0xff, 0x00, 0x48, 0x39, 0x4d, 0x5a, 0x35, 0x30, 0x32, 0x32, 0x39, 0x37, 0x0a, 0x20, 0x20, 0x00, 0xba]>}), (1, 47, -1, [0, 1, 2], 'LVDS-1', [3, 26, 13, 12], [], {'vendor': <'LGD'>, 'product': <'0x02d8'>, 'serial': <'0x00000000'>, 'width-mm': <280>, 'height-mm': <160>, 'display-name': <'Affichage intégré'>, 'backlight': <-1>, 'min-backlight-step': <-1>, 'primary': <false>, 'presentation': <false>, 'connector-type': <'LVDS'>, 'underscanning': <false>, 'supports-underscanning': <false>, 'edid': <[byte 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x30, 0xe4, 0xd8, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x01, 0x03, 0x80, 0x1c, 0x10, 0x78, 0xea, 0x88, 0x55, 0x99, 0x5b, 0x55, 0x8f, 0x26, 0x1d, 0x50, 0x54, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x60, 0x1d, 0x56, 0xd8, 0x50, 0x00, 0x18, 0x30, 0x30, 0x40, 0x47, 0x00, 0x15, 0x9c, 0x10, 0x00, 0x00, 0x1b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfe, 0x00, 0x4c, 0x47, 0x20, 0x44, 0x69, 0x73, 0x70, 0x6c, 0x61, 0x79, 0x0a, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfe, 0x00, 0x4c, 0x50, 0x31, 0x32, 0x35, 0x57, 0x48, 0x32, 0x2d, 0x53, 0x4c, 0x42, 0x33, 0x00, 0x59]>})], [(uint32 0, int64 0, uint32 1680, uint32 1050, 59.883750915527344, uint32 9), (1, 1, 1280, 800, 59.910045623779297, 9), (2, 2, 640, 480, 59.940975189208984, 10), (3, 3, 1366, 768, 60.019145965576172, 9), (4, 4, 800, 600, 56.250495910644531, 5), (5, 5, 1440, 900, 59.901962280273438, 9), (6, 6, 1920, 1080, 60.000495910644531, 5), (7, 7, 1280, 960, 60.000499725341797, 5), (8, 8, 1280, 1024, 60.020236968994141, 5), (9, 9, 800, 600, 60.317043304443359, 5), (10, 10, 1600, 1200, 60.000499725341797, 5), (11, 11, 1024, 768, 60.004344940185547, 10), (12, 12, 800, 600, 59.861904144287109, 6), (13, 13, 1024, 768, 59.920631408691406, 6), (14, 14, 1152, 864, 59.959136962890625, 6), (15, 15, 1280, 960, 59.939552307128906, 6), (16, 16, 1400, 1050, 59.97894287109375, 6), (17, 17, 1440, 1080, 59.989345550537109, 6), (18, 18, 1600, 1200, 59.869609832763672, 6), (19, 19, 1920, 1440, 59.968486785888672, 6), (20, 20, 2048, 1536, 59.954292297363281, 6), (21, 21, 1280, 800, 59.810825347900391, 6), (22, 22, 1440, 900, 59.887947082519531, 6), (23, 23, 1680, 1050, 59.954746246337891, 6), (24, 24, 1920, 1200, 59.885101318359375, 6), (25, 25, 2560, 1600, 59.987091064453125, 6), (26, 26, 1280, 720, 59.855625152587891, 6), (27, 27, 1368, 768, 59.882545471191406, 6), (28, 28, 1600, 900, 59.946521759033203, 6), (29, 29, 1920, 1080, 59.963344573974609, 6), (30, 30, 2048, 1152, 59.903682708740234, 6), (31, 31, 2560, 1440, 59.961124420166016, 6), (32, 32, 2880, 1620, 59.960758209228516, 6), (33, 33, 3200, 1800, 59.956634521484375, 6), (34, 34, 3840, 2160, 59.981430053710938, 6), (35, 35, 4096, 2304, 59.989528656005859, 6), (36, 36, 5120, 2880, 59.987663269042969, 6)], 65535, 65535)
Comment 7 Olivier Tilloy 2017-09-15 08:03:08 UTC
(maybe) relevant excerpts of journalctl:

sept. 15 09:58:21 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/shell/ui/main.js 314]: reference to undefined property "MetaStage"
sept. 15 09:58:21 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/shell/ui/layout.js 217]: reference to undefined property "MetaWindowGroup"
sept. 15 09:58:21 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/shell/ui/osdMonitorLabeler.js 59]: reference to undefined property "MetaDBusDisplayConfigSkeleton"
sept. 15 09:58:21 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/shell/ui/slider.js 38]: reference to undefined property "CallyActor"
sept. 15 09:58:22 bribon polkitd(authority=local)[2168]: Registered Authentication Agent for unix-session:c1 (system bus name :1.37 [/usr/bin/gnome-shell], object path /org/freedesktop/PolicyKit1/AuthenticationAgent, locale fr_FR.UTF-8)
sept. 15 09:58:22 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/gjs/modules/tweener/tweener.js 540]: reference to undefined property "isSpecialProperty"
sept. 15 09:58:22 bribon gnome-shell[4841]: Error looking up permission: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.impl.portal.PermissionStore was not provided by any .service files
sept. 15 09:58:22 bribon gnome-shell[4841]: JS WARNING: [resource:///org/gnome/shell/ui/windowManager.js 1470]: reference to undefined property "MetaWindowX11"
sept. 15 09:58:29 bribon gnome-shell[4841]: meta_renderer_native_release_onscreen: assertion 'onscreen_native->gbm.next_fb_id == 0' failed
sept. 15 09:58:29 bribon gnome-shell[4841]: _cogl_onscreen_free: assertion 'onscreen->winsys == ((void *)0)' failed
sept. 15 09:58:29 bribon audit[4841]: ANOM_ABEND auid=4294967295 uid=133 gid=145 ses=4294967295 pid=4841 comm="gnome-shell" exe="/usr/bin/gnome-shell" sig=11 res=1
sept. 15 09:58:29 bribon kernel: gnome-shell[4841]: segfault at 1a0 ip 00007f4cb99bf836 sp 00007ffd8a7b9a60 error 4 in libmutter-1.so.0.0.0[7f4cb98db000+13f000]
sept. 15 09:58:29 bribon gnome-shell[6869]: JS WARNING: [resource:///org/gnome/shell/ui/main.js 314]: reference to undefined property "MetaStage"
sept. 15 09:58:29 bribon gnome-shell[6869]: JS WARNING: [resource:///org/gnome/shell/ui/layout.js 217]: reference to undefined property "MetaWindowGroup"
sept. 15 09:58:29 bribon gnome-shell[6869]: JS WARNING: [resource:///org/gnome/shell/ui/osdMonitorLabeler.js 59]: reference to undefined property "MetaDBusDisplayConfigSkeleton"
sept. 15 09:58:30 bribon gnome-shell[6869]: JS WARNING: [resource:///org/gnome/shell/ui/slider.js 38]: reference to undefined property "CallyActor"
sept. 15 09:58:30 bribon polkitd(authority=local)[2168]: Registered Authentication Agent for unix-session:2 (system bus name :1.78 [/usr/bin/gnome-shell], object path /org/freedesktop/PolicyKit1/AuthenticationAgent, locale fr_FR.UTF-8)
sept. 15 09:58:30 bribon gnome-shell[6869]: JS WARNING: [resource:///org/gnome/gjs/modules/tweener/tweener.js 540]: reference to undefined property "isSpecialProperty"
Comment 8 Dmitrii Shcherbakov 2017-09-15 19:53:40 UTC
I can confirm this behavior on 3.26:

un  gnome-session                                       <none>                         <none>                         (no description available)
ii  gnome-session-bin                                   3.26.0-0ubuntu1                amd64                          GNOME Session Manager - Minimal runtime
ii  gnome-session-canberra                              0.30-3ubuntu1                  amd64                          GNOME session log in and log out sound events
ii  gnome-session-common                                3.26.0-0ubuntu1                all                            GNOME Session Manager - common files
un  gnome-session-flashback                             <none>                         <none>                         (no description available)
ii  gnome-settings-daemon                               3.26.0-0ubuntu1                amd64                          daemon handling the GNOME session settings
ii  gnome-settings-daemon-schemas                       3.26.0-0ubuntu1                all                            Shared schemas for gnome-settings-daemon
ii  gnome-shell                                         3.26.0-0ubuntu1                amd64                          graphical shell for the GNOME desktop
ii  gnome-shell-common                                  3.26.0-0ubuntu1                all                            common files for the GNOME graphical shell
ii  gnome-shell-extension-appindicator                  17.10                          all                            App indicators for GNOME Shell
un  gnome-shell-extension-dashtodock                    <none>                         <none>                         (no description available)
ii  gnome-shell-extension-ubuntu-dock                   0.4                            all                            Ubuntu Dock for GNOME Shell
ii  gnome-shell-extensions                              3.26.0-1                       all                            Extensions to extend functionality of GNOME Shell

➜  ~ dpkg -l '*mutter*' | grep ii
ii  gir1.2-mutter-1:amd64 3.26.0-1     amd64        GObject introspection data for Mutter
ii  libmutter-1-0:amd64   3.26.0-1     amd64        window manager library from the Mutter window manager
ii  mutter                3.26.0-1     amd64        lightweight GTK+ window manager
ii  mutter-common         3.26.0-1     all          shared files for the Mutter window manager

http://paste.ubuntu.com/25542530/

monitors.xml looks as follows after settings are manually changed:

http://paste.ubuntu.com/25542540/
Comment 9 Michael Biebl 2017-09-24 23:12:24 UTC
I can confirm this issue as well. I see this bug is still listed as NEEDINFO.
Let me know which info you are missing and if there is a way I can help.
Comment 10 Jonas Ådahl 2017-09-28 16:02:41 UTC
Created attachment 360610 [details] [review]
monitor-config-store: Make monitor spec parsing/writing reusable

Another use of <monitorspec> will be added, so make the code dealing
with parsing and writing the reusable.
Comment 11 Jonas Ådahl 2017-09-28 16:03:41 UTC
Created attachment 360611 [details] [review]
monitor-config-store: Make monitor spec parsing/writing reusable

Another use of <monitorspec> will be added, so make the code dealing
with parsing and writing the reusable.
Comment 12 Jonas Ådahl 2017-09-28 16:03:45 UTC
Created attachment 360612 [details] [review]
monitor-config: Keep track of disabled monitors for stored configs

When saving and restoring monitor configurations, we must take disabled
monitors into account, as otherwise one cannot store/restore a
configuration where one or more monitors are explicitly disabled. Make
this possible by adding a <disabled> element to the <configure> element
which lists the monitors that are explicitly disabled. These ones are
included when generating the configuration key, meaning they'll be
picked up correctly.
Comment 13 Jonas Ådahl 2017-09-28 16:03:49 UTC
Created attachment 360613 [details] [review]
monitor-unit-tests: Add way for test case setup to specify output serial

This is needed to avoid migration tests to avoid the best-effort tiling
monitor detection paths.
Comment 14 Jonas Ådahl 2017-09-28 16:03:54 UTC
Created attachment 360614 [details] [review]
monitor-unit-tests: Test configs with explicitly disabled monitors

Check that configurations where monitors are disabled are properly
used. Also test that old configurations with explicitly disabled
outputs are migrated properly.
Comment 15 Jonas Ådahl 2017-09-28 16:03:58 UTC
Created attachment 360615 [details] [review]
renderer-native: Unset mode on disabled CRTCs

Make sure we properly unset the CRTC mode when a monitor is disabled.
Comment 16 Rui Matos 2017-09-29 14:13:48 UTC
Review of attachment 360612 [details] [review]:

::: src/backends/meta-monitor-config-manager.c
@@ +642,3 @@
                                            primary_logical_monitor_config);
 
+  return meta_monitors_config_new (logical_monitor_configs, NULL, layout_mode,

hmmm, but there might be disabled outputs here. perhaps we should have two versions of meta_monitors_config_new(), one to be used in config-store and config-migration that takes and explicit list of disabled outputs and another that does find_disabled_monitor_specs() automatically ?

same comment on all the calls below and in meta-monitor-manager

@@ +1394,2 @@
 gboolean
+meta_logical_monitor_configs_has_monitor (GList           *logical_monitor_configs,

naming nit: understand why you named this _configs_ (plural) but then should be _have_ otherwise this sounds strange

@@ +1417,3 @@
+
+static gboolean
+meta_monitors_configs_is_monitor_enabled (MetaMonitorsConfig *config,

naming nit: but here it should definitely be _config_ (singular)
Comment 17 Rui Matos 2017-09-29 14:13:57 UTC
Review of attachment 360611 [details] [review]:

::: src/backends/meta-monitor-config-store.c
@@ +601,1 @@
+        parser->state = parser->monitor_spec_parent_state;

should we unset ->monitor_spec_parent_state just to be safe?
Comment 18 Rui Matos 2017-09-29 14:16:37 UTC
Review of attachment 360613 [details] [review]:

ok
Comment 19 Rui Matos 2017-09-29 14:18:29 UTC
Review of attachment 360614 [details] [review]:

sure
Comment 20 Jonas Ådahl 2017-09-29 14:23:05 UTC
(In reply to Rui Matos from comment #16)
> Review of attachment 360612 [details] [review] [review]:
> 
> ::: src/backends/meta-monitor-config-manager.c
> @@ +642,3 @@
>                                             primary_logical_monitor_config);
>  
> +  return meta_monitors_config_new (logical_monitor_configs, NULL,
> layout_mode,
> 
> hmmm, but there might be disabled outputs here. perhaps we should have two
> versions of meta_monitors_config_new(), one to be used in config-store and
> config-migration that takes and explicit list of disabled outputs and
> another that does find_disabled_monitor_specs() automatically ?

Thats a good idea. Something like meta_monitors_config_new_temporary() and ..._new() to distinguish the two.

> 
> same comment on all the calls below and in meta-monitor-manager
> 
> @@ +1394,2 @@
>  gboolean
> +meta_logical_monitor_configs_has_monitor (GList          
> *logical_monitor_configs,
> 
> naming nit: understand why you named this _configs_ (plural) but then should
> be _have_ otherwise this sounds strange

True.

> 
> @@ +1417,3 @@
> +
> +static gboolean
> +meta_monitors_configs_is_monitor_enabled (MetaMonitorsConfig *config,
> 
> naming nit: but here it should definitely be _config_ (singular)

True.

(In reply to Rui Matos from comment #17)
> Review of attachment 360611 [details] [review] [review]:
> 
> ::: src/backends/meta-monitor-config-store.c
> @@ +601,1 @@
> +        parser->state = parser->monitor_spec_parent_state;
> 
> should we unset ->monitor_spec_parent_state just to be safe?

Don't really have a state to unset it to though (well, INITIAL maybe, but thats more about being in the beginning of the document).
Comment 21 Rui Matos 2017-09-29 14:38:47 UTC
Review of attachment 360615 [details] [review]:

This is a good catch, but see below

::: src/backends/native/meta-renderer-native.c
@@ +1402,1 @@
 }

this function is only called on init and set_legacy_view_size(). I believe we need to call it in meta_stage_native_rebuild_views() too
Comment 22 Rui Matos 2017-09-29 14:53:58 UTC
*** Bug 788249 has been marked as a duplicate of this bug. ***
Comment 23 Jonas Ådahl 2017-09-29 15:26:04 UTC
Created attachment 360672 [details] [review]
monitor-config: Keep track of disabled monitors for stored configs

When saving and restoring monitor configurations, we must take disabled
monitors into account, as otherwise one cannot store/restore a
configuration where one or more monitors are explicitly disabled. Make
this possible by adding a <disabled> element to the <configure> element
which lists the monitors that are explicitly disabled. These ones are
included when generating the configuration key, meaning they'll be
picked up correctly.
Comment 24 Jonas Ådahl 2017-09-29 15:26:17 UTC
Created attachment 360673 [details] [review]
renderer-native: Unset mode on disabled CRTCs

Make sure we properly unset the CRTC mode when a monitor is disabled.
Comment 25 Rui Matos 2017-09-29 15:47:41 UTC
Review of attachment 360672 [details] [review]:

::: src/backends/meta-monitor-config-manager.c
@@ +1216,3 @@
+                                    MetaMonitorsConfigFlag        flags)
+{
+  return meta_monitors_config_new (logical_monitor_configs, NULL, layout_mode, flags);

my suggestion was to populate the disabled monitor specs here automatically in this case instead of passing NULL when there might in fact be disabled monitors. I guess it doesn't really change anything right now but it seems the correct thing to do

::: src/backends/meta-monitor-manager.c
@@ +1861,3 @@
+      if (meta_logical_monitor_configs_have_monitor (logical_monitor_configs,
+                                                     monitor_spec))
+        continue;

monitor_spec gets leaked here, perhaps just clone when adding to the list ?
Comment 26 Rui Matos 2017-09-29 15:51:22 UTC
Review of attachment 360673 [details] [review]:

lgtm

::: src/backends/native/meta-renderer-native.c
@@ +134,2 @@
   int64_t frame_counter;
+  gboolean pending_set_crtc;

naming nit: this is used only to disable crtcs unlike the similary named variable in the onscreens so using a different name here would be more self-explanatory
Comment 27 Jonas Ådahl 2017-09-29 15:56:37 UTC
(In reply to Rui Matos from comment #25)
> Review of attachment 360672 [details] [review] [review]:
> 
> ::: src/backends/meta-monitor-config-manager.c
> @@ +1216,3 @@
> +                                    MetaMonitorsConfigFlag        flags)
> +{
> +  return meta_monitors_config_new (logical_monitor_configs, NULL,
> layout_mode, flags);
> 
> my suggestion was to populate the disabled monitor specs here automatically
> in this case instead of passing NULL when there might in fact be disabled
> monitors. I guess it doesn't really change anything right now but it seems
> the correct thing to do

Ah, I guess that fine too. It won't have any functional changes, but at least makes things saner if we'd ever start comparing keys in new ways.

> 
> ::: src/backends/meta-monitor-manager.c
> @@ +1861,3 @@
> +      if (meta_logical_monitor_configs_have_monitor
> (logical_monitor_configs,
> +                                                     monitor_spec))
> +        continue;
> 
> monitor_spec gets leaked here, perhaps just clone when adding to the list ?

Oops! Well spotted.

(In reply to Rui Matos from comment #26)
> Review of attachment 360673 [details] [review] [review]:
> 
> lgtm
> 
> ::: src/backends/native/meta-renderer-native.c
> @@ +134,2 @@
>    int64_t frame_counter;
> +  gboolean pending_set_crtc;
> 
> naming nit: this is used only to disable crtcs unlike the similary named
> variable in the onscreens so using a different name here would be more
> self-explanatory

Good point. Maybe 'pending_unset_disabled_crtcs'?
Comment 28 Rui Matos 2017-09-29 15:58:36 UTC
(In reply to Jonas Ådahl from comment #27)
> Good point. Maybe 'pending_unset_disabled_crtcs'?

Sounds good to me
Comment 29 Jonas Ådahl 2017-09-29 19:44:21 UTC
Created attachment 360682 [details] [review]
monitor-config: Keep track of disabled monitors for stored configs

When saving and restoring monitor configurations, we must take disabled
monitors into account, as otherwise one cannot store/restore a
configuration where one or more monitors are explicitly disabled. Make
this possible by adding a <disabled> element to the <configure> element
which lists the monitors that are explicitly disabled. These ones are
included when generating the configuration key, meaning they'll be
picked up correctly.
Comment 30 Rui Matos 2017-10-02 09:28:41 UTC
Review of attachment 360682 [details] [review]:

seems good anyway

::: src/backends/meta-monitor-manager.c
@@ +1962,3 @@
 
+  disabled_monitors_specs =
+    find_disabled_monitor_specs (manager, logical_monitor_configs);

why not expose meta_monitors_config_new_generated() and just call it here instead of having this duplicated function?
Comment 31 Duncan Mac-Vicar P. 2017-10-02 15:38:55 UTC
I switch between two docking stations one with two monitors, the other with a single monitor. I have LCD always off when I am docked.

This used to work correctly, and I had to do nothing once I docked. The settings relative to the current dock where applied.

Will the fix for that bug fix my use-case?
Comment 32 Jonas Ådahl 2017-10-02 20:15:32 UTC
Attachment 360611 [details] pushed as 838df4b - monitor-config-store: Make monitor spec parsing/writing reusable
Attachment 360613 [details] pushed as efdbeb7 - monitor-unit-tests: Add way for test case setup to specify output serial
Attachment 360614 [details] pushed as 99e1cd5 - monitor-unit-tests: Test configs with explicitly disabled monitors
Attachment 360673 [details] pushed as 20749e5 - renderer-native: Unset mode on disabled CRTCs
Attachment 360682 [details] pushed as ea4dbd1 - monitor-config: Keep track of disabled monitors for stored configs