GNOME Bugzilla – Bug 749671
system: Use iio-sensor-proxy directly
Last modified: 2015-05-27 08:47:44 UTC
.
Created attachment 303733 [details] [review] system: Use iio-sensor-proxy directly Instead of using gnome-settings-daemon's D-Bus interface.
Created attachment 303734 [details] [review] system: Use iio-sensor-proxy directly Instead of using gnome-settings-daemon's D-Bus interface.
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
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()?
*** Bug 745682 has been marked as a duplicate of this bug. ***
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?
(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.
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.
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?
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.
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
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.
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.
Attachment 304054 [details] pushed as 2e77f6b - system: Use iio-sensor-proxy to detect accelerometer