Created attachment 250980 [details] [review]
Adapt to GnomeRR DBus rewrite

A number of APIs were changed to adapt GnomeRR to the new
mutter DBus API.
Also, now that mutter is in charge of the display configuration,
most of the xrandr plugin can be removed. Parts still there are
those handling rotation and fn+f7.


Attaching for Giovanni since his laptop won't work right now.

Review of attachment 250980 [details] [review]:

::: plugins/power/gpm-common.c
@@ -1170,2 @@
         for (i = 0; outputs[i] != NULL; i++) {
-                if (gnome_rr_output_is_connected (outputs[i]) &&

Still not happy about connected being gone.

@@ +1433,2 @@
 int
 backlight_get_min (GnomeRRScreen *rr_screen)

What's the point of doing that? We should remove it.

@@ +1445,3 @@
         output = get_primary_output (rr_screen);
+        if (output != NULL)
+                return 100;

When is it useful not to just return 100?

::: plugins/xrandr/gsd-xrandr-manager.c
@@ -63,3 @@
 #define CONF_SCHEMA "org.gnome.settings-daemon.plugins.xrandr"
 #define CONF_KEY_DEFAULT_MONITORS_SETUP   "default-monitors-setup"
-#define CONF_KEY_DEFAULT_CONFIGURATION_FILE   "default-configuration-file"

I'd like some better reasoning behind removing the ApplyConfiguration method.

(In reply to comment #2)
> Review of attachment 250980 [details] [review]:
> 
> ::: plugins/power/gpm-common.c
> @@ -1170,2 @@
>          for (i = 0; outputs[i] != NULL; i++) {
> -                if (gnome_rr_output_is_connected (outputs[i]) &&
> 
> Still not happy about connected being gone.

Well, it did nothing in the unconnected case...

> @@ +1433,2 @@
>  int
>  backlight_get_min (GnomeRRScreen *rr_screen)
> 
> What's the point of doing that? We should remove it.
> 
> @@ +1445,3 @@
>          output = get_primary_output (rr_screen);
> +        if (output != NULL)
> +                return 100;
> 
> When is it useful not to just return 100?

Unfortunately, normalized 0-100 is what the DBus API exposed, but that wraps XBACKLIGHT, which is X specific.
There is no decent API for backlight except for sysfs, so probably the power plugin will keep using the setuid helper when we move to wayland, and the sysfs interface uses different values for minimum and maximum, depending on HW and driver.

> ::: plugins/xrandr/gsd-xrandr-manager.c
> @@ -63,3 @@
>  #define CONF_SCHEMA "org.gnome.settings-daemon.plugins.xrandr"
>  #define CONF_KEY_DEFAULT_MONITORS_SETUP   "default-monitors-setup"
> -#define CONF_KEY_DEFAULT_CONFIGURATION_FILE   "default-configuration-file"
> 
> I'd like some better reasoning behind removing the ApplyConfiguration method.

Well, ApplyConfiguration means "I just updated monitors.xml, read it again and apply", but I really want to move to a model were nobody except mutter touches monitors.xml, so we can implement failure policies where they belong.

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 250980 [details] [review] [details]:
> > 
> > ::: plugins/power/gpm-common.c
> > @@ -1170,2 @@
> >          for (i = 0; outputs[i] != NULL; i++) {
> > -                if (gnome_rr_output_is_connected (outputs[i]) &&
> > 
> > Still not happy about connected being gone.
> 
> Well, it did nothing in the unconnected case...
> 
> > @@ +1433,2 @@
> >  int
> >  backlight_get_min (GnomeRRScreen *rr_screen)
> > 
> > What's the point of doing that? We should remove it.
> > 
> > @@ +1445,3 @@
> >          output = get_primary_output (rr_screen);
> > +        if (output != NULL)
> > +                return 100;
> > 
> > When is it useful not to just return 100?
> 
> Unfortunately, normalized 0-100 is what the DBus API exposed, but that wraps
> XBACKLIGHT, which is X specific.
> There is no decent API for backlight except for sysfs, so probably the power
> plugin will keep using the setuid helper when we move to wayland, and the sysfs
> interface uses different values for minimum and maximum, depending on HW and
> driver.

I wanted to remove the sysfs stuff :(

It has no concept of multiple outputs (at least not in a way that we can connect to what X gives us).

> > ::: plugins/xrandr/gsd-xrandr-manager.c
> > @@ -63,3 @@
> >  #define CONF_SCHEMA "org.gnome.settings-daemon.plugins.xrandr"
> >  #define CONF_KEY_DEFAULT_MONITORS_SETUP   "default-monitors-setup"
> > -#define CONF_KEY_DEFAULT_CONFIGURATION_FILE   "default-configuration-file"
> > 
> > I'd like some better reasoning behind removing the ApplyConfiguration method.
> 
> Well, ApplyConfiguration means "I just updated monitors.xml, read it again and
> apply", but I really want to move to a model were nobody except mutter touches
> monitors.xml, so we can implement failure policies where they belong.

I meant a justification in the commit messages. I think this should be a separate commit as well (one per plugin/directory would be nice).

Created attachment 251143 [details] [review]
color: adapt to GnomeRR API changes

GnomeRR no longer exposes disconnected outputs, so we don't need
to filter them ourselves.

Created attachment 251144 [details] [review]
power: adapt to GnomeRR API changes

Reading the backlight from GnomeRR can't fail anymore (although
-1 can be returned, meaning that XBACKLIGHT is not supported).
Also, for GnomeRR the value is always a percentage.

We still need to handle min/max in case we use sysfs directly
(which will be the common case when we move to wayland)

Created attachment 251145 [details] [review]
wacom: adapt to GnomeRR API changes

GnomeRR now returns the EDID IDs as strings directly, so we don't
need to convert them to compare to the settings. Also, we don't
need to filter disconnected outputs, it's done by mutter now.

Created attachment 251146 [details] [review]
xrandr: remove ApplyConfiguration DBus method

In the new model, the client that wants to change the display
configuration asks mutter for the new layout directly, and that
takes care of saving to monitors.xml or restoring the existing
configuration if the new one fails.

Created attachment 251147 [details] [review]
xrandr: adapt to the new model for display configuration

Stop handling monitors.xml and the initial modeset for the session,
as well as monitor hotplug and lid switches. All that is implemented
in mutter now.
What is left in the xrandr plugin is Fn-F7 (for lack of a better
place) and Rotate. Both will go in mutter/gnome-shell at some point.

Review of attachment 251143 [details] [review]:

++

Review of attachment 251144 [details] [review]:

++

(Make sure it passes make check in plugins/power)

Review of attachment 251145 [details] [review]:

This file will get cut'n'paste to gnome-control-center. So the g-c-c patch should be replaced by a run of "make update-from-gsd" (see the Makefile.am in panels/wacom)

Review of attachment 251146 [details] [review]:

Looks good.

Review of attachment 251147 [details] [review]:

That looks fine.

Created attachment 252081 [details] [review]
color: adapt to GnomeRR API changes

GnomeRR no longer exposes disconnected outputs, so we don't need
to filter them ourselves.

Created attachment 252082 [details] [review]
power: adapt to GnomeRR API changes

Reading the backlight from GnomeRR can't fail anymore (although
-1 can be returned, meaning that XBACKLIGHT is not supported).
Also, for GnomeRR the value is always a percentage.

We still need to handle min/max in case we use sysfs directly
(which will be the common case when we move to wayland)

Created attachment 252083 [details] [review]
wacom: adapt to GnomeRR API changes

GnomeRR now returns the EDID IDs as strings directly, so we don't
need to convert them to compare to the settings. Also, we don't
need to filter disconnected outputs, it's done by mutter now.

Created attachment 252084 [details] [review]
xrandr: remove ApplyConfiguration DBus method

In the new model, the client that wants to change the display
configuration asks mutter for the new layout directly, and that
takes care of saving to monitors.xml or restoring the existing
configuration if the new one fails.

Created attachment 252085 [details] [review]
xrandr: adapt to the new model for display configuration

Stop handling monitors.xml and the initial modeset for the session,
as well as monitor hotplug and lid switches. All that is implemented
in mutter now.
What is left in the xrandr plugin is Fn-F7 (for lack of a better
place) and Rotate. Both will go in mutter/gnome-shell at some point.

Converted to async initialization.
Sorry but I realized only today that blocking on a process that is only spawned later would not work (because I was thinking in the wayland model, in which mutter is spawned before everything else)

Created attachment 252089 [details] [review]
power: adapt to GnomeRR API changes

Reading the backlight from GnomeRR can't fail anymore (although
-1 can be returned, meaning that XBACKLIGHT is not supported).
Also, for GnomeRR the value is always a percentage.

We still need to handle min/max in case we use sysfs directly
(which will be the common case when we move to wayland)

Another attempt at async construction, because if we're
not careful gnome-shell starts asking for backlight
before the GnomeRRScreen is loaded.

Review of attachment 252081 [details] [review]:

still looks fine

Review of attachment 252083 [details] [review]:

still looks fine

Review of attachment 252084 [details] [review]:

still looks fine

Review of attachment 252085 [details] [review]:

still looks fine

Review of attachment 252089 [details] [review]:

still looks fine. Make sure the testsuite in plugins/power passes.

Created attachment 252161 [details] [review]
power: restore the ability of running the test suite

We need to provide a mock mutter in the test environment.
Also, we need to make sure a few objects exist right away, as
the mock environment starts dbus calls before we're really
initialized.

Review of attachment 252161 [details] [review]:

Why do you need to double the timeouts in the blank checks ? Are thinkgs slower now, or was the test unreliable before ?

(In reply to comment #28)
> Review of attachment 252161 [details] [review]:
> 
> Why do you need to double the timeouts in the blank checks ? Are thinkgs slower
> now, or was the test unreliable before ?

Nah, it was a mistaken attempt to debug a failure that was instead caused by the test environment. Will remove.

Attachment 252081 [details] pushed as 7167c78 - color: adapt to GnomeRR API changes
Attachment 252083 [details] pushed as c0d0112 - wacom: adapt to GnomeRR API changes
Attachment 252084 [details] pushed as 4c8fa6e - xrandr: remove ApplyConfiguration DBus method
Attachment 252085 [details] pushed as 2f78a82 - xrandr: adapt to the new model for display configuration
Attachment 252089 [details] pushed as ddfef4e - power: adapt to GnomeRR API changes
Attachment 252161 [details] pushed as 0eb96d7 - power: restore the ability of running the test suite