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 749671 - system: Use iio-sensor-proxy directly
system: Use iio-sensor-proxy directly
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 745682 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-05-21 09:58 UTC by Bastien Nocera
Modified: 2015-05-27 08:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
system: Use iio-sensor-proxy directly (3.57 KB, patch)
2015-05-21 09:58 UTC, Bastien Nocera
none Details | Review
system: Use iio-sensor-proxy directly (3.57 KB, patch)
2015-05-21 09:59 UTC, Bastien Nocera
needs-work Details | Review
system: Use iio-sensor-proxy to detect accelerometer (4.01 KB, patch)
2015-05-21 14:17 UTC, Bastien Nocera
none Details | Review
system: Use iio-sensor-proxy to detect accelerometer (4.27 KB, patch)
2015-05-21 14:25 UTC, Bastien Nocera
none Details | Review
system: Use iio-sensor-proxy to detect accelerometer (4.27 KB, patch)
2015-05-22 13:32 UTC, Bastien Nocera
none Details | Review
system: Use iio-sensor-proxy to detect accelerometer (4.01 KB, patch)
2015-05-27 08:43 UTC, Florian Müllner
committed Details | Review

Description Bastien Nocera 2015-05-21 09:58:10 UTC
.
Comment 1 Bastien Nocera 2015-05-21 09:58:15 UTC
Created attachment 303733 [details] [review]
system: Use iio-sensor-proxy directly

Instead of using gnome-settings-daemon's D-Bus interface.
Comment 2 Bastien Nocera 2015-05-21 09:59:21 UTC
Created attachment 303734 [details] [review]
system: Use iio-sensor-proxy directly

Instead of using gnome-settings-daemon's D-Bus interface.
Comment 3 Bastien Nocera 2015-05-21 10:03:28 UTC
To test, with https://github.com/hadess/iio-sensor-proxy :
- launch fake-input-accelerometer as root
- launch iio-sensor-proxy as root
- check that the orientation lock is visible in the menu
- kill fake-input-accelerometer
- check that the orientation lock is not visible in the menu
- check that iio-sensor-proxy closes
- check that the orientation lock is not visible in the menu
Comment 4 Florian Müllner 2015-05-21 10:58:09 UTC
Review of attachment 303734 [details] [review]:

In addition to the comments below, please add a short explanation for the reasons for this change to the commit message (also "instead of" is a bit misleading if we are still using the gsd interface after all)

::: js/ui/status/system.js
@@ +28,3 @@
 
+const BUS_NAME = 'net.hadess.SensorProxy';
+const OBJECT_PATH = '/net/hadess/SensorProxy';

Some less generic names please

@@ +161,1 @@
         this._orientationExists = false;

No longer used

@@ +161,2 @@
         this._orientationExists = false;
         Gio.DBus.session.watch_name('org.gnome.SettingsDaemon.Orientation',

So we're still watching the gsd name, presumably because we still want to be bound to the plugin's 'active' setting - any other reasons?

@@ +178,3 @@
+                                                 if (error) {
+                                                     log(error.message);
+                                                 return;

Indentation gone awry

@@ +181,3 @@
+                                             }
+                                             this._proxy.connect('g-properties-changed',
+                                                                 Lang.bind(this, this._sync));

This method doesn't exist, I assume you mean _updateOrientationLock()?
Comment 5 Florian Müllner 2015-05-21 11:40:33 UTC
*** Bug 745682 has been marked as a duplicate of this bug. ***
Comment 6 Florian Müllner 2015-05-21 11:42:52 UTC
Review of attachment 303734 [details] [review]:

::: js/ui/status/system.js
@@ +184,3 @@
+                                             this._updateOrientationLock();
+                                             }));
+    }

There's also a missing comma here, resulting in a syntax error - are you sure you did attach the correct patch version?
Comment 7 Bastien Nocera 2015-05-21 14:17:07 UTC
(In reply to Florian Müllner from comment #4)
> Review of attachment 303734 [details] [review] [review]:
> 
> In addition to the comments below, please add a short explanation for the
> reasons for this change to the commit message (also "instead of" is a bit
> misleading if we are still using the gsd interface after all)

Done.

> ::: js/ui/status/system.js
> @@ +28,3 @@
>  
> +const BUS_NAME = 'net.hadess.SensorProxy';
> +const OBJECT_PATH = '/net/hadess/SensorProxy';
> 
> Some less generic names please

Done.

> @@ +161,1 @@
>          this._orientationExists = false;
> 
> No longer used

Removed.

> @@ +161,2 @@
>          this._orientationExists = false;
>          Gio.DBus.session.watch_name('org.gnome.SettingsDaemon.Orientation',
> 
> So we're still watching the gsd name, presumably because we still want to be
> bound to the plugin's 'active' setting - any other reasons?

No, we're supposed to be watching the sensor proxy, which will exit when no sensors are available.

> @@ +178,3 @@
> +                                                 if (error) {
> +                                                     log(error.message);
> +                                                 return;
> 
> Indentation gone awry

Fixed

> @@ +181,3 @@
> +                                             }
> +                                            
> this._proxy.connect('g-properties-changed',
> +                                                                
> Lang.bind(this, this._sync));
> 
> This method doesn't exist, I assume you mean _updateOrientationLock()?

Yep, fixed.

(In reply to Florian Müllner from comment #6)
> Review of attachment 303734 [details] [review] [review]:
> 
> ::: js/ui/status/system.js
> @@ +184,3 @@
> +                                             this._updateOrientationLock();
> +                                             }));
> +    }
> 
> There's also a missing comma here, resulting in a syntax error - are you
> sure you did attach the correct patch version?

You're asking if I tested the patch? No, I did not.
Comment 8 Bastien Nocera 2015-05-21 14:17:17 UTC
Created attachment 303762 [details] [review]
system: Use iio-sensor-proxy to detect accelerometer

Instead of using gnome-settings-daemon's D-Bus interface's presence.
iio-sensor-proxy now offers a D-Bus interface, which will exported
"HasAccelerometer = true" when an accelerometer is present.
Comment 9 Florian Müllner 2015-05-21 14:22:49 UTC
Review of attachment 303762 [details] [review]:

::: js/ui/status/system.js
@@ +159,3 @@
         this._orientationSettings.connect('changed::orientation-lock',
                                           Lang.bind(this, this._updateOrientationLock));
+        Gio.DBus.session.watch_name('net.hadess.SensorProxy',

You are watching the session bus (also: use SENSOR_BUS_NAME) ...

@@ +173,3 @@
 
+    _sensorProxyAppeared: function() {
+        this._sensorProxy = new SensorProxy(Gio.DBus.system, SENSOR_BUS_NAME, SENSOR_OBJECT_PATH,

... to make a connection on the system bus - which one is it?
Comment 10 Bastien Nocera 2015-05-21 14:25:28 UTC
Created attachment 303763 [details] [review]
system: Use iio-sensor-proxy to detect accelerometer

Instead of using gnome-settings-daemon's D-Bus interface's presence.
iio-sensor-proxy now offers a D-Bus interface, which will exported
"HasAccelerometer = true" when an accelerometer is present.
Comment 11 Florian Müllner 2015-05-21 16:49:52 UTC
Review of attachment 303763 [details] [review]:

Runs with the change outlined below; still untested though, as iio-sensor-proxy for some reason fails to acquire the bus name ...

::: js/ui/status/system.js
@@ +161,3 @@
+        Gio.DBus.system.watch_name(SENSOR_BUS_NAME,
+                                   Gio.BusNameWatcherFlags.NONE,
+                                   this._sensorProxyAppeared;

Another syntax error: comma instead of semicolon
Comment 12 Bastien Nocera 2015-05-22 13:32:56 UTC
Created attachment 303821 [details] [review]
system: Use iio-sensor-proxy to detect accelerometer

Instead of using gnome-settings-daemon's D-Bus interface's presence.
iio-sensor-proxy now offers a D-Bus interface, which will exported
"HasAccelerometer = true" when an accelerometer is present.
Comment 13 Florian Müllner 2015-05-27 08:43:51 UTC
Created attachment 304054 [details] [review]
system: Use iio-sensor-proxy to detect accelerometer

I finally managed to properly test the patch, this version should fix the remaining errors that popped up.
Comment 14 Florian Müllner 2015-05-27 08:47:38 UTC
Attachment 304054 [details] pushed as 2e77f6b - system: Use iio-sensor-proxy to detect accelerometer