Merge lp:~charlesk/indicator-power/lp-880881 into lp:indicator-power/14.04

[Charles Kerr]

If there are two batteries detected, combine their percentages and their time-remainings as per the revised spec.

If there are two batteries detected, combine their percentages and their time-remainings as per the revised spec.

https://wiki.ubuntu.com/Power#Handling_multiple_batteries

If a device has multiple batteries and uses only one of them at a time, they should be presented as separate items inside the battery menu, but everywhere else they should be aggregated (bug 880881). Their percentages should be averaged. If any are discharging, the aggregated time remaining should be the maximum of the times for all those that are discharging, plus the sum of the times for all those that are idle. Otherwise, the aggregated time remaining should be the the maximum of the times for all those that are charging.

For example, if your notebook has two internal batteries — one fully charged, and one that will discharge in 22 minutes — and your wireless mouse battery is estimated to discharge in 27 minutes, then the menu title should represent the mouse. Even though the internal battery will lose power first, the mouse is the device that will lose power first.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:232
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/96/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-amd64-ci/33
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/33
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-trusty-armhf-ci/33/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/96/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

Wish I had a machine to test this for real on :-)

review: Approve

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

233. By Charles Kerr on 2014-03-25

add translator comments to the new translatable strings.

[Charles Kerr (charlesk)]

Erp, looks like that didn't merge cleanly.

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

234. By Charles Kerr on 2014-03-25

sync to lp:indicator-power

=== modified file 'src/device.c'
--- src/device.c	2014-03-13 15:31:33 +0000
+++ src/device.c	2014-03-25 19:49:27 +0000
@@ -595,10 +595,12 @@
 
       if (p->state == UP_DEVICE_STATE_CHARGING)
         {
+          /* TRANSLATORS: H:MM (hours, minutes) to charge the battery. Example: "1:30 to charge" */
           str = g_strdup_printf (_("%0d:%02d to charge"), hours, minutes);
         }
       else // discharging
         {
+          /* TRANSLATORS: H:MM (hours, minutes) to discharge the battery. Example: "1:30 left"*/
           str = g_strdup_printf (_("%0d:%02d left"), hours, minutes);
         }
     }
@@ -631,22 +633,38 @@
       if (p->state == UP_DEVICE_STATE_CHARGING)
         {
           if (hours > 0)
-            str = g_strdup_printf (_("%d %s %d %s to charge"),
-                        hours, g_dngettext (NULL, "hour", "hours", hours),
-                        minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+            {
+              /* TRANSLATORS: "X (hour,hours) Y (minute,minutes) to charge" the battery.
+                 Example: "1 hour 10 minutes to charge" */
+              str = g_strdup_printf (_("%d %s %d %s to charge"),
+                          hours, g_dngettext (NULL, "hour", "hours", hours),
+                          minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+           }
          else
-            str = g_strdup_printf (_("%d %s to charge"),
-                        minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+           {
+              /* TRANSLATORS: "Y (minute,minutes) to charge" the battery.
+                 Example: "59 minutes to charge" */
+              str = g_strdup_printf (_("%d %s to charge"),
+                          minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+           }
         }
       else // discharging
         {
           if (hours > 0)
-            str = g_strdup_printf (_("%d %s %d %s left"),
-                        hours, g_dngettext (NULL, "hour", "hours", hours),
-                        minutes, g_dngettext (NULL, "minute", "minutes", minutes));
-         else
-            str = g_strdup_printf (_("%d %s left"),
-                        minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+            {
+              /* TRANSLATORS: "X (hour,hours) Y (minute,minutes) left" until the battery's empty.
+                 Example: "1 hour 10 minutes left" */
+              str = g_strdup_printf (_("%d %s %d %s left"),
+                          hours, g_dngettext (NULL, "hour", "hours", hours),
+                          minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+            }
+          else
+            {
+              /* TRANSLATORS: "Y (minute,minutes) left" until the battery's empty.
+                 Example: "59 minutes left" */
+              str = g_strdup_printf (_("%d %s left"),
+                          minutes, g_dngettext (NULL, "minute", "minutes", minutes));
+            }
         }
     }
   else
@@ -700,6 +718,7 @@
 
   if (p->state == UP_DEVICE_STATE_FULLY_CHARGED)
     {
+      /* TRANSLATORS: example: "battery (charged)" */
       str = g_strdup_printf (_("%s (charged)"), kind_str);
     }
   else
@@ -715,9 +734,14 @@
         }
 
       if (time_str && *time_str)
-        str = g_strdup_printf (_("%s (%s)"), kind_str, time_str);
+        {
+          /* TRANSLATORS: example: "battery (time remaining)" */
+          str = g_strdup_printf (_("%s (%s)"), kind_str, time_str);
+        }
       else
-        str = g_strdup (kind_str);
+        {
+          str = g_strdup (kind_str);
+        }
 
       g_free (time_str);
     }
@@ -783,14 +807,17 @@
 
   if (want_time && want_percent)
     {
+      /* TRANSLATORS: after the icon, a time-remaining string + battery %. Example: "(0:59, 33%)" */
       str = g_strdup_printf (_("(%s, %.0lf%%)"), time_str, p->percentage);
     }
   else if (want_time)
     {
+      /* TRANSLATORS: after the icon, a time-remaining string Example: "(0:59)" */
       str = g_strdup_printf (_("(%s)"), time_str);
     }
   else if (want_percent)
     {
+      /* TRANSLATORS: after the icon, a battery %. Example: "(33%)" */
       str = g_strdup_printf (_("(%.0lf%%)"), p->percentage);
     }
   else
 
=== modified file 'src/service.c'
--- src/service.c	2014-03-13 22:27:29 +0000
+++ src/service.c	2014-03-25 19:49:27 +0000
@@ -1135,6 +1135,129 @@
     }
 }
 
+/* If a device has multiple batteries and uses only one of them at a time,
+   they should be presented as separate items inside the battery menu,
+   but everywhere else they should be aggregated (bug 880881).
+   Their percentages should be averaged. If any are discharging,
+   the aggregated time remaining should be the maximum of the times
+   for all those that are discharging, plus the sum of the times
+   for all those that are idle. Otherwise, the aggregated time remaining
+   should be the the maximum of the times for all those that are charging. */
+static IndicatorPowerDevice *
+create_totalled_battery_device (const GList * devices)
+{
+  const GList * l;
+  guint n_charged = 0;
+  guint n_charging = 0;
+  guint n_discharging = 0;
+  guint n_batteries = 0;
+  double sum_percent = 0;
+  time_t max_discharge_time = 0;
+  time_t max_charge_time = 0;
+  time_t sum_charged_time = 0;
+  IndicatorPowerDevice * device = NULL;
+
+  for (l=devices; l!=NULL; l=l->next)
+    {
+      const IndicatorPowerDevice * device = INDICATOR_POWER_DEVICE(l->data);
+
+      if (indicator_power_device_get_kind(device) == UP_DEVICE_KIND_BATTERY)
+        {
+          const double percent = indicator_power_device_get_percentage (device);
+          const time_t t = indicator_power_device_get_time (device);
+          const UpDeviceState state = indicator_power_device_get_state (device);
+
+          ++n_batteries;
+
+          if (percent > 0.01)
+            sum_percent += percent;
+
+          if (state == UP_DEVICE_STATE_CHARGING)
+            {
+              ++n_charging;
+              max_charge_time = MAX(max_charge_time, t);
+            }
+          else if (state == UP_DEVICE_STATE_DISCHARGING)
+            {
+              ++n_discharging;
+              max_discharge_time = MAX(max_discharge_time, t);
+            }
+          else if (state == UP_DEVICE_STATE_FULLY_CHARGED)
+            {
+              ++n_charged;
+              sum_charged_time += t;
+            }
+        }
+    }
+
+  if (n_batteries > 1)
+    {
+      const double percent = sum_percent / n_batteries;
+      UpDeviceState state;
+      time_t time_left;
+
+      if (n_discharging > 0)
+        {
+          state = UP_DEVICE_STATE_DISCHARGING;
+          time_left = max_discharge_time + sum_charged_time;
+        }
+      else if (n_charging > 0)
+        {
+          state = UP_DEVICE_STATE_CHARGING;
+          time_left = max_charge_time;
+        }
+      else if (n_charged > 0)
+        {
+          state = UP_DEVICE_STATE_FULLY_CHARGED;
+          time_left = 0;
+        }
+      else
+        {
+          state = UP_DEVICE_STATE_UNKNOWN;
+          time_left = 0;
+        }
+
+      device = indicator_power_device_new (NULL,
+                                           UP_DEVICE_KIND_BATTERY,
+                                           percent,
+                                           state,
+                                           time_left);
+    }
+
+  return device;
+}
+
+/**
+ * If there are multiple UP_DEVICE_KIND_BATTERY devices in the list,
+ * they're merged into a new 'totalled' device representing the sum of them.
+ *
+ * Returns: (element-type IndicatorPowerDevice)(transfer full): a list of devices
+ */
+static GList*
+merge_batteries_together (GList * devices)
+{
+  GList * ret;
+  IndicatorPowerDevice * merged_device;
+
+  if ((merged_device = create_totalled_battery_device (devices)))
+    {
+      GList * l;
+
+      ret = g_list_append (NULL, merged_device);
+
+      for (l=devices; l!=NULL; l=l->next)
+        if (indicator_power_device_get_kind(INDICATOR_POWER_DEVICE(l->data)) != UP_DEVICE_KIND_BATTERY)
+          ret = g_list_append (ret, g_object_ref(l->data));
+    }
+  else /* not enough batteries to merge */
+    {
+      ret = g_list_copy (devices);
+      g_list_foreach (ret, (GFunc)g_object_ref, NULL);
+    }
+
+  return ret;
+}
+
 IndicatorPowerDevice *
 indicator_power_service_choose_primary_device (GList * devices)
 {
@@ -1142,13 +1265,10 @@
 
   if (devices != NULL)
     {
-      GList * tmp;
-
-      tmp = g_list_copy (devices);
+      GList * tmp = merge_batteries_together (devices);
       tmp = g_list_sort (tmp, device_compare_func);
       primary = g_object_ref (tmp->data);
-
-      g_list_free (tmp);
+      g_list_free_full (tmp, (GDestroyNotify)g_object_unref);
     }
 
   return primary;
 
=== modified file 'tests/Makefile.am'
--- tests/Makefile.am	2014-03-05 04:57:52 +0000
+++ tests/Makefile.am	2014-03-25 19:49:27 +0000
@@ -25,6 +25,13 @@
 ###  tests: indicator-power-device
 ###
 
+dear-reader-please-note-the-next-test-takes-90-seconds:
+	@echo "#!/bin/bash" > $@
+	@echo "exit 0" >> $@
+	@chmod +x $@
+TESTS += dear-reader-please-note-the-next-test-takes-90-seconds
+CLEANFILES += dear-reader-please-note-the-next-test-takes-90-seconds
+
 TEST_LIBS = $(COVERAGE_LDFLAGS) libgtest.a -lpthread
 TEST_CPPFLAGS = $(SERVICE_DEPS_CFLAGS) $(AM_CPPFLAGS)
 
 
=== modified file 'tests/test-device.cc'
--- tests/test-device.cc	2014-03-13 15:32:04 +0000
+++ tests/test-device.cc	2014-03-25 19:49:27 +0000
@@ -19,6 +19,7 @@
 
 #include <gio/gio.h>
 #include <gtest/gtest.h>
+#include <cmath> // ceil()
 #include "device.h"
 #include "service.h"
 
@@ -668,77 +669,106 @@
   g_free (real_lang);
 }
 
-
-/* The menu title should tell you at a glance what you need to know most: what
-   device will lose power soonest (and optionally when), or otherwise which
-   device will take longest to charge (and optionally how long it will take). */
+/* If a device has multiple batteries and uses only one of them at a time,
+   they should be presented as separate items inside the battery menu,
+   but everywhere else they should be aggregated (bug 880881).
+   Their percentages should be averaged. If any are discharging,
+   the aggregated time remaining should be the maximum of the times
+   for all those that are discharging, plus the sum of the times
+   for all those that are idle. Otherwise, the aggregated time remaining
+   should be the the maximum of the times for all those that are charging. */
 TEST_F(DeviceTest, ChoosePrimary)
 {
-  GList * device_list;
-  IndicatorPowerDevice * a;
-  IndicatorPowerDevice * b;
-
-  a = indicator_power_device_new ("/org/freedesktop/UPower/devices/mouse",
-                                  UP_DEVICE_KIND_MOUSE,
-                                  0.0,
-                                  UP_DEVICE_STATE_DISCHARGING,
-                                  0);
-  b = indicator_power_device_new ("/org/freedesktop/UPower/devices/battery",
-                                  UP_DEVICE_KIND_BATTERY,
-                                  0.0,
-                                  UP_DEVICE_STATE_DISCHARGING,
-                                  0);
-
-  /* device states + time left to {discharge,charge} + % of charge left,
-     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;
+  struct Description
+  { 
+    const char * path;
+    UpDeviceKind kind;
+    UpDeviceState state;
     guint64 time;
     double percentage;
+  };
+
+  const Description descriptions[] = {
+    { "/some/path/d0", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   10,  60.0 }, // 0
+    { "/some/path/d1", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   20,  80.0 }, // 1
+    { "/some/path/d2", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_DISCHARGING,   30, 100.0 }, // 2
+
+    { "/some/path/c0", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      10,  60.0 }, // 3
+    { "/some/path/c1", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      20,  80.0 }, // 4
+    { "/some/path/c2", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_CHARGING,      30, 100.0 }, // 5
+
+    { "/some/path/f0", UP_DEVICE_KIND_BATTERY,    UP_DEVICE_STATE_FULLY_CHARGED,  0, 100.0 }, // 6
+    { "/some/path/m0", UP_DEVICE_KIND_MOUSE,      UP_DEVICE_STATE_DISCHARGING,   20,  80.0 }, // 7
+    { "/some/path/m1", UP_DEVICE_KIND_MOUSE,      UP_DEVICE_STATE_FULLY_CHARGED,  0, 100.0 }, // 8
+    { "/some/path/pw", UP_DEVICE_KIND_LINE_POWER, UP_DEVICE_STATE_UNKNOWN,        0,   0.0 }  // 9
+  };
+
+  std::vector<IndicatorPowerDevice*> devices;
+  for(const auto& desc : descriptions)
+    devices.push_back(indicator_power_device_new(desc.path, desc.kind, desc.percentage, desc.state, desc.time));
+
+  const struct {
+    std::vector<int> device_indices;
+    Description expected;
   } tests[] = {
-    { 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 }
+
+    { { 0 }, descriptions[0] }, // 1 discharging
+    { { 0, 1 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 20, 70.0 } }, // 2 discharging
+    { { 1, 2 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 30, 90.0 } }, // 2 discharging
+    { { 0, 1, 2 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 30, 80.0 } }, // 3 discharging
+
+    { { 3 }, descriptions[3] }, // 1 charging
+    { { 3, 4 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 20, 70.0 } }, // 2 charging
+    { { 4, 5 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 30, 90.0 } }, // 2 charging
+    { { 3, 4, 5 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 30, 80.0 } }, // 3 charging
+
+    { { 6 }, descriptions[6] }, // 1 charged
+    { { 6, 0 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 10, 80.0 } }, // 1 charged, 1 discharging
+    { { 6, 3 },    { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING,    10, 80.0 } }, // 1 charged, 1 charging
+    { { 6, 0, 3 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 10, 73.3 } }, // 1 charged, 1 charging, 1 discharging
+
+    { { 0, 7 }, descriptions[0] }, // 1 discharging battery, 1 discharging mouse. pick the one with the least time left.
+    { { 2, 7 }, descriptions[7] }, // 1 discharging battery, 1 discharging mouse. pick the one with the least time left.
+
+    { { 0, 8 }, descriptions[0] }, // 1 discharging battery, 1 fully-charged mouse. pick the one that's discharging.
+    { { 6, 7 }, descriptions[7] }, // 1 discharging mouse, 1 fully-charged battery. pick the one that's discharging.
+
+    { { 0, 9 }, descriptions[0] }, // everything comes before power lines
+    { { 3, 9 }, descriptions[3] },
+    { { 7, 9 }, descriptions[7] }
   };
-
-  device_list = NULL;
-  device_list = g_list_append (device_list, a);
-  device_list = g_list_append (device_list, b);
-
-  for (int i=0, n=G_N_ELEMENTS(tests); i<n; i++)
-    {
-      for (int j=i+1; j<n; j++)
-        {
-          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_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);
-          ASSERT_EQ (a, indicator_power_service_choose_primary_device(device_list));
-          g_object_unref (G_OBJECT(a));
-
-          /* reverse the list to check that list order doesn't matter */
-          device_list = g_list_reverse (device_list);
-          ASSERT_EQ (a, indicator_power_service_choose_primary_device(device_list));
-          g_object_unref (G_OBJECT(a));
-        }
-    }
-
-  // cleanup
-  g_list_free_full (device_list, g_object_unref);
+  
+  for(const auto& test : tests)
+  {
+    const auto& x = test.expected;
+
+    GList * device_glist = nullptr;
+    for(const auto& i : test.device_indices)
+      device_glist = g_list_append(device_glist, devices[i]);
+
+    auto primary = indicator_power_service_choose_primary_device(device_glist);
+    EXPECT_STREQ(x.path, indicator_power_device_get_object_path(primary));
+    EXPECT_EQ(x.kind, indicator_power_device_get_kind(primary));
+    EXPECT_EQ(x.state, indicator_power_device_get_state(primary));
+    EXPECT_EQ(x.time, indicator_power_device_get_time(primary));
+    EXPECT_EQ((int)ceil(x.percentage), (int)ceil(indicator_power_device_get_percentage(primary)));
+    g_object_unref(primary);
+
+    // reverse the list and repeat the test
+    // to confirm that list order doesn't matter
+    device_glist = g_list_reverse (device_glist);
+    primary = indicator_power_service_choose_primary_device (device_glist);
+    EXPECT_STREQ(x.path, indicator_power_device_get_object_path(primary));
+    EXPECT_EQ(x.kind, indicator_power_device_get_kind(primary));
+    EXPECT_EQ(x.state, indicator_power_device_get_state(primary));
+    EXPECT_EQ(x.time, indicator_power_device_get_time(primary));
+    EXPECT_EQ((int)ceil(x.percentage), (int)ceil(indicator_power_device_get_percentage(primary)));
+    g_object_unref(primary);
+
+    // cleanup
+    g_list_free(device_glist);
+  }
+
+  for (auto& device : devices)
+    g_object_unref (device);
 }