Merge lp:~diegoprovinciani/switchboard-plug-power/1383209 into lp:~elementary-apps/switchboard-plug-power/trunk

[Diego Provinciani]

Brightness slider update when changing brightness via keyboard shortcut

Added a mechanism to conect to signal emited when the brightness property on dbus is changed.

To test it, build and install the project, open the switchboard and then power plug. Press the brightness keyboard shortcut.
You can also try change the brightness changing the slider position with the pointer and with the direction keys on keyboard.

[Danielle Foré (danrabbit)]

Please revert all the file permissions changes

review: Needs Fixing

[lp:~diegoprovinciani/switchboard-plug-power/1383209]

326. By Diego Provinciani on 2016-07-04

Reverted all the file permissions

327. By Diego Provinciani on 2016-07-04

Reverted all the file permissions

[Diego Provinciani (diegoprovinciani)]

All file permissions were restored.

On dom, jul 3, 2016 at 10:53 , Daniel Fore <email address hidden> wrote:
> Review: Needs Fixing
>
> Please revert all the file permissions changes
> --
> https://code.launchpad.net/~diegoprovinciani/switchboard-plug-power/1383209/+merge/299001
> You are the owner of
> lp:~diegoprovinciani/switchboard-plug-power/1383209.

[Adam Bieńkowski (donadigo)]

Some minor things about the code:
- Some lines aren't correctly idented (look diff).

- code style: "string []" should be strip down to just: "string[]"

- I do not like how the UpowerProperties variable is named: "screen_signal". It suggests it's a signal, but it's not. I would suggest maybe to change it to just "upower_properties"?

- code style: we do not use upper case in methods: change "on_screen_signal_PropertiesChanged" to "on_screen_signal_properties_changed".

- Connecting the PropertiesChanged signal could be simplified: just replace it like this:
screen_signal.PropertiesChanged.connect (on_screen_signal_properties_changed);

- In method on_screen_signal_PropertiesChanged: there's no guarantee that "Brightness" property changed and it can be null, perhaps you could use this on the beggining of the method:

if (!changed_properties.contains ("Brightness")) {
   return;
}

review: Needs Fixing (code)

[lp:~diegoprovinciani/switchboard-plug-power/1383209]

328. By Diego Provinciani on 2016-07-05

Fixed all code style suggestions

329. By Diego Provinciani on 2016-07-05

Fixed all code style suggestions

[Diego Provinciani (diegoprovinciani)]

All the suggestions has been fixed and the project has been tested again, checking the correct behavior.

[Corentin Noël (tintou)]

You never need to implement org.freedesktop.DBus.Properties
See here for an example about how to handle invalidated properties the better way:
http://bazaar.launchpad.net/~wingpanel-devs/wingpanel-indicator-bluetooth/trunk/view/head:/src/Services/Manager.vala#L64

[lp:~diegoprovinciani/switchboard-plug-power/1383209]

330. By Diego Provinciani on 2016-07-07

Refactorized property signal to detect brightness changes

[Diego Provinciani (diegoprovinciani)]

The mechanism to detect the signal generated when the brightness property change has been refactored as Corentin Noël has been suggested.

=== modified file 'src/Interfaces.vala'
--- src/Interfaces.vala	2016-06-03 22:30:14 +0000
+++ src/Interfaces.vala	2016-07-07 04:28:03 +0000
@@ -21,7 +21,7 @@
     public const string DBUS_UPOWER_NAME = "org.freedesktop.UPower";
     public const string DBUS_UPOWER_PATH = "/org/freedesktop/UPower";
 
- [DBus (name = "org.gnome.SettingsDaemon.Power.Screen")]
+    [DBus (name = "org.gnome.SettingsDaemon.Power.Screen")]
     interface PowerSettings : GLib.Object {
 #if OLD_GSD
         public abstract uint GetPercentage () throws IOError;
@@ -32,28 +32,28 @@
 #endif
     }
 
-	[DBus (name = "org.freedesktop.UPower.Device")]
-	interface UpowerDevice : Object {
-		public signal void Changed ();
-		public abstract void Refresh () throws IOError;
-		public abstract bool Online { owned get; private set; }
-		public abstract bool PowerSupply { owned get; private set; }
-		public abstract bool IsPresent { owned get; private set; }
-		public abstract uint Type { owned get; private set; }
-	}
-
-
-	[DBus (name = "org.freedesktop.UPower")]
-	interface Upower : Object {
-		public signal void Changed ();
-		public abstract bool OnBattery { owned get; private set; }
-		public abstract bool LowOnBattery { owned get; private set; }
-		public abstract ObjectPath[] EnumerateDevices () throws IOError;
-	}
-
-	[DBus (name = "org.freedesktop.DBus.Properties")]
-	public interface UpowerProperties : Object {
-		public abstract Variant Get (string interface, string propname) throws IOError;
-		public abstract void Set (string interface, string propname, Variant value) throws IOError;
-	}
+    [DBus (name = "org.freedesktop.UPower.Device")]
+    interface UpowerDevice : Object {
+        public signal void Changed ();
+        public abstract void Refresh () throws IOError;
+        public abstract bool Online { owned get; private set; }
+        public abstract bool PowerSupply { owned get; private set; }
+        public abstract bool IsPresent { owned get; private set; }
+        public abstract uint Type { owned get; private set; }
+    }
+
+
+    [DBus (name = "org.freedesktop.UPower")]
+    interface Upower : Object {
+        public signal void Changed ();
+        public abstract bool OnBattery { owned get; private set; }
+        public abstract bool LowOnBattery { owned get; private set; }
+        public abstract ObjectPath[] EnumerateDevices () throws IOError;
+    }
+
+    [DBus (name = "org.freedesktop.DBus.Properties")]
+    public interface UpowerProperties : Object {
+        public abstract Variant Get (string interface, string propname) throws IOError;
+        public abstract void Set (string interface, string propname, Variant value) throws IOError;
+    }
 }

=== modified file 'src/Plug.vala'
--- src/Plug.vala	2016-06-05 12:08:06 +0000
+++ src/Plug.vala	2016-07-07 04:28:03 +0000
@@ -34,6 +34,7 @@
         private CliCommunicator cli_communicator;
         private Gtk.Image lock_image;
         private Gtk.Image lock_image2;
+        private Gtk.Scale scale;
 
         private const string NO_PERMISSION_STRING  = _("You do not have permission to change this");
         private const string LAPTOP_DETECT_BINARY = "/usr/sbin/laptop-detect";
@@ -139,8 +140,8 @@
 
         private void connect_dbus () {
             try {
-                screen = Bus.get_proxy_sync (BusType.SESSION,
-                    "org.gnome.SettingsDaemon", "/org/gnome/SettingsDaemon/Power");
+                screen = Bus.get_proxy_sync (BusType.SESSION, "org.gnome.SettingsDaemon",
+                    "/org/gnome/SettingsDaemon/Power", DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
             } catch (IOError e) {
                 warning ("Failed to get settings daemon for brightness setting");
             }
@@ -218,17 +219,15 @@
 
                 settings.bind ("ambient-enabled", als_switch, "active", SettingsBindFlags.DEFAULT);
 
-                var scale = new Gtk.Scale.with_range (Gtk.Orientation.HORIZONTAL, 0, 100, 10);
+                scale = new Gtk.Scale.with_range (Gtk.Orientation.HORIZONTAL, 0, 100, 10);
                 scale.draw_value = false;
                 scale.hexpand = true;
                 scale.width_request = 480;
 
                 scale.set_value (screen.Brightness);
 
-                scale.value_changed.connect (() => {
-                    var val = (int) scale.get_value ();
-                    screen.Brightness = val;
-                });
+                scale.value_changed.connect (on_scale_value_changed);
+                (screen as DBusProxy).g_properties_changed.connect (on_screen_properties_changed);
 
                 main_grid.attach (brightness_label, 0, 0, 1, 1);
                 main_grid.attach (scale, 1, 0, 1, 1);
@@ -258,6 +257,23 @@
 
             return main_grid;
         }
+        
+        private void on_scale_value_changed () {
+            var val = (int) scale.get_value ();
+            (screen as DBusProxy).g_properties_changed.disconnect (on_screen_properties_changed);
+            screen.Brightness = val;
+            (screen as DBusProxy).g_properties_changed.connect (on_screen_properties_changed);
+        }
+        
+        private void on_screen_properties_changed (Variant changed_properties, string[] invalidated_properties) {
+            var changed_brightness = changed_properties.lookup_value("Brightness", new VariantType("i"));
+            if (changed_brightness != null) {
+                var val = screen.Brightness;
+                scale.value_changed.disconnect (on_scale_value_changed);
+                scale.set_value (val);
+                scale.value_changed.connect (on_scale_value_changed);
+            }
+        }
 
         private Gtk.Grid create_notebook_pages (bool ac) {
             var grid = new Gtk.Grid ();