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 705507 - Merge the wip/wayland-display branch
Merge the wip/wayland-display branch
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: xrandr
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Debarshi Ray
gnome-settings-daemon-maint
wayland
Depends on: 705510
Blocks:
 
 
Reported: 2013-08-05 12:46 UTC by Matthias Clasen
Modified: 2013-08-19 07:54 UTC
See Also:
GNOME target: 3.10
GNOME version: ---


Attachments
Adapt to GnomeRR DBus rewrite (45.89 KB, patch)
2013-08-06 15:01 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
color: adapt to GnomeRR API changes (2.39 KB, patch)
2013-08-08 12:13 UTC, Giovanni Campagna
accepted-commit_now Details | Review
power: adapt to GnomeRR API changes (6.87 KB, patch)
2013-08-08 12:13 UTC, Giovanni Campagna
accepted-commit_now Details | Review
wacom: adapt to GnomeRR API changes (4.21 KB, patch)
2013-08-08 12:13 UTC, Giovanni Campagna
accepted-commit_now Details | Review
xrandr: remove ApplyConfiguration DBus method (4.03 KB, patch)
2013-08-08 12:14 UTC, Giovanni Campagna
accepted-commit_now Details | Review
xrandr: adapt to the new model for display configuration (33.11 KB, patch)
2013-08-08 12:14 UTC, Giovanni Campagna
accepted-commit_now Details | Review
color: adapt to GnomeRR API changes (5.05 KB, patch)
2013-08-17 23:34 UTC, Giovanni Campagna
committed Details | Review
power: adapt to GnomeRR API changes (9.70 KB, patch)
2013-08-17 23:34 UTC, Giovanni Campagna
none Details | Review
wacom: adapt to GnomeRR API changes (6.09 KB, patch)
2013-08-17 23:34 UTC, Giovanni Campagna
committed Details | Review
xrandr: remove ApplyConfiguration DBus method (4.03 KB, patch)
2013-08-17 23:34 UTC, Giovanni Campagna
committed Details | Review
xrandr: adapt to the new model for display configuration (34.37 KB, patch)
2013-08-17 23:34 UTC, Giovanni Campagna
committed Details | Review
power: adapt to GnomeRR API changes (12.71 KB, patch)
2013-08-18 00:22 UTC, Giovanni Campagna
committed Details | Review
power: restore the ability of running the test suite (7.50 KB, patch)
2013-08-18 21:25 UTC, Giovanni Campagna
committed Details | Review

Description Matthias Clasen 2013-08-05 12:46:21 UTC
For 3.10, we want to support display configuration in the Wayland tech preview. To do that, we'll move the xrandr code from gnome-desktop behind a new D-Bus interface in mutter (or gnome-shell), and use that from gnome-settings-daemon.

The code is in the wip/wayland-display branch.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-08-06 15:01:41 UTC
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.
Comment 2 Bastien Nocera 2013-08-07 09:08:47 UTC
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.
Comment 3 Giovanni Campagna 2013-08-07 09:24:12 UTC
(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.
Comment 4 Bastien Nocera 2013-08-07 09:30:01 UTC
(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).
Comment 5 Giovanni Campagna 2013-08-08 12:13:43 UTC
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.
Comment 6 Giovanni Campagna 2013-08-08 12:13:48 UTC
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)
Comment 7 Giovanni Campagna 2013-08-08 12:13:52 UTC
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.
Comment 8 Giovanni Campagna 2013-08-08 12:14:22 UTC
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.
Comment 9 Giovanni Campagna 2013-08-08 12:14:28 UTC
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.
Comment 10 Bastien Nocera 2013-08-08 14:00:29 UTC
Review of attachment 251143 [details] [review]:

++
Comment 11 Bastien Nocera 2013-08-08 14:01:49 UTC
Review of attachment 251144 [details] [review]:

++

(Make sure it passes make check in plugins/power)
Comment 12 Bastien Nocera 2013-08-08 14:53:49 UTC
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)
Comment 13 Bastien Nocera 2013-08-08 14:54:25 UTC
Review of attachment 251146 [details] [review]:

Looks good.
Comment 14 Bastien Nocera 2013-08-08 14:55:40 UTC
Review of attachment 251147 [details] [review]:

That looks fine.
Comment 15 Giovanni Campagna 2013-08-17 23:34:34 UTC
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.
Comment 16 Giovanni Campagna 2013-08-17 23:34:38 UTC
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)
Comment 17 Giovanni Campagna 2013-08-17 23:34:42 UTC
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.
Comment 18 Giovanni Campagna 2013-08-17 23:34:46 UTC
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.
Comment 19 Giovanni Campagna 2013-08-17 23:34:50 UTC
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.
Comment 20 Giovanni Campagna 2013-08-17 23:36:11 UTC
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)
Comment 21 Giovanni Campagna 2013-08-18 00:22:16 UTC
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.
Comment 22 Matthias Clasen 2013-08-18 18:07:41 UTC
Review of attachment 252081 [details] [review]:

still looks fine
Comment 23 Matthias Clasen 2013-08-18 18:09:47 UTC
Review of attachment 252083 [details] [review]:

still looks fine
Comment 24 Matthias Clasen 2013-08-18 18:10:42 UTC
Review of attachment 252084 [details] [review]:

still looks fine
Comment 25 Matthias Clasen 2013-08-18 18:11:59 UTC
Review of attachment 252085 [details] [review]:

still looks fine
Comment 26 Matthias Clasen 2013-08-18 18:14:49 UTC
Review of attachment 252089 [details] [review]:

still looks fine. Make sure the testsuite in plugins/power passes.
Comment 27 Giovanni Campagna 2013-08-18 21:25:53 UTC
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.
Comment 28 Matthias Clasen 2013-08-18 22:00:02 UTC
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 ?
Comment 29 Giovanni Campagna 2013-08-18 22:12:55 UTC
(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.
Comment 30 Giovanni Campagna 2013-08-19 07:53:57 UTC
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