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 777732 - Handle all low level monitor configuration in mutter
Handle all low level monitor configuration in mutter
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-01-25 08:26 UTC by Jonas Ådahl
Modified: 2017-08-21 04:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
monitor-config-manager: Handle comparing different sized config keys (973 bytes, patch)
2017-04-13 05:09 UTC, Jonas Ådahl
committed Details | Review
monitor-config-store: Replace key when replacing config (1.53 KB, patch)
2017-04-13 05:09 UTC, Jonas Ådahl
committed Details | Review
backend: Make the getter use the cached ui scale (2.06 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/xrandr: Use xcb API to configure CRTC (7.98 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
backend: Make X11 display opened function explicitly named (3.11 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
backend: Move settings into a new MetaSettings object (35.29 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/kms: Move global ui scaling setting to MetaSettings (6.86 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/kms: Move scale calculation to MetaMonitor (7.25 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
committed Details | Review
window/wayland: Don't try to resize window on tear down (1.25 KB, patch)
2017-05-04 06:48 UTC, Jonas Ådahl
none Details | Review
gschema: Add description for "monitor-config-manager" (1.80 KB, patch)
2017-05-04 06:49 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/xrandr: Allow configuring scales on X11 too (40.18 KB, patch)
2017-05-04 06:49 UTC, Jonas Ådahl
none Details | Review
(gnome-shell) shell-global: Use MetaSettings for settings (3.18 KB, patch)
2017-05-04 07:00 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/xrandr: Allow configuring scales on X11 too (40.13 KB, patch)
2017-05-24 08:12 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/xrandr: Fix name inconsistencies (9.62 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
Remove old monitor configuration system (139.41 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/x11: Don't complain about 'normal' transform (1.19 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
monitor-manager: Fix output variable naming (3.24 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
monitor-manager/kms: Use connector id to find old output (1.30 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
Migrate old monitor configuration files to new system (64.37 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
tests: Add monitor config migration tests (24.87 KB, patch)
2017-08-11 07:29 UTC, Jonas Ådahl
committed Details | Review
tests: Add rotated multi head monitor config migration test (4.21 KB, patch)
2017-08-11 07:30 UTC, Jonas Ådahl
committed Details | Review
monitor-config-manager: Keep short history of configurations (5.11 KB, patch)
2017-08-11 07:30 UTC, Jonas Ådahl
committed Details | Review
monitor-manager: Try to restore previous config before regenerating (3.66 KB, patch)
2017-08-11 07:30 UTC, Jonas Ådahl
committed Details | Review
[gnome-control-center] panels/display: Remove support for old config system (41.43 KB, patch)
2017-08-11 07:32 UTC, Jonas Ådahl
committed Details | Review
[gnome-settings-daemon] xsettings: Only support the new mutter D-Bus DisplayConfig API (8.72 KB, patch)
2017-08-11 07:35 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2017-01-25 08:26:32 UTC
This bug is a place holder for the progress related to moving all low level monitor logic (i.e. CRTC/connector/... things into mutter, hiding it behind a higher level configuration API).

While the current API simply provides a getter and a setter for all KMS like state, the new will be placed a layer above, with a set of "monitors" that can be placed in "logical monitors". One will be able to rotate a logical monitor, set a scale, place multiple monitors inside one (i.e. mirroring).
Comment 1 Jonas Ådahl 2017-01-25 08:50:12 UTC
First batch landed:

https://git.gnome.org/browse/mutter/log/?qt=range&q=9b64e09a24643264a537970572deaabe1c1098b7..6fa8238ebf5f1a1854b748533ad99ea15d647896

Current state:

* Disabled by default
* Missing D-Bus API (read part available on https://github.com/jadahl/mutter/commits/wip/monitor-config )
* Doesn't write config to disk
* Configuring transforms (rotation) missing
* Configuring scale (available on https://github.com/jadahl/mutter/commits/wip/monitor-config )
Comment 2 Olivier Fourdan 2017-02-21 07:30:59 UTC
fwiw I filed bug 779001 because this braks mutter standalone for me.
Comment 3 Frank 2017-04-08 01:38:35 UTC
Is far as I read your commits of today, my issue #771559 seems resolved if control-center gets enabled to control the scaling factor (via your "int->float" commit) appropriately, am I right?

If mutter is able to scale monitors individually, that would be really great, I'd need this at work! :D
Comment 4 Jonas Ådahl 2017-04-08 01:49:02 UTC
(In reply to Frank from comment #3)
> Is far as I read your commits of today, my issue #771559 seems resolved if
> control-center gets enabled to control the scaling factor (via your
> "int->float" commit) appropriately, am I right?

The compositor will report what scales are supported. Fractional scaling is not supported yet, its only the API that has been adapted to eventually make it possible.

> 
> If mutter is able to scale monitors individually, that would be really
> great, I'd need this at work! :D

You can try that by enabling the relevant experimental features (see the gscheme file). Note that things like the shell chrome won't be displayed properly yet, it is still in the experimental phase.
Comment 6 Jonas Ådahl 2017-04-13 05:09:45 UTC
Created attachment 349759 [details] [review]
monitor-config-manager: Handle comparing different sized config keys

The guard for handling size differences between keys were broken, it
only checked if the key passed by the second argument ended up being
shorter.
Comment 7 Jonas Ådahl 2017-04-13 05:09:52 UTC
Created attachment 349760 [details] [review]
monitor-config-store: Replace key when replacing config

g_hash_table_insert() doesn't replace the key. This was a problem
because the key was owned by the value inserted into the hash table, so
when a value was removed, the key was freed, meaning that the key in
the hash table was no pointing to freed memory. Fix this by using
g_hash_table_replace() instead, which work the same except that it
replaces the key with the one passed. This means that the key of a
value in the hash table is always the key owned by the value.
Comment 8 Rui Matos 2017-04-13 07:31:10 UTC
Review of attachment 349759 [details] [review]:

++
Comment 9 Rui Matos 2017-04-13 07:31:38 UTC
Review of attachment 349760 [details] [review]:

right, g_hash_table_insert() strikes again
Comment 10 Jonas Ådahl 2017-04-17 09:46:41 UTC
Attachment 349759 [details] pushed as 3f107da - monitor-config-manager: Handle comparing different sized config keys
Attachment 349760 [details] pushed as 0608ae2 - monitor-config-store: Replace key when replacing config
Comment 11 Jonas Ådahl 2017-05-04 06:48:14 UTC
Created attachment 351028 [details] [review]
backend: Make the getter use the cached ui scale

The cached ui scale is kept up to date, so don't recalculate it
everytime meta_backend_get_ui_scaling() is called.
Comment 12 Jonas Ådahl 2017-05-04 06:48:19 UTC
Created attachment 351029 [details] [review]
monitor-manager/xrandr: Use xcb API to configure CRTC

Use xcb-randr instead of libXrandr to set the CRTC configuration. This
is needed because data from the reply will later be used.
Comment 13 Jonas Ådahl 2017-05-04 06:48:25 UTC
Created attachment 351030 [details] [review]
backend: Make X11 display opened function explicitly named

It didn't say anything about being the X11 display, so make it say so.
Comment 14 Jonas Ådahl 2017-05-04 06:48:32 UTC
Created attachment 351031 [details] [review]
backend: Move settings into a new MetaSettings object

Introduce MetaSettings and add the settings managed by MetaBackend into
the new object. These settings include: experimental-features and UI
scaling factor.
Comment 15 Jonas Ådahl 2017-05-04 06:48:43 UTC
Created attachment 351032 [details] [review]
monitor-manager/kms: Move global ui scaling setting to MetaSettings

It'll be used elsewhere, so shouldn't be in MetaMonitorManagerKms.
Comment 16 Jonas Ådahl 2017-05-04 06:48:49 UTC
Created attachment 351033 [details] [review]
monitor-manager/kms: Move scale calculation to MetaMonitor

The scale calculation doesn't really have anything to do with KMS, and
eventually we'll want to have mutter calculate the monitor scale for
non-KMS backends too, so move the scale calculation to MetaMonitor.
Comment 17 Jonas Ådahl 2017-05-04 06:48:54 UTC
Created attachment 351034 [details] [review]
window/wayland: Don't try to resize window on tear down

When exiting mutter, don't try to resize maximized windows when
unmanaging, as at this point, they will have no MetaWaylandSurface.
Originally this was done instead of setting the net_wm_state to not
mess with future window managers, but when we're a Wayland compositor,
this does not matter.
Comment 18 Jonas Ådahl 2017-05-04 06:49:00 UTC
Created attachment 351035 [details] [review]
gschema: Add description for "monitor-config-manager"
Comment 19 Jonas Ådahl 2017-05-04 06:49:06 UTC
Created attachment 351036 [details] [review]
monitor-manager/xrandr: Allow configuring scales on X11 too

This commit makes it possible to configure logical monitor scale also
when running on top of an X11 server using Xrandr. An extra property
'requires-globla-scale' is added to the D-Bus API is added to instruct
a configuration application to only allow setting a global logical
monitor scale.

This is needed to let gsd-xsettings use the configured state to set a
XSettings state that respects the explicit monitor configuration.
Comment 20 Jonas Ådahl 2017-05-04 07:00:44 UTC
Created attachment 351037 [details] [review]
(gnome-shell) shell-global: Use MetaSettings for settings

Scale settings were moved from MetaBackend to a new MetaSettings.
Comment 21 Jonas Ådahl 2017-05-04 08:30:43 UTC
Comment on attachment 351034 [details] [review]
window/wayland: Don't try to resize window on tear down

Marking "window/wayland: Don't try to resize window on tear down" as obsolete. It is unrelated, and I'll attach that to a separate bug.
Comment 22 Rui Matos 2017-05-23 15:43:59 UTC
Review of attachment 351028 [details] [review]:

Looks good. The usage in experimental_features_changed() seems like it could be removed since the notification should be done automatically
Comment 23 Rui Matos 2017-05-23 15:55:06 UTC
Review of attachment 351030 [details] [review]:

::: src/Makefile.am
@@ +83,3 @@
 	backends/meta-backend.c			\
 	meta/meta-backend.h			\
+	meta/meta-settings.h			\

this seems unrelated

::: src/backends/meta-backend-private.h
@@ +39,3 @@
 #include "backends/meta-pointer-constraint.h"
 #include "backends/meta-renderer.h"
+#include "backends/meta-settings-private.h"

unrelated
Comment 24 Rui Matos 2017-05-23 16:15:36 UTC
Review of attachment 351030 [details] [review]:

I see you just added those makefile/include changes in the wrong patch, they just need to be moved to the following one
Comment 25 Rui Matos 2017-05-23 16:16:07 UTC
Review of attachment 351031 [details] [review]:

looks fine

::: src/backends/meta-monitor.c
@@ +26,3 @@
 #include "backends/meta-backend-private.h"
 #include "backends/meta-monitor-manager-private.h"
+#include "backends/meta-settings-private.h"

?

::: src/backends/meta-settings.c
@@ +256,3 @@
+#if 0
+void
+meta_settings_register_proxy (MetaSettings      *settings,

do we need to keep this #if 0'd code around?
Comment 26 Rui Matos 2017-05-23 16:22:17 UTC
Review of attachment 351032 [details] [review]:

::: src/backends/meta-settings.c
@@ +356,3 @@
+  /* Chain up inter-dependent settings. */
+  g_signal_connect (settings, "global-scaling-factor-changed",
+                    G_CALLBACK (meta_settings_update_ui_scaling_factor), NULL);

connecting to our own signals looks strange, why not call this directly when needed?
Comment 27 Rui Matos 2017-05-23 16:24:19 UTC
Review of attachment 351033 [details] [review]:

Ah, I see how that meta-settings-private.h header showed up in meta-monitor.c in a previous commit :-)
Comment 28 Rui Matos 2017-05-23 16:25:42 UTC
Review of attachment 351035 [details] [review]:

I think we should also enable this by default in master as we talked about on IRC
Comment 29 Rui Matos 2017-05-23 17:09:40 UTC
Review of attachment 351037 [details] [review]:

ok
Comment 30 Rui Matos 2017-05-23 17:12:01 UTC
Review of attachment 351036 [details] [review]:

seems right but didn't test this yet

::: src/backends/x11/meta-monitor-manager-xrandr.c
@@ +1120,3 @@
 
+static gboolean
+is_crtc_assignment_unchanged (MetaCrtc      *crtc,

all the double negations caused by these functions being _unchanged instead of _changed make reading the logic harder than it could be
Comment 31 Rui Matos 2017-05-23 17:12:40 UTC
Review of attachment 351029 [details] [review]:

seems fine otherwise

::: src/backends/x11/meta-monitor-manager-xrandr.c
@@ +996,2 @@
     case META_MONITOR_TRANSFORM_FLIPPED:
+      return RR_Reflect_X | XCB_RANDR_ROTATION_ROTATE_0;

missed this RR_Reflect_X
Comment 32 Jonas Ådahl 2017-05-24 02:28:24 UTC
(In reply to Rui Matos from comment #25)
> Review of attachment 351031 [details] [review] [review]:
> 
> looks fine
> 
> ::: src/backends/meta-monitor.c
> @@ +26,3 @@
>  #include "backends/meta-backend-private.h"
>  #include "backends/meta-monitor-manager-private.h"
> +#include "backends/meta-settings-private.h"
> 
> ?
> 
> ::: src/backends/meta-settings.c
> @@ +256,3 @@
> +#if 0
> +void
> +meta_settings_register_proxy (MetaSettings      *settings,
> 
> do we need to keep this #if 0'd code around?

No. These "register" things was part of a preparation for attaching an XSettings "proxy" to MetaSettings, but discarded that idea for now, but obviously did a bad job at git history surgery, here and on other commits :P
Comment 33 Jonas Ådahl 2017-05-24 08:06:19 UTC
(In reply to Rui Matos from comment #26)
> Review of attachment 351032 [details] [review] [review]:
> 
> ::: src/backends/meta-settings.c
> @@ +356,3 @@
> +  /* Chain up inter-dependent settings. */
> +  g_signal_connect (settings, "global-scaling-factor-changed",
> +                    G_CALLBACK (meta_settings_update_ui_scaling_factor),
> NULL);
> 
> connecting to our own signals looks strange, why not call this directly when
> needed?

Just thought it would be nice to have inter-dependencies listed like this, making the "meta_update_configuration()" just update the configuration. I can change it to add "meta_update_other_configuration()" to "meta_update_configuration()" if you prefer.
Comment 34 Jonas Ådahl 2017-05-24 08:12:21 UTC
Created attachment 352476 [details] [review]
monitor-manager/xrandr: Allow configuring scales on X11 too

This commit makes it possible to configure logical monitor scale also
when running on top of an X11 server using Xrandr. An extra property
'requires-globla-scale' is added to the D-Bus API is added to instruct
a configuration application to only allow setting a global logical
monitor scale.

This is needed to let gsd-xsettings use the configured state to set a
XSettings state that respects the explicit monitor configuration.

-----------

This changes ".._unchanged()" to "..._changed()" as suggested.
Comment 35 Rui Matos 2017-05-24 13:24:11 UTC
Review of attachment 351032 [details] [review]:

(In reply to Jonas Ådahl from comment #33)
> Just thought it would be nice to have inter-dependencies listed like this,
> making the "meta_update_configuration()" just update the configuration.

Ok, makes sense. It caught my eye because it's not a common idiom
Comment 36 Rui Matos 2017-05-24 13:49:17 UTC
Review of attachment 352476 [details] [review]:

lgtm
Comment 37 Jonas Ådahl 2017-05-26 07:20:12 UTC
Attachment 351028 [details] pushed as 43cdf81 - backend: Make the getter use the cached ui scale
Attachment 351029 [details] pushed as df39a7d - monitor-manager/xrandr: Use xcb API to configure CRTC
Attachment 351030 [details] pushed as be17555 - backend: Make X11 display opened function explicitly named
Attachment 351031 [details] pushed as 2718699 - backend: Move settings into a new MetaSettings object
Attachment 351032 [details] pushed as 0bc312a - monitor-manager/kms: Move global ui scaling setting to MetaSettings
Attachment 351033 [details] pushed as 3b097c7 - monitor-manager/kms: Move scale calculation to MetaMonitor
Attachment 351035 [details] pushed as 05bc2e2 - gschema: Add description for "monitor-config-manager"
Attachment 352476 [details] pushed as 1bb0e18 - monitor-manager/xrandr: Allow configuring scales on X11 too
Comment 38 Jonas Ådahl 2017-08-11 07:29:08 UTC
Created attachment 357383 [details] [review]
monitor-manager/xrandr: Fix name inconsistencies

MetaOutputs should be "output" and external types should be named as
such, i.e. XRandr outputs are renamed to "xrandr_output".
Comment 39 Jonas Ådahl 2017-08-11 07:29:16 UTC
Created attachment 357384 [details] [review]
Remove old monitor configuration system

Remove the old MetaMonitorConfig system and mark the new one as
non-experimental. This also removes the D-Bus property.
Comment 40 Jonas Ådahl 2017-08-11 07:29:23 UTC
Created attachment 357385 [details] [review]
monitor-manager/x11: Don't complain about 'normal' transform

The 'normal' transform has the value 0, so the g_warn_if_fail()
expression failed. Correct it so that it doesn't complain when no
transform is checked.
Comment 41 Jonas Ådahl 2017-08-11 07:29:29 UTC
Created attachment 357386 [details] [review]
monitor-manager: Fix output variable naming

Fix the last case of using the variable name "meta_output" for a
MetaOutput.
Comment 42 Jonas Ådahl 2017-08-11 07:29:35 UTC
Created attachment 357387 [details] [review]
monitor-manager/kms: Use connector id to find old output

The zero-initialized winsys id was incorrectly used as the key to find
the old output to base active/primary state from, which would never
succeed unless the winsys id happened to be 0. Fix this by using the
winsys id that will be used, i.e. the connector id.
Comment 43 Jonas Ådahl 2017-08-11 07:29:48 UTC
Created attachment 357388 [details] [review]
Migrate old monitor configuration files to new system

This commit changes the new configuration system to use monitors.xml
instead of monitors-experimental.xml. When starting up and the
monitors.xml file is loaded, if a legacy monitors.xml file is
discovered (it has the version number 1), an attempt is made to migrate
the stored configuration onto the new system.

This is done in two steps:

1) Parsing and translation of the old configuration. This works by
parsing file using the mostly the old parser, but then translating the
resulting configuration structs into the new configuration system. As
the legacy configuration system doesn't carry over some state (such as
tiling and scale used), some things are not available. For tiling, the
migration paths makes an attempt to discover tiled monitors by
comparing EDID data, and guessing what the main tile is. Determination
of the scale of a migrated configuration is postponed until the
configuration is actually applied. This works by flagging the
configuration as 'migrated'.

2) Finishing the migration when applying. When a configuration with the
'migrated' flag is retrieved from the configuration store, the final
step of the migration is taken place. This involves calculating the
preferred scale given the mode configured, while making sure this
doesn't result in any overlapping logical monitor regions etc.
Comment 44 Jonas Ådahl 2017-08-11 07:29:55 UTC
Created attachment 357389 [details] [review]
tests: Add monitor config migration tests

So far some basic testing, including:

 * Test that the migrated configuration is applicable
 * Test that a monitors.xml with multiple configurations are translated
 * Test rotation
 * Test tiled monitor discovery (well, test a made up tiled monitor
   configuration since I don't have a real one)
Comment 45 Jonas Ådahl 2017-08-11 07:30:03 UTC
Created attachment 357390 [details] [review]
tests: Add rotated multi head monitor config migration test
Comment 46 Jonas Ådahl 2017-08-11 07:30:10 UTC
Created attachment 357391 [details] [review]
monitor-config-manager: Keep short history of configurations

In order to go back in monitor configurations, save them to a history.
The history is implemented as a max 3 element long queue, where newly
set configurations are pushed to the head, and old are popped from the
tail.

The difference between using a single previous config reference and a
queue is that we can now remember the configuration used prior to a
D-Bus triggered configuration when the user discarded the configuration.

This will later be used to restore a previous configuration when a
laptop lid is opened.
Comment 47 Jonas Ådahl 2017-08-11 07:30:19 UTC
Created attachment 357392 [details] [review]
monitor-manager: Try to restore previous config before regenerating

When opening a laptop lid, one will likely want to restore the
configuration one had prior to closing it, so when ensuring monitor
configuration, first try to see if the previously set configuration is
both complete (all connected monitors are configured) and applicable
(it is a valid configuration) and only try to generate a new from
scratch if that failed.
Comment 48 Jonas Ådahl 2017-08-11 07:32:24 UTC
Created attachment 357393 [details] [review]
[gnome-control-center] panels/display: Remove support for old config system

As mutter now has removed the legacy monitor config system and the
associated property on org.gnome.Mutter.DisplayConfig, also remove the
support from gnome-control-center.
Comment 49 Jonas Ådahl 2017-08-11 07:35:21 UTC
Created attachment 357394 [details] [review]
[gnome-settings-daemon] xsettings: Only support the new mutter D-Bus DisplayConfig API

The legacy API is no longer supported by mutter, so lets remove it from
gnome-settings-daemon too.
Comment 50 Florian Müllner 2017-08-11 19:02:09 UTC
Review of attachment 357383 [details] [review]:

LGTM
Comment 51 Florian Müllner 2017-08-11 19:02:11 UTC
Review of attachment 357384 [details] [review]:

::: src/backends/meta-monitor-config-manager.c
@@ +1148,3 @@
 
+void
+meta_crtc_info_free (MetaCrtcInfo *info)

It's a bit odd to the the declarations in monitor-manager-private, and the definitions in monitor-config-manager.c (although that was already true for monitor-config.c of course). I don't see those functions used anywhere else now, so maybe make them static and remove the declarations altogether?

::: src/tests/meta-monitor-manager-test.c
@@ +118,3 @@
   config = meta_monitor_manager_ensure_configured (manager);
 
+  if (meta_is_stage_views_enabled ())

Mmmh, this is unexpected - shouldn't the conditional be removed altogether now?
Comment 52 Florian Müllner 2017-08-11 19:02:15 UTC
Review of attachment 357385 [details] [review]:

Makes sense to me
Comment 53 Florian Müllner 2017-08-11 19:02:18 UTC
Review of attachment 357386 [details] [review]:

Love the commit message ;-)
Comment 54 Florian Müllner 2017-08-11 19:02:22 UTC
Review of attachment 357387 [details] [review]:

OK
Comment 55 Rui Matos 2017-08-18 12:09:54 UTC
Review of attachment 357384 [details] [review]:

::: data/org.gnome.mutter.gschema.xml.in
@@ -115,3 @@
         Currently possible keywords:
 
-        • “monitor-config-manager”    — use the new monitor configuration

we should remove it from the default value too
Comment 56 Rui Matos 2017-08-18 12:38:51 UTC
Review of attachment 357388 [details] [review]:

looks good

::: src/backends/meta-monitor-config-migration.c
@@ +1147,3 @@
+    }
+
+  configs = load_config_file (user_file, error);

is this needed?
Comment 57 Rui Matos 2017-08-18 12:43:22 UTC
Review of attachment 357391 [details] [review]:

sure
Comment 58 Rui Matos 2017-08-18 12:44:48 UTC
Review of attachment 357392 [details] [review]:

lgtm
Comment 59 Rui Matos 2017-08-18 12:49:04 UTC
Review of attachment 357393 [details] [review]:

looks great
Comment 60 Jonas Ådahl 2017-08-18 12:49:48 UTC
Review of attachment 357388 [details] [review]:

::: src/backends/meta-monitor-config-migration.c
@@ +1147,3 @@
+    }
+
+  configs = load_config_file (user_file, error);

Hmm, looks like I changed to use the meta_migrate_old_monitors_config() (below) but didn't remove the old load thingy.
Comment 61 Rui Matos 2017-08-18 13:08:57 UTC
Review of attachment 357394 [details] [review]:

We can now drop all usage of libgnome-desktop here by connecting directly to the MonitorsChanged mutter DBus signal. I can do that on a follow up patch afterwards
Comment 62 Rui Matos 2017-08-18 13:10:32 UTC
Didn't check the tests thoroughly but all seems sane to me
Comment 63 Jonas Ådahl 2017-08-21 03:16:51 UTC
(In reply to Florian Müllner from comment #51)
> Review of attachment 357384 [details] [review] [review]:
> 
> ::: src/backends/meta-monitor-config-manager.c
> @@ +1148,3 @@
>  
> +void
> +meta_crtc_info_free (MetaCrtcInfo *info)
> 
> It's a bit odd to the the declarations in monitor-manager-private, and the
> definitions in monitor-config-manager.c (although that was already true for
> monitor-config.c of course). I don't see those functions used anywhere else
> now, so maybe make them static and remove the declarations altogether?

Sounds good.

> 
> ::: src/tests/meta-monitor-manager-test.c
> @@ +118,3 @@
>    config = meta_monitor_manager_ensure_configured (manager);
>  
> +  if (meta_is_stage_views_enabled ())
> 
> Mmmh, this is unexpected - shouldn't the conditional be removed altogether
> now?

I have some ideas of being able to run the tests with stage views disabled to mimic how X11 handles things, which is why I left them as is.
Comment 64 Jonas Ådahl 2017-08-21 04:47:31 UTC
Attachment 357383 [details] pushed as 9ac87b3 - monitor-manager/xrandr: Fix name inconsistencies
Attachment 357384 [details] pushed as e8a6286 - Remove old monitor configuration system
Attachment 357385 [details] pushed as ab04286 - monitor-manager/x11: Don't complain about 'normal' transform
Attachment 357386 [details] pushed as 88f2441 - monitor-manager: Fix output variable naming
Attachment 357387 [details] pushed as 27a4f9f - monitor-manager/kms: Use connector id to find old output
Attachment 357388 [details] pushed as bc31624 - Migrate old monitor configuration files to new system
Attachment 357389 [details] pushed as 6195075 - tests: Add monitor config migration tests
Attachment 357390 [details] pushed as 6c0f107 - tests: Add rotated multi head monitor config migration test
Attachment 357391 [details] pushed as b140e7f - monitor-config-manager: Keep short history of configurations
Attachment 357392 [details] pushed as 35c9280 - monitor-manager: Try to restore previous config before regenerating
Comment 65 Jonas Ådahl 2017-08-21 04:49:05 UTC
The g-c-c and g-s-d was pushed too, so I guess we can close this one now.