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

Proposed by Charles Kerr on 2014-03-19
Status: Merged
Approved by: Ted Gould on 2014-03-25
Approved revision: 232
Merged at revision: 237
Proposed branch: lp:~charlesk/indicator-power/lp-880881
Merge into: lp:indicator-power/14.04
Diff against target: 463 lines (+269/-85)
4 files modified
src/device.c (+40/-13)
src/service.c (+125/-5)
tests/Makefile.am (+7/-0)
tests/test-device.cc (+97/-67)
To merge this branch: bzr merge lp:~charlesk/indicator-power/lp-880881
Reviewer Review Type Date Requested Status
Ted Gould (community) 2014-03-19 Approve on 2014-03-25
PS Jenkins bot (community) continuous-integration Approve on 2014-03-19
Review via email: mp+211817@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Ted Gould (ted) wrote :

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

review: Approve
233. By Charles Kerr on 2014-03-25

add translator comments to the new translatable strings.

Charles Kerr (charlesk) wrote :

Erp, looks like that didn't merge cleanly.

234. By Charles Kerr on 2014-03-25

sync to lp:indicator-power

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/device.c'
2--- src/device.c 2014-03-13 15:31:33 +0000
3+++ src/device.c 2014-03-25 19:49:27 +0000
4@@ -595,10 +595,12 @@
5
6 if (p->state == UP_DEVICE_STATE_CHARGING)
7 {
8+ /* TRANSLATORS: H:MM (hours, minutes) to charge the battery. Example: "1:30 to charge" */
9 str = g_strdup_printf (_("%0d:%02d to charge"), hours, minutes);
10 }
11 else // discharging
12 {
13+ /* TRANSLATORS: H:MM (hours, minutes) to discharge the battery. Example: "1:30 left"*/
14 str = g_strdup_printf (_("%0d:%02d left"), hours, minutes);
15 }
16 }
17@@ -631,22 +633,38 @@
18 if (p->state == UP_DEVICE_STATE_CHARGING)
19 {
20 if (hours > 0)
21- str = g_strdup_printf (_("%d %s %d %s to charge"),
22- hours, g_dngettext (NULL, "hour", "hours", hours),
23- minutes, g_dngettext (NULL, "minute", "minutes", minutes));
24+ {
25+ /* TRANSLATORS: "X (hour,hours) Y (minute,minutes) to charge" the battery.
26+ Example: "1 hour 10 minutes to charge" */
27+ str = g_strdup_printf (_("%d %s %d %s to charge"),
28+ hours, g_dngettext (NULL, "hour", "hours", hours),
29+ minutes, g_dngettext (NULL, "minute", "minutes", minutes));
30+ }
31 else
32- str = g_strdup_printf (_("%d %s to charge"),
33- minutes, g_dngettext (NULL, "minute", "minutes", minutes));
34+ {
35+ /* TRANSLATORS: "Y (minute,minutes) to charge" the battery.
36+ Example: "59 minutes to charge" */
37+ str = g_strdup_printf (_("%d %s to charge"),
38+ minutes, g_dngettext (NULL, "minute", "minutes", minutes));
39+ }
40 }
41 else // discharging
42 {
43 if (hours > 0)
44- str = g_strdup_printf (_("%d %s %d %s left"),
45- hours, g_dngettext (NULL, "hour", "hours", hours),
46- minutes, g_dngettext (NULL, "minute", "minutes", minutes));
47- else
48- str = g_strdup_printf (_("%d %s left"),
49- minutes, g_dngettext (NULL, "minute", "minutes", minutes));
50+ {
51+ /* TRANSLATORS: "X (hour,hours) Y (minute,minutes) left" until the battery's empty.
52+ Example: "1 hour 10 minutes left" */
53+ str = g_strdup_printf (_("%d %s %d %s left"),
54+ hours, g_dngettext (NULL, "hour", "hours", hours),
55+ minutes, g_dngettext (NULL, "minute", "minutes", minutes));
56+ }
57+ else
58+ {
59+ /* TRANSLATORS: "Y (minute,minutes) left" until the battery's empty.
60+ Example: "59 minutes left" */
61+ str = g_strdup_printf (_("%d %s left"),
62+ minutes, g_dngettext (NULL, "minute", "minutes", minutes));
63+ }
64 }
65 }
66 else
67@@ -700,6 +718,7 @@
68
69 if (p->state == UP_DEVICE_STATE_FULLY_CHARGED)
70 {
71+ /* TRANSLATORS: example: "battery (charged)" */
72 str = g_strdup_printf (_("%s (charged)"), kind_str);
73 }
74 else
75@@ -715,9 +734,14 @@
76 }
77
78 if (time_str && *time_str)
79- str = g_strdup_printf (_("%s (%s)"), kind_str, time_str);
80+ {
81+ /* TRANSLATORS: example: "battery (time remaining)" */
82+ str = g_strdup_printf (_("%s (%s)"), kind_str, time_str);
83+ }
84 else
85- str = g_strdup (kind_str);
86+ {
87+ str = g_strdup (kind_str);
88+ }
89
90 g_free (time_str);
91 }
92@@ -783,14 +807,17 @@
93
94 if (want_time && want_percent)
95 {
96+ /* TRANSLATORS: after the icon, a time-remaining string + battery %. Example: "(0:59, 33%)" */
97 str = g_strdup_printf (_("(%s, %.0lf%%)"), time_str, p->percentage);
98 }
99 else if (want_time)
100 {
101+ /* TRANSLATORS: after the icon, a time-remaining string Example: "(0:59)" */
102 str = g_strdup_printf (_("(%s)"), time_str);
103 }
104 else if (want_percent)
105 {
106+ /* TRANSLATORS: after the icon, a battery %. Example: "(33%)" */
107 str = g_strdup_printf (_("(%.0lf%%)"), p->percentage);
108 }
109 else
110
111=== modified file 'src/service.c'
112--- src/service.c 2014-03-13 22:27:29 +0000
113+++ src/service.c 2014-03-25 19:49:27 +0000
114@@ -1135,6 +1135,129 @@
115 }
116 }
117
118+/* If a device has multiple batteries and uses only one of them at a time,
119+ they should be presented as separate items inside the battery menu,
120+ but everywhere else they should be aggregated (bug 880881).
121+ Their percentages should be averaged. If any are discharging,
122+ the aggregated time remaining should be the maximum of the times
123+ for all those that are discharging, plus the sum of the times
124+ for all those that are idle. Otherwise, the aggregated time remaining
125+ should be the the maximum of the times for all those that are charging. */
126+static IndicatorPowerDevice *
127+create_totalled_battery_device (const GList * devices)
128+{
129+ const GList * l;
130+ guint n_charged = 0;
131+ guint n_charging = 0;
132+ guint n_discharging = 0;
133+ guint n_batteries = 0;
134+ double sum_percent = 0;
135+ time_t max_discharge_time = 0;
136+ time_t max_charge_time = 0;
137+ time_t sum_charged_time = 0;
138+ IndicatorPowerDevice * device = NULL;
139+
140+ for (l=devices; l!=NULL; l=l->next)
141+ {
142+ const IndicatorPowerDevice * device = INDICATOR_POWER_DEVICE(l->data);
143+
144+ if (indicator_power_device_get_kind(device) == UP_DEVICE_KIND_BATTERY)
145+ {
146+ const double percent = indicator_power_device_get_percentage (device);
147+ const time_t t = indicator_power_device_get_time (device);
148+ const UpDeviceState state = indicator_power_device_get_state (device);
149+
150+ ++n_batteries;
151+
152+ if (percent > 0.01)
153+ sum_percent += percent;
154+
155+ if (state == UP_DEVICE_STATE_CHARGING)
156+ {
157+ ++n_charging;
158+ max_charge_time = MAX(max_charge_time, t);
159+ }
160+ else if (state == UP_DEVICE_STATE_DISCHARGING)
161+ {
162+ ++n_discharging;
163+ max_discharge_time = MAX(max_discharge_time, t);
164+ }
165+ else if (state == UP_DEVICE_STATE_FULLY_CHARGED)
166+ {
167+ ++n_charged;
168+ sum_charged_time += t;
169+ }
170+ }
171+ }
172+
173+ if (n_batteries > 1)
174+ {
175+ const double percent = sum_percent / n_batteries;
176+ UpDeviceState state;
177+ time_t time_left;
178+
179+ if (n_discharging > 0)
180+ {
181+ state = UP_DEVICE_STATE_DISCHARGING;
182+ time_left = max_discharge_time + sum_charged_time;
183+ }
184+ else if (n_charging > 0)
185+ {
186+ state = UP_DEVICE_STATE_CHARGING;
187+ time_left = max_charge_time;
188+ }
189+ else if (n_charged > 0)
190+ {
191+ state = UP_DEVICE_STATE_FULLY_CHARGED;
192+ time_left = 0;
193+ }
194+ else
195+ {
196+ state = UP_DEVICE_STATE_UNKNOWN;
197+ time_left = 0;
198+ }
199+
200+ device = indicator_power_device_new (NULL,
201+ UP_DEVICE_KIND_BATTERY,
202+ percent,
203+ state,
204+ time_left);
205+ }
206+
207+ return device;
208+}
209+
210+/**
211+ * If there are multiple UP_DEVICE_KIND_BATTERY devices in the list,
212+ * they're merged into a new 'totalled' device representing the sum of them.
213+ *
214+ * Returns: (element-type IndicatorPowerDevice)(transfer full): a list of devices
215+ */
216+static GList*
217+merge_batteries_together (GList * devices)
218+{
219+ GList * ret;
220+ IndicatorPowerDevice * merged_device;
221+
222+ if ((merged_device = create_totalled_battery_device (devices)))
223+ {
224+ GList * l;
225+
226+ ret = g_list_append (NULL, merged_device);
227+
228+ for (l=devices; l!=NULL; l=l->next)
229+ if (indicator_power_device_get_kind(INDICATOR_POWER_DEVICE(l->data)) != UP_DEVICE_KIND_BATTERY)
230+ ret = g_list_append (ret, g_object_ref(l->data));
231+ }
232+ else /* not enough batteries to merge */
233+ {
234+ ret = g_list_copy (devices);
235+ g_list_foreach (ret, (GFunc)g_object_ref, NULL);
236+ }
237+
238+ return ret;
239+}
240+
241 IndicatorPowerDevice *
242 indicator_power_service_choose_primary_device (GList * devices)
243 {
244@@ -1142,13 +1265,10 @@
245
246 if (devices != NULL)
247 {
248- GList * tmp;
249-
250- tmp = g_list_copy (devices);
251+ GList * tmp = merge_batteries_together (devices);
252 tmp = g_list_sort (tmp, device_compare_func);
253 primary = g_object_ref (tmp->data);
254-
255- g_list_free (tmp);
256+ g_list_free_full (tmp, (GDestroyNotify)g_object_unref);
257 }
258
259 return primary;
260
261=== modified file 'tests/Makefile.am'
262--- tests/Makefile.am 2014-03-05 04:57:52 +0000
263+++ tests/Makefile.am 2014-03-25 19:49:27 +0000
264@@ -25,6 +25,13 @@
265 ### tests: indicator-power-device
266 ###
267
268+dear-reader-please-note-the-next-test-takes-90-seconds:
269+ @echo "#!/bin/bash" > $@
270+ @echo "exit 0" >> $@
271+ @chmod +x $@
272+TESTS += dear-reader-please-note-the-next-test-takes-90-seconds
273+CLEANFILES += dear-reader-please-note-the-next-test-takes-90-seconds
274+
275 TEST_LIBS = $(COVERAGE_LDFLAGS) libgtest.a -lpthread
276 TEST_CPPFLAGS = $(SERVICE_DEPS_CFLAGS) $(AM_CPPFLAGS)
277
278
279=== modified file 'tests/test-device.cc'
280--- tests/test-device.cc 2014-03-13 15:32:04 +0000
281+++ tests/test-device.cc 2014-03-25 19:49:27 +0000
282@@ -19,6 +19,7 @@
283
284 #include <gio/gio.h>
285 #include <gtest/gtest.h>
286+#include <cmath> // ceil()
287 #include "device.h"
288 #include "service.h"
289
290@@ -668,77 +669,106 @@
291 g_free (real_lang);
292 }
293
294-
295-/* The menu title should tell you at a glance what you need to know most: what
296- device will lose power soonest (and optionally when), or otherwise which
297- device will take longest to charge (and optionally how long it will take). */
298+/* If a device has multiple batteries and uses only one of them at a time,
299+ they should be presented as separate items inside the battery menu,
300+ but everywhere else they should be aggregated (bug 880881).
301+ Their percentages should be averaged. If any are discharging,
302+ the aggregated time remaining should be the maximum of the times
303+ for all those that are discharging, plus the sum of the times
304+ for all those that are idle. Otherwise, the aggregated time remaining
305+ should be the the maximum of the times for all those that are charging. */
306 TEST_F(DeviceTest, ChoosePrimary)
307 {
308- GList * device_list;
309- IndicatorPowerDevice * a;
310- IndicatorPowerDevice * b;
311-
312- a = indicator_power_device_new ("/org/freedesktop/UPower/devices/mouse",
313- UP_DEVICE_KIND_MOUSE,
314- 0.0,
315- UP_DEVICE_STATE_DISCHARGING,
316- 0);
317- b = indicator_power_device_new ("/org/freedesktop/UPower/devices/battery",
318- UP_DEVICE_KIND_BATTERY,
319- 0.0,
320- UP_DEVICE_STATE_DISCHARGING,
321- 0);
322-
323- /* device states + time left to {discharge,charge} + % of charge left,
324- sorted in order of preference wrt the spec's criteria.
325- So tests[i] should be picked over any test with an index greater than i */
326- struct {
327- int kind;
328- int state;
329+ struct Description
330+ {
331+ const char * path;
332+ UpDeviceKind kind;
333+ UpDeviceState state;
334 guint64 time;
335 double percentage;
336+ };
337+
338+ const Description descriptions[] = {
339+ { "/some/path/d0", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 10, 60.0 }, // 0
340+ { "/some/path/d1", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 20, 80.0 }, // 1
341+ { "/some/path/d2", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 30, 100.0 }, // 2
342+
343+ { "/some/path/c0", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 10, 60.0 }, // 3
344+ { "/some/path/c1", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 20, 80.0 }, // 4
345+ { "/some/path/c2", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 30, 100.0 }, // 5
346+
347+ { "/some/path/f0", UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_FULLY_CHARGED, 0, 100.0 }, // 6
348+ { "/some/path/m0", UP_DEVICE_KIND_MOUSE, UP_DEVICE_STATE_DISCHARGING, 20, 80.0 }, // 7
349+ { "/some/path/m1", UP_DEVICE_KIND_MOUSE, UP_DEVICE_STATE_FULLY_CHARGED, 0, 100.0 }, // 8
350+ { "/some/path/pw", UP_DEVICE_KIND_LINE_POWER, UP_DEVICE_STATE_UNKNOWN, 0, 0.0 } // 9
351+ };
352+
353+ std::vector<IndicatorPowerDevice*> devices;
354+ for(const auto& desc : descriptions)
355+ devices.push_back(indicator_power_device_new(desc.path, desc.kind, desc.percentage, desc.state, desc.time));
356+
357+ const struct {
358+ std::vector<int> device_indices;
359+ Description expected;
360 } tests[] = {
361- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 49, 50.0 },
362- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 50, 50.0 },
363- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 50, 100.0 },
364- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 51, 50.0 },
365- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 50, 50.0 },
366- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 49, 50.0 },
367- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 49, 100.0 },
368- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 48, 50.0 },
369- { UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_FULLY_CHARGED, 0, 50.0 },
370- { UP_DEVICE_KIND_KEYBOARD, UP_DEVICE_STATE_FULLY_CHARGED, 0, 50.0 },
371- { UP_DEVICE_KIND_LINE_POWER, UP_DEVICE_STATE_UNKNOWN, 0, 0.0 }
372+
373+ { { 0 }, descriptions[0] }, // 1 discharging
374+ { { 0, 1 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 20, 70.0 } }, // 2 discharging
375+ { { 1, 2 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 30, 90.0 } }, // 2 discharging
376+ { { 0, 1, 2 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 30, 80.0 } }, // 3 discharging
377+
378+ { { 3 }, descriptions[3] }, // 1 charging
379+ { { 3, 4 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 20, 70.0 } }, // 2 charging
380+ { { 4, 5 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 30, 90.0 } }, // 2 charging
381+ { { 3, 4, 5 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 30, 80.0 } }, // 3 charging
382+
383+ { { 6 }, descriptions[6] }, // 1 charged
384+ { { 6, 0 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 10, 80.0 } }, // 1 charged, 1 discharging
385+ { { 6, 3 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_CHARGING, 10, 80.0 } }, // 1 charged, 1 charging
386+ { { 6, 0, 3 }, { nullptr, UP_DEVICE_KIND_BATTERY, UP_DEVICE_STATE_DISCHARGING, 10, 73.3 } }, // 1 charged, 1 charging, 1 discharging
387+
388+ { { 0, 7 }, descriptions[0] }, // 1 discharging battery, 1 discharging mouse. pick the one with the least time left.
389+ { { 2, 7 }, descriptions[7] }, // 1 discharging battery, 1 discharging mouse. pick the one with the least time left.
390+
391+ { { 0, 8 }, descriptions[0] }, // 1 discharging battery, 1 fully-charged mouse. pick the one that's discharging.
392+ { { 6, 7 }, descriptions[7] }, // 1 discharging mouse, 1 fully-charged battery. pick the one that's discharging.
393+
394+ { { 0, 9 }, descriptions[0] }, // everything comes before power lines
395+ { { 3, 9 }, descriptions[3] },
396+ { { 7, 9 }, descriptions[7] }
397 };
398-
399- device_list = NULL;
400- device_list = g_list_append (device_list, a);
401- device_list = g_list_append (device_list, b);
402-
403- for (int i=0, n=G_N_ELEMENTS(tests); i<n; i++)
404- {
405- for (int j=i+1; j<n; j++)
406- {
407- g_object_set (a, INDICATOR_POWER_DEVICE_KIND, tests[i].kind,
408- INDICATOR_POWER_DEVICE_STATE, tests[i].state,
409- INDICATOR_POWER_DEVICE_TIME, guint64(tests[i].time),
410- INDICATOR_POWER_DEVICE_PERCENTAGE, tests[i].percentage,
411- NULL);
412- g_object_set (b, INDICATOR_POWER_DEVICE_KIND, tests[j].kind,
413- INDICATOR_POWER_DEVICE_STATE, tests[j].state,
414- INDICATOR_POWER_DEVICE_TIME, guint64(tests[j].time),
415- INDICATOR_POWER_DEVICE_PERCENTAGE, tests[j].percentage,
416- NULL);
417- ASSERT_EQ (a, indicator_power_service_choose_primary_device(device_list));
418- g_object_unref (G_OBJECT(a));
419-
420- /* reverse the list to check that list order doesn't matter */
421- device_list = g_list_reverse (device_list);
422- ASSERT_EQ (a, indicator_power_service_choose_primary_device(device_list));
423- g_object_unref (G_OBJECT(a));
424- }
425- }
426-
427- // cleanup
428- g_list_free_full (device_list, g_object_unref);
429+
430+ for(const auto& test : tests)
431+ {
432+ const auto& x = test.expected;
433+
434+ GList * device_glist = nullptr;
435+ for(const auto& i : test.device_indices)
436+ device_glist = g_list_append(device_glist, devices[i]);
437+
438+ auto primary = indicator_power_service_choose_primary_device(device_glist);
439+ EXPECT_STREQ(x.path, indicator_power_device_get_object_path(primary));
440+ EXPECT_EQ(x.kind, indicator_power_device_get_kind(primary));
441+ EXPECT_EQ(x.state, indicator_power_device_get_state(primary));
442+ EXPECT_EQ(x.time, indicator_power_device_get_time(primary));
443+ EXPECT_EQ((int)ceil(x.percentage), (int)ceil(indicator_power_device_get_percentage(primary)));
444+ g_object_unref(primary);
445+
446+ // reverse the list and repeat the test
447+ // to confirm that list order doesn't matter
448+ device_glist = g_list_reverse (device_glist);
449+ primary = indicator_power_service_choose_primary_device (device_glist);
450+ EXPECT_STREQ(x.path, indicator_power_device_get_object_path(primary));
451+ EXPECT_EQ(x.kind, indicator_power_device_get_kind(primary));
452+ EXPECT_EQ(x.state, indicator_power_device_get_state(primary));
453+ EXPECT_EQ(x.time, indicator_power_device_get_time(primary));
454+ EXPECT_EQ((int)ceil(x.percentage), (int)ceil(indicator_power_device_get_percentage(primary)));
455+ g_object_unref(primary);
456+
457+ // cleanup
458+ g_list_free(device_glist);
459+ }
460+
461+ for (auto& device : devices)
462+ g_object_unref (device);
463 }

Subscribers

People subscribed via source and target branches