Merge lp:~donadigo/switchboard-plug-power/desktop-machines-fixes into lp:~elementary-apps/switchboard-plug-power/trunk

[Adam Bieńkowski]

Fix bug #1556607: "Power plug crashes on ALL desktop machines".
* Rename "return_value" to the actual name of returned variables.
* Code style fixes.
* Remove duplicated variables and replace them with constants.
* Remove unneded variables.
* Grammar fixes.
* Check if DBus interfaces are null to prevent crashes.

Fixes bug #1556607: "Power plug crashes on ALL desktop machines".

This branch makes an overall cleanup of the code, making it more stable and logical including:
* Renaming "return_value" to the actual name of returned variables.
* Code style fixes.
* Removing duplicated variables and replacing them with constants.
* Removing unneded variables.
* Gramatic fixes.
* Checking if DBus interfaces are null to prevent crashes.

I would like to have at least 2 reviews for either battery and desktop devices in order to merge this.

[lp:~donadigo/switchboard-plug-power/desktop-machines-fixes]

312. By Adam Bieńkowski on 2016-06-05

Simplify condition

313. By Adam Bieńkowski on 2016-06-05

Remove unneded line

[Danielle Foré (danrabbit)]

I can confirm that this is working as expected on my notebook. No problems here :)

review: Approve (testing, laptop)

[Cody Garver (codygarver)]

review: Approve (testing, desktop)

=== modified file 'src/Battery.vala'
--- src/Battery.vala	2016-04-30 22:14:22 +0000
+++ src/Battery.vala	2016-06-05 12:08:16 +0000
@@ -18,11 +18,7 @@
  */
 
 namespace Power {
-    public class Battery {
-
-        private const string dbus_upower_name = "org.freedesktop.UPower";
-        private const string dbus_upower_root_path = "/org/freedesktop/UPower";
-
+    public class Battery : Object {
         private Upower? upower;
         private UpowerDevice? upower_device;
 
@@ -35,38 +31,39 @@
         }
 
         public bool check_present () {
-            bool return_value = false;
+            bool present = false;
             if (laptop) {
-                if (upower.OnBattery) {
-                    return_value = true;
-                }  else if (upower_device.IsPresent) {
-                    return_value = true;
-                } 
-            }         
-            return return_value;
+                if (upower.OnBattery || upower_device.IsPresent) {
+                    present = true;
+                }
+            }  
+
+            return present;
         }
 
-        private string get_dbus_path(Upower upow) {
-            string return_value = "";
+        private string get_dbus_path (Upower upow) {
+            string path = "";
             try {
                 ObjectPath[] devs = upow.EnumerateDevices();
-                for (int i =0; i<devs.length; i++) {
-                    if (devs[i].contains("BAT0")) {
-                        return_value = devs[i].to_string();
+                for (int i = 0; i < devs.length; i++) {
+                    if (devs[i].contains ("BAT0")) {
+                        path = devs[i].to_string();
+                        break;
                     }
                 }
             } catch (Error e) {
                 critical("acpi couldn't get upower devices");
             }
-            return return_value;
+
+            return path;
         }
 
         private void connect_dbus () {
             try {
-                upower = Bus.get_proxy_sync (BusType.SYSTEM, dbus_upower_name, dbus_upower_root_path, DBusProxyFlags.NONE);
+                upower = Bus.get_proxy_sync (BusType.SYSTEM, DBUS_UPOWER_NAME, DBUS_UPOWER_PATH, DBusProxyFlags.NONE);
                 dbus_upower_battery_path = get_dbus_path(upower);
                 if (dbus_upower_battery_path != "" && dbus_upower_battery_path != null) {
-                    upower_device = Bus.get_proxy_sync (BusType.SYSTEM, dbus_upower_name, dbus_upower_battery_path, DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
+                    upower_device = Bus.get_proxy_sync (BusType.SYSTEM, DBUS_UPOWER_NAME, dbus_upower_battery_path, DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
 
                     laptop = true;
                     debug ("battery path:%s , its a laptop, dbus connected", dbus_upower_battery_path);

=== modified file 'src/CliCommunicator.vala'
--- src/CliCommunicator.vala	2016-04-30 17:36:20 +0000
+++ src/CliCommunicator.vala	2016-06-05 12:08:16 +0000
@@ -40,51 +40,53 @@
         }
 
         private string action_to_string (Action value) {
-            string return_val = "NOT_SUPPORTED";
+            string str = "NOT_SUPPORTED";
             switch (value) {
                 case Action.IGNORE:
-                    return_val = "ignore";
+                    str = "ignore";
                     break;
                 case Action.POWEROFF:
-                    return_val = "poweroff";
+                    str = "poweroff";
                     break;
                 case Action.LOCK:
-                    return_val = "lock";
+                    str = "lock";
                     break;
                 case Action.SUSPEND:
-                    return_val = "suspend";
+                    str = "suspend";
                     break;
                 case Action.HALT:
-                    return_val = "halt";
+                    str = "halt";
                     break;
                 default:
                     break;
             }
-            return return_val;
+
+            return str;
         }
 
         private Action string_to_action (string value) {
-            Action return_val = Action.NOT_SUPPORTED;
+            Action action = Action.NOT_SUPPORTED;
             switch (value) {
                 case "ignore":
-                    return_val = Action.IGNORE;
+                    action = Action.IGNORE;
                     break;
                 case "poweroff":
-                    return_val = Action.POWEROFF;
+                    action = Action.POWEROFF;
                     break;
                 case "lock":
-                    return_val = Action.LOCK;
+                    action = Action.LOCK;
                     break;
                 case "suspend":
-                    return_val = Action.SUSPEND;
+                    action = Action.SUSPEND;
                     break;
                 case "halt":
-                    return_val = Action.HALT;
+                    action = Action.HALT;
                     break;
                 default:
                     break;
             }
-            return return_val;
+            
+            return action;
         }
 
         private void seperate_string (string value) {

=== modified file 'src/Interfaces.vala'
--- src/Interfaces.vala	2016-05-24 17:21:48 +0000
+++ src/Interfaces.vala	2016-06-05 12:08:16 +0000
@@ -18,6 +18,8 @@
  */
 
 namespace Power {
+    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")]
     interface PowerSettings : GLib.Object {

=== modified file 'src/Plug.vala'
--- src/Plug.vala	2016-05-01 00:04:56 +0000
+++ src/Plug.vala	2016-06-05 12:08:16 +0000
@@ -24,7 +24,6 @@
     Gtk.Grid main_grid;
 
     public class Plug : Switchboard.Plug {
-
         private Gtk.SizeGroup label_size;
         private Gtk.StackSwitcher stack_switcher;
         private GLib.Settings pantheon_dpms_settings;
@@ -36,7 +35,8 @@
         private Gtk.Image lock_image;
         private Gtk.Image lock_image2;
 
-        private const string no_permission_string   = _("You do not have permission to change this");
+        private const string NO_PERMISSION_STRING  = _("You do not have permission to change this");
+        private const string LAPTOP_DETECT_BINARY = "/usr/sbin/laptop-detect";
 
         public Plug () {
             Object (category: Category.HARDWARE,
@@ -62,10 +62,10 @@
         }
 
         public override void shown () {
-            if (power_supply.check_present ()) {
+            if (battery.check_present ()) {
+                stack_switcher.get_stack ().visible_child_name = "battery";
+            } else {
                 stack_switcher.get_stack ().visible_child_name = "ac";
-            } else {
-                stack_switcher.get_stack ().visible_child_name = "battery";
             }
         }
 
@@ -99,13 +99,16 @@
 
             Gtk.Stack stack = new Gtk.Stack ();
 
-            Gtk.Grid plug_grid = create_notebook_pages ("ac");
+            Gtk.Grid plug_grid = create_notebook_pages (true);
             stack.add_titled (plug_grid, "ac", _("Plugged In"));
 
             stack_container.add (info_bars);
 
+            stack_switcher = new Gtk.StackSwitcher ();
+            stack_switcher.stack = stack;
+
             if (laptop_detect () || battery.laptop) {
-                Gtk.Grid battery_grid = create_notebook_pages ("battery");
+                Gtk.Grid battery_grid = create_notebook_pages (false);
                 stack.add_titled (battery_grid, "battery", _("On Battery"));
 
                 var left_sep = new Gtk.Separator (Gtk.Orientation.HORIZONTAL);
@@ -114,9 +117,6 @@
                 var right_sep = new Gtk.Separator (Gtk.Orientation.HORIZONTAL);
                 right_sep.hexpand = true;
 
-                stack_switcher = new Gtk.StackSwitcher ();
-                stack_switcher.stack = stack;
-
                 var switcher_grid = new Gtk.Grid ();
                 switcher_grid.margin_top = 24;
                 switcher_grid.margin_bottom = 12;
@@ -132,7 +132,8 @@
 
             stack_container.margin_bottom = 12;
             stack_container.show_all ();
-            // hide stack switcher we only have ac line
+
+            // hide stack switcher if we only have ac line
             stack_switcher.visible = stack.get_children ().length () > 1;
         }
 
@@ -197,10 +198,10 @@
 
         private Gtk.Grid create_common_settings () {
             lock_image = new Gtk.Image.from_icon_name ("changes-prevent-symbolic", Gtk.IconSize.BUTTON);
-            lock_image.tooltip_text = no_permission_string;
+            lock_image.tooltip_text = NO_PERMISSION_STRING;
             lock_image.sensitive = false;
             lock_image2 = new Gtk.Image.from_icon_name ("changes-prevent-symbolic", Gtk.IconSize.BUTTON);
-            lock_image2.tooltip_text = no_permission_string;
+            lock_image2.tooltip_text = NO_PERMISSION_STRING;
             lock_image2.sensitive = false;
 
             if (laptop_detect () || battery.laptop) {
@@ -258,7 +259,7 @@
             return main_grid;
         }
 
-        private Gtk.Grid create_notebook_pages (string type) {
+        private Gtk.Grid create_notebook_pages (bool ac) {
             var grid = new Gtk.Grid ();
             grid.column_spacing = 12;
             grid.row_spacing = 12;
@@ -267,6 +268,11 @@
             ((Gtk.Misc) sleep_timeout_label).xalign = 1.0f;
             label_size.add_widget (sleep_timeout_label);
 
+            string type = "battery";
+            if (ac) {
+                type = "ac";
+            }
+
             var scale_settings = @"sleep-inactive-$type-timeout";
             var sleep_timeout = new TimeoutComboBox (settings, scale_settings);
 
@@ -276,7 +282,7 @@
             var lid_dock_box = new LidCloseActionComboBox (_("When docked and lid is closed:"), cli_communicator);
             var lid_closed_box = new LidCloseActionComboBox (_("When lid is closed:"), cli_communicator);
 
-            if (type != "ac") {
+            if (!ac) {
                 var dim_label = new Gtk.Label (_("Dim display when inactive:"));
                 ((Gtk.Misc) dim_label).xalign = 1.0f;
                 label_size.add_widget (dim_label);
@@ -332,35 +338,30 @@
         }
 
         private bool laptop_detect () {
-            string test_laptop_detect = Environment.find_program_in_path ("laptop-detect");
-            if (test_laptop_detect == null && 
-                FileUtils.test ("/usr/sbin/laptop-detect", FileTest.EXISTS) &&
-                FileUtils.test ("/usr/sbin/laptop-detect", FileTest.IS_REGULAR) &&
-                FileUtils.test ("/usr/sbin/laptop-detect", FileTest.IS_EXECUTABLE)) {
-                test_laptop_detect = "/usr/sbin/laptop-detect";
+            string? laptop_detect_executable = Environment.find_program_in_path ("laptop-detect");
+            if (laptop_detect_executable == null || 
+                !FileUtils.test (laptop_detect_executable, FileTest.IS_EXECUTABLE) ||
+                !FileUtils.test (LAPTOP_DETECT_BINARY, FileTest.IS_EXECUTABLE)) {
+                warning ("Laptop detect not found");
+                return false;
             }
 
-            if (test_laptop_detect != null) {
-                int exit_status;
-                string standard_output, standard_error;
-                try {
-                    Process.spawn_command_line_sync (test_laptop_detect, out standard_output,
-                        out standard_error, out exit_status);
-                    if (exit_status == 0) {
-                        debug ("Laptop detect return true");
-                        return true;
-                    } else {
-                        debug ("Laptop detect return false");
-                        return false;
-                    }
-                } catch (SpawnError err) {
-                    warning (err.message);
-                    return false;
+            int exit_status;
+            string standard_output, standard_error;
+            try {
+                Process.spawn_command_line_sync (laptop_detect_executable, out standard_output,
+                    out standard_error, out exit_status);
+                if (exit_status == 0) {
+                    debug ("Laptop detect returned true");
+                    return true;
                 }
-            } else {
-                warning ("Laptop detect not find");
+            } catch (SpawnError err) {
+                warning (err.message);
                 return false;
             }
+
+            debug ("Laptop detect returned false");
+            return false;
         }
 
         private void run_dpms_helper () {

=== modified file 'src/PowerSupply.vala'
--- src/PowerSupply.vala	2016-05-24 17:21:48 +0000
+++ src/PowerSupply.vala	2016-06-05 12:08:16 +0000
@@ -21,9 +21,6 @@
     public class PowerSupply {
         private Upower? upower;
         private UpowerDevice? upower_device;
-
-        private const string dbus_upower_name = "org.freedesktop.UPower";
-        private const string dbus_upower_root_path = "/org/freedesktop/UPower";
         private const uint LINE_POWER_TYPE = 1;
         private string dbus_upower_ac_path;
         
@@ -31,37 +28,41 @@
             connect_dbus ();
         }
 
-
         public bool check_present () {
-            bool return_value = false;
+            bool present = false;
+            if (upower_device == null) {
+                return false;
+            }
 
             try {
                 upower_device.Refresh ();
 
                 if (upower_device.Online && upower_device.PowerSupply) {
-                    return_value = true;
+                    present = true;
                 }
             } catch (Error e) {
-                warning ("power supply:%s", e.message);
+                warning ("power supply: %s", e.message);
             }
-            return return_value;
+
+            return present;
         }
 
         private void connect_dbus () {
-            try{
-                upower = Bus.get_proxy_sync (BusType.SYSTEM, dbus_upower_name, dbus_upower_root_path, DBusProxyFlags.NONE);
+            try {
+                upower = Bus.get_proxy_sync (BusType.SYSTEM, DBUS_UPOWER_NAME, DBUS_UPOWER_PATH, DBusProxyFlags.NONE);
                 get_upower_ac_device (upower);
             } catch (Error e) {
                 critical ("power supply dbus connection to upower fault");
             }
-            debug ("power supply path:%s  dbus connected", dbus_upower_ac_path);
+
+            debug ("power supply path: %s dbus connected", dbus_upower_ac_path);
         }
 
         private void get_upower_ac_device (Upower upow) {
             try {
                 ObjectPath[] devs = upow.EnumerateDevices ();
                 for (int i = 0; i < devs.length; i++) {
-                    UpowerDevice dev = Bus.get_proxy_sync (BusType.SYSTEM, dbus_upower_name, devs[i], DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
+                    UpowerDevice dev = Bus.get_proxy_sync (BusType.SYSTEM, DBUS_UPOWER_NAME, devs[i], DBusProxyFlags.GET_INVALIDATED_PROPERTIES);
                     if (dev.Type == LINE_POWER_TYPE) {
                         upower_device = dev;
                         return;