Merge lp:~charlesk/indicator-power/lp-1075192 into lp:indicator-power/13.04

[Charles Kerr]

Handle choosing a primary device when no device is charging nor discharging.

Handle choosing a primary device when no device is charging nor discharging.

Full description at https://bugs.launchpad.net/ubuntu/+source/indicator-power/+bug/1075192/comments/4

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:156
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/13/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-ci/./build=pbuilder,distribution=quantal,flavor=amd64/13/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-ci/./build=pbuilder,distribution=quantal,flavor=i386/13/console

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

While this looks like a worthwhile fix in its own right, I'm not sure how this fixes bug 1075192.

(a) according to your explanation in comment 4 on the bug, I should experience the same problem -- but I am not able to reproduce this at all

(b) a label should *never* be displayed, no matter how the devices are sorted

(c) with this fix, the label would still appear when there's only line power available (although we don't show the indicator in that case)

review: Needs Information

[Charles Kerr (charlesk)]

> I'm not sure how this fixes bug 1075192.

Well, there are two parts to this bug -- the general case of the sorting, and the specific case of the AC Adapter's label. This patch fixes the first part, and since this should prevent an AC Adapter from ever being chosen as a primary device, the second part won't ever arise.

> (a) according to your explanation in comment 4 on the bug, I should experience the same problem -- but I am not able to reproduce this at all

Running i-power 12.10.4 on a laptop with a fully-charged battery should trigger the behavior. You can also mockup an equivalent configuration by hardwiring it in indicator_power_set_devices():

   + /* for testing only -- forcing a specific configuration */
   + devices = NULL;
   + devices = g_slist_append (devices, indicator_power_device_new (
   + "/org/freedesktop/UPower/devices/line_power_AC",
   + UP_DEVICE_KIND_LINE_POWER, 0, UP_DEVICE_STATE_UNKNOWN, 0));
   + devices = g_slist_append (devices, indicator_power_device_new (
   + "/org/freedesktop/UPower/devices/battery_BAT0",
   + UP_DEVICE_KIND_BATTERY, 100, UP_DEVICE_STATE_FULLY_CHARGED, 0));

> (b) a label should *never* be displayed, no matter how the devices are sorted

The AC Adapter is the only device where this happens, and with proper sorting we'll never choose that device as a primary.

Still, it's a fair point that the code should be correct.

I've added unit tests for this case in r157 and the fix in r158.

> (c) with this fix, the label would still appear when there's only line power available (although we don't show the indicator in that case)

This seems to be the same as (b)?

[lp:~charlesk/indicator-power/lp-1075192]

157. By Charles Kerr on 2012-11-07

modify DeviceTest.Labels s.t. it fails (currently failing) if the 'short' string for an AC Adapter is nonempty.

158. By Charles Kerr on 2012-11-07

change indicator_power_device_get_time_details() s.t. the 'short details' for an AC Adapter is an empty string.

[Lars Karlitski (larsu)]

> > I'm not sure how this fixes bug 1075192.
>
> Well, there are two parts to this bug -- the general case of the sorting, and
> the specific case of the AC Adapter's label. This patch fixes the first part,
> and since this should prevent an AC Adapter from ever being chosen as a
> primary device, the second part won't ever arise.

Got it. Makes me a little uneasy, though ;)

> > (a) according to your explanation in comment 4 on the bug, I should
> experience the same problem -- but I am not able to reproduce this at all
>
> Running i-power 12.10.4 on a laptop with a fully-charged battery should
> trigger the behavior. You can also mockup an equivalent configuration by
> hardwiring it in indicator_power_set_devices():
> [...]

... and it does. I wasn't running 12.10.4 before - sorry about that.

> > (b) a label should *never* be displayed, no matter how the devices are
> sorted
>
> The AC Adapter is the only device where this happens, and with proper sorting
> we'll never choose that device as a primary.
>
> Still, it's a fair point that the code should be correct.
>
> I've added unit tests for this case in r157 and the fix in r158.

Thanks! Makes me feel much less uneasy :)

> > (c) with this fix, the label would still appear when there's only line power
> available (although we don't show the indicator in that case)
>
> This seems to be the same as (b)?

The fix is the same, yes. I was just describing a different scenario when this might have turned up.

Thanks for the fixes!

review: Approve

=== modified file 'src/device.c'
--- src/device.c	2012-10-26 08:53:51 +0000
+++ src/device.c	2012-11-07 17:37:25 +0000
@@ -612,7 +612,7 @@
     {
       *details         = g_strdup (device_name);
       *accessible_name = g_strdup (device_name);
-      *short_details   = g_strdup (device_name);
+      *short_details   = g_strdup ("");
     }
   else
     {

=== modified file 'src/indicator-power.c'
--- src/indicator-power.c	2012-10-29 10:59:09 +0000
+++ src/indicator-power.c	2012-11-07 17:37:25 +0000
@@ -352,12 +352,38 @@
   gtk_widget_show_all (GTK_WIDGET (priv->menu));
 }
 
+/* the higher the weight, the more interesting the device */
+static int
+get_device_kind_weight (const IndicatorPowerDevice * device)
+{
+  UpDeviceKind kind;
+  static gboolean initialized = FALSE;
+  static int weights[UP_DEVICE_KIND_LAST];
+
+  kind = indicator_power_device_get_kind (device);
+  g_return_val_if_fail (0<=kind && kind<UP_DEVICE_KIND_LAST, 0);
+
+  if (G_UNLIKELY(!initialized))
+    {
+      int i;
+
+      initialized = TRUE;
+
+      for (i=0; i<UP_DEVICE_KIND_LAST; i++)
+        weights[i] = 1;
+      weights[UP_DEVICE_KIND_BATTERY] = 2;
+      weights[UP_DEVICE_KIND_LINE_POWER] = 0;
+    }
+
+  return weights[kind];
+}
+
 /* sort devices from most interesting to least interesting on this criteria:
    1. discharging items from least time remaining until most time remaining
    2. discharging items with an unknown time remaining
    3. charging items from most time left to charge to least time left to charge
    4. charging items with an unknown time remaining
-   5. everything else */
+   5. batteries, then non-line power, then line-power */
 static gint
 device_compare_func (gconstpointer ga, gconstpointer gb)
 {
@@ -418,6 +444,21 @@
         }
     }
 
+  if (!ret) /* neither device is charging nor discharging... */
+    {
+      const int weight_a = get_device_kind_weight (a);
+      const int weight_b = get_device_kind_weight (b);
+
+      if (weight_a > weight_b)
+        {
+          ret = -1;
+        }
+      else if (weight_a < weight_b)
+        {
+          ret = 1;
+        }
+    }
+
   if (!ret)
     ret = a_state - b_state;
 

=== modified file 'tests/test-device.cc'
--- tests/test-device.cc	2012-10-28 07:40:44 +0000
+++ tests/test-device.cc	2012-11-07 17:37:25 +0000
@@ -555,7 +555,7 @@
                    INDICATOR_POWER_DEVICE_PERCENTAGE, 0.0,
                    INDICATOR_POWER_DEVICE_TIME, guint64(0),
                    NULL);
-  check_strings (device, "AC Adapter", "AC Adapter", "AC Adapter");
+  check_strings (device, "", "AC Adapter", "AC Adapter");
 
   // cleanup
   g_object_unref(o);
@@ -587,19 +587,22 @@
      sorted in order of preference wrt the spec's criteria.
      So tests[i] should be picked over any test with an index greater than i */
   struct {
+    int kind;
     int state;
     guint64 time;
     double percentage;
   } tests[] = {
-    { UP_DEVICE_STATE_DISCHARGING, 49, 50.0 },
-    { UP_DEVICE_STATE_DISCHARGING, 50, 50.0 },
-    { UP_DEVICE_STATE_DISCHARGING, 50, 100.0 },
-    { UP_DEVICE_STATE_DISCHARGING, 51, 50.0 },
-    { UP_DEVICE_STATE_CHARGING, 50, 50.0 },
-    { UP_DEVICE_STATE_CHARGING, 49, 50.0 },
-    { UP_DEVICE_STATE_CHARGING, 49, 100.0 },
-    { UP_DEVICE_STATE_CHARGING, 48, 50.0 },
-    { UP_DEVICE_STATE_FULLY_CHARGED, 0, 50.0 }
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   49,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   50,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   50, 100.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   51,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      50,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      49,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      49, 100.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      48,  50.0 },
+    { UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_FULLY_CHARGED,  0,  50.0 },
+    { UP_DEVICE_KIND_KEYBOARD,   UP_DEVICE_STATE_FULLY_CHARGED,  0,  50.0 },
+    { UP_DEVICE_KIND_LINE_POWER, UP_DEVICE_STATE_UNKNOWN,        0,   0.0 }
   };
 
   device_list = NULL;
@@ -610,11 +613,13 @@
     {
       for (int j=i+1; j<n; j++)
         {
-          g_object_set (a, INDICATOR_POWER_DEVICE_STATE, tests[i].state,
+          g_object_set (a, INDICATOR_POWER_DEVICE_KIND, tests[i].kind,
+                           INDICATOR_POWER_DEVICE_STATE, tests[i].state,
                            INDICATOR_POWER_DEVICE_TIME, guint64(tests[i].time),
                            INDICATOR_POWER_DEVICE_PERCENTAGE, tests[i].percentage,
                            NULL);
-          g_object_set (b, INDICATOR_POWER_DEVICE_STATE, tests[j].state,
+          g_object_set (b, INDICATOR_POWER_DEVICE_KIND, tests[j].kind,
+                           INDICATOR_POWER_DEVICE_STATE, tests[j].state,
                            INDICATOR_POWER_DEVICE_TIME, guint64(tests[j].time),
                            INDICATOR_POWER_DEVICE_PERCENTAGE, tests[j].percentage,
                            NULL);