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

4 files changed, 267 insertions, 83 deletions

diff --git a/src/device.c b/src/device.c
index ed3c399..e3b655a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -595,10 +595,12 @@ get_expanded_time_remaining (const IndicatorPowerDevice * device)
 
       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 @@ get_accessible_time_remaining (const IndicatorPowerDevice * device)
       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 @@ get_menuitem_text (const IndicatorPowerDevice * device,
 
   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 @@ get_menuitem_text (const IndicatorPowerDevice * device,
         }
 
       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 @@ indicator_power_device_get_readable_title (const IndicatorPowerDevice * device,
 
   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
diff --git a/src/service.c b/src/service.c
index d3e143e..b01128e 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1175,6 +1175,129 @@ indicator_power_service_set_device_provider (IndicatorPowerService * self,
     }
 }
 
+/* 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)
 {
@@ -1182,13 +1305,10 @@ indicator_power_service_choose_primary_device (GList * devices)
 
   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;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 47b3e84..483bb8c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -25,6 +25,13 @@ AM_CXXFLAGS = $(GTEST_CXXFLAGS)
 ###  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)
 
diff --git a/tests/test-device.cc b/tests/test-device.cc
index 4d4a89f..96bea80 100644
--- a/tests/test-device.cc
+++ b/tests/test-device.cc
@@ -19,6 +19,7 @@ with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 #include <gio/gio.h>
 #include <gtest/gtest.h>
+#include <cmath> // ceil()
 #include "device.h"
 #include "service.h"
 
@@ -668,77 +669,106 @@ TEST_F(DeviceTest, Inestimable___this_takes_80_seconds)
   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;
-  } 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 }
   };
 
-  device_list = NULL;
-  device_list = g_list_append (device_list, a);
-  device_list = g_list_append (device_list, b);
+  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
 
-  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));
-        }
-    }
+    { "/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
 
-  // cleanup
-  g_list_free_full (device_list, g_object_unref);
+    { "/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[] = {
+
+    { { 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] }
+  };
+  
+  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);
 }