Merge lp:~hloeung/indicator-power/show-percentage into lp:indicator-power/13.10

[Haw Loeung]

Add option to display battery percentage in both menu bar and display menu (LP: #811777).

Add option to display battery percentage in both menu bar and display menu (LP: #811777).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:188
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/46/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/17

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

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

The code is fine; thank you for writing this patch.

One issue is that now half of indicator-power's menu is filled with display toggles. Maybe it would make more sense in the panel.

Most importantly, though, the feature's placement (and existence) needs to be signed-off by Design. This change isn't in mpt's spec <https://wiki.ubuntu.com/Power> so I'm not approving this patch as-is. Please ping mpt and ask him to re-engage on this ticket.

One final option would be to make this an "invisible" feature accessible through dconf-editor, much like indicator-bluetooth's visibility toggle or indicator-datetime's custom-time-format string.

review: Needs Fixing

[Matthew Paul Thomas (mpt)]

Thanks Haw. I've updated the design spec to include the "Show Percentage in Menu Bar" item. <https://wiki.ubuntu.com/Power?action=diff&rev2=37&rev1=36>

It's not clear to me from the code, but I think you might be altering the text of the menu items and not the menu title. I think we want the other way around: alter the menu title, not the individual menu items.

review: Needs Fixing (design)

[lp:~hloeung/indicator-power/show-percentage]

189. By Haw Loeung on 2013-08-22

Fixed as per updated design spec - https://wiki.ubuntu.com/Power.

190. By Haw Loeung on 2013-08-22

Pull in changes from upstream and resolve conflicts.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:190
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/48/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-amd64-ci/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/19/console

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

review: Needs Fixing (continuous-integration)

[lp:~hloeung/indicator-power/show-percentage]

191. By Haw Loeung on 2013-08-23

Don't use deprecated g_simple_action_group_insert(), use g_action_map_add_action() instead.

192. By Haw Loeung on 2013-08-23

Fixed up test cases so package now builds successfully.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:192
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/49/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/20/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[lp:~hloeung/indicator-power/show-percentage]

193. By Haw Loeung on 2013-08-23

Fixed showing two brackets when charging.

194. By Haw Loeung on 2013-08-23

As per updated design spec, 'Show Time in Menu Bar' should be enabled by default.

[Haw Loeung (hloeung)]

Fixed up as per updated design spec. See screenshots below:

http://people.canonical.com/~hloeung/indicator-power/charge-time-only.png
http://people.canonical.com/~hloeung/indicator-power/charge-percentage-only.png
http://people.canonical.com/~hloeung/indicator-power/charge-both.png

http://people.canonical.com/~hloeung/indicator-power/discharge-time-only.png
http://people.canonical.com/~hloeung/indicator-power/discharge-percentage-only.png
http://people.canonical.com/~hloeung/indicator-power/discharge-both.png

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:194
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/50/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-saucy-armhf-ci/21/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

mpt,thanks for looking into this.

I've got a patch based on Haw's that follows the new spec; MP forthcoming.

[Charles Kerr (charlesk)]

Oh! we were working on the same thing. I guess I should have commented sooner :)

[Charles Kerr (charlesk)]

Haw, there are still a couple of issues here, the menuitem's labels are still getting changed. Also there are fringe cases for showing the % in the header when we shouldn't, such as when the primary device is UP_DEVICE_KIND_LINE_POWER.

When we were accidentally coding in parallel a few hours ago, I worked on a branch based off your earlier one, which fixes these and also adds unit tests for how header's label will look for all combinations of the time & percentage flag settings.

If you're agreeable, I'll MP it (lp:~charlesk/indicator-power/lp-811777) and we can go from there. On the other hand, you were here first and I don't want to step on your toes -- if you want to continue on with this current MP that's great too. :-)

[Haw Loeung (hloeung)]

Marking this as invalid. See https://code.launchpad.net/~charlesk/indicator-power/lp-811777/+merge/181707

Unmerged revisions

194. By Haw Loeung on 2013-08-23

As per updated design spec, 'Show Time in Menu Bar' should be enabled by default.

193. By Haw Loeung on 2013-08-23

Fixed showing two brackets when charging.

192. By Haw Loeung on 2013-08-23

Fixed up test cases so package now builds successfully.

191. By Haw Loeung on 2013-08-23

Don't use deprecated g_simple_action_group_insert(), use g_action_map_add_action() instead.

190. By Haw Loeung on 2013-08-22

Pull in changes from upstream and resolve conflicts.

189. By Haw Loeung on 2013-08-22

Fixed as per updated design spec - https://wiki.ubuntu.com/Power.

188. By Haw Loeung on 2013-08-12

Fixed percentages as I think it looks nicer outside of parentheses.

187. By Haw Loeung on 2013-08-12

Fixed so that both time and percentage could be shown.

186. By Haw Loeung on 2013-08-12

Show percentage in drop-down menu and added option to show percentage in menu bar - LP:811777.

=== modified file 'data/com.canonical.indicator.power.gschema.xml.in'
--- data/com.canonical.indicator.power.gschema.xml.in	2012-02-02 05:00:03 +0000
+++ data/com.canonical.indicator.power.gschema.xml.in	2013-08-23 01:04:40 +0000
@@ -6,10 +6,15 @@
   </enum>
   <schema gettext-domain="@GETTEXT_PACKAGE@" id="com.canonical.indicator.power" path="/com/canonical/indicator/power/">
     <key name="show-time" type="b">
-      <default>false</default>
+      <default>true</default>
       <_summary>Show time in Menu Bar</_summary>
       <_description>Whether or not to show the time in the menu bar.</_description>
     </key>
+    <key name="show-percentage" type="b">
+      <default>false</default>
+      <_summary>Show percentage in Menu Bar</_summary>
+      <_description>Whether or not to show the percentage in the menu bar.</_description>
+    </key>
     <key enum="icon-policy-enum" name="icon-policy">
       <default>"present"</default>
       <_summary>When to show the battery status in the menu bar.</_summary>

=== modified file 'src/device.c'
--- src/device.c	2013-06-17 04:16:19 +0000
+++ src/device.c	2013-08-23 01:04:40 +0000
@@ -484,7 +484,7 @@
 
   if (minutes == 0)
     {
-      *detailed_timestring = g_strdup_printf (g_dngettext (GETTEXT_PACKAGE, 
+      *detailed_timestring = g_strdup_printf (g_dngettext (GETTEXT_PACKAGE,
                                               "%i hour",
                                               "%i hours",
                                               hours), hours);
@@ -595,8 +595,8 @@
         {
           /* TRANSLATORS: %2 is a time string, e.g. "1 hour 5 minutes" */
           *accessible_name = g_strdup_printf (_("%s (%s to charge (%.0lf%%))"), device_name, detailed_timestring, percentage);
-          *details = g_strdup_printf (_("%s (%s to charge)"), device_name, short_timestring);
-          *short_details = g_strdup_printf ("(%s)", short_timestring);
+          *details = g_strdup_printf (_("%s (%s to charge (%.0lf%%))"), device_name, short_timestring, percentage);
+          *short_details = g_strdup (short_timestring);
         }
       else if ((state == UP_DEVICE_STATE_DISCHARGING) && (time > (60*60*12)))
         {
@@ -608,7 +608,7 @@
         {
           /* TRANSLATORS: %2 is a time string, e.g. "1 hour 5 minutes" */
           *accessible_name = g_strdup_printf (_("%s (%s left (%.0lf%%))"), device_name, detailed_timestring, percentage);
-          *details = g_strdup_printf (_("%s (%s left)"), device_name, short_timestring);
+          *details = g_strdup_printf (_("%s (%s left (%.0lf%%))"), device_name, short_timestring, percentage);
           *short_details = g_strdup (short_timestring);
         }
 

=== modified file 'src/service.c'
--- src/service.c	2013-08-20 03:47:23 +0000
+++ src/service.c	2013-08-23 01:04:40 +0000
@@ -32,6 +32,7 @@
 #define BUS_PATH "/com/canonical/indicator/power"
 
 #define SETTINGS_SHOW_TIME_S "show-time"
+#define SETTINGS_SHOW_PERCENTAGE_S "show-percentage"
 #define SETTINGS_ICON_POLICY_S "icon-policy"
 
 G_DEFINE_TYPE (IndicatorPowerService,
@@ -112,6 +113,7 @@
   GSimpleActionGroup * actions;
   GSimpleAction * header_action;
   GSimpleAction * show_time_action;
+  GSimpleAction * show_percentage_action;
   GSimpleAction * battery_level_action;
   GSimpleAction * brightness_action;
 
@@ -311,6 +313,8 @@
 create_header_state (IndicatorPowerService * self)
 {
   GVariantBuilder b;
+  gchar * short_time = NULL;
+  gdouble percentage = 0;
   gchar * label = NULL;
   gchar * a11y = NULL;
   GIcon * icon = NULL;
@@ -321,10 +325,12 @@
       gchar * details;
 
       indicator_power_device_get_time_details (p->primary_device,
-                                               &label,
+                                               &short_time,
                                                &details,
                                                &a11y);
 
+      percentage = indicator_power_device_get_percentage (p->primary_device);
+
       icon = indicator_power_device_get_gicon (p->primary_device);
 
       g_free (details);
@@ -335,11 +341,26 @@
   g_variant_builder_add (&b, "{sv}", "visible",
                          g_variant_new_boolean (should_be_visible (self)));
 
+  /* Show both time and percentage */
+  if ((short_time != NULL) &&
+    g_settings_get_boolean (p->settings, SETTINGS_SHOW_TIME_S) &&
+    g_settings_get_boolean (p->settings, SETTINGS_SHOW_PERCENTAGE_S))
+      label = g_strdup_printf (_("(%s %.0lf%%)"), short_time, percentage);
+  /* Show only time */
+  else if ((short_time != NULL) &&
+    g_settings_get_boolean (p->settings, SETTINGS_SHOW_TIME_S))
+      label = g_strdup_printf (_("(%s)"), short_time);
+  /* Show only percentage */
+  else if (g_settings_get_boolean (p->settings, SETTINGS_SHOW_PERCENTAGE_S))
+    label = g_strdup_printf (_("(%.0lf%%)"), percentage);
+
+  if (short_time != NULL)
+    g_free (short_time);
+
   if (label != NULL)
     {
-      if (g_settings_get_boolean (p->settings, SETTINGS_SHOW_TIME_S))
-        g_variant_builder_add (&b, "{sv}", "label",
-                               g_variant_new_string (label));
+      g_variant_builder_add (&b, "{sv}", "label",
+                             g_variant_new_string (label));
 
       g_free (label);
     }
@@ -521,6 +542,10 @@
                  "indicator.show-time");
 
   g_menu_append (menu,
+                 _("Show Percentage in Menu Bar"),
+                 "indicator.show-percentage");
+
+  g_menu_append (menu,
                  _("Power Settings…"),
                  "indicator.activate-settings");
 
@@ -744,6 +769,45 @@
   g_variant_unref (v);
 }
 
+static void
+set_show_percentage_flag (IndicatorPowerService * self, gboolean b)
+{
+  GVariant * v;
+  priv_t * p = self->priv;
+
+  /* update the settings */
+  if (b != g_settings_get_boolean (p->settings, SETTINGS_SHOW_PERCENTAGE_S))
+    g_settings_set_boolean (p->settings, SETTINGS_SHOW_PERCENTAGE_S, b);
+
+  /* update the action state */
+  v = g_action_get_state (G_ACTION(p->show_percentage_action));
+  if (b != g_variant_get_boolean (v))
+    g_simple_action_set_state (p->show_percentage_action,
+                               g_variant_new_boolean (b));
+  g_variant_unref (v);
+
+  rebuild_header_now (self);
+}
+static void
+on_show_percentage_setting_changed (GSettings * settings,
+                                    gchar * key,
+                                    gpointer gself)
+{
+  set_show_percentage_flag (INDICATOR_POWER_SERVICE(gself),
+                            g_settings_get_boolean (settings, key));
+}
+
+static void
+on_show_percentage_action_state_changed (GAction    * action,
+                                         GParamSpec * pspec   G_GNUC_UNUSED,
+                                         gpointer     gself)
+{
+  GVariant * v = g_action_get_state (action);
+  set_show_percentage_flag (INDICATOR_POWER_SERVICE(gself),
+                            g_variant_get_boolean (v));
+  g_variant_unref (v);
+}
+
 /* toggles the state */
 static void
 on_show_time_action_activated (GSimpleAction * simple,
@@ -755,12 +819,23 @@
   g_simple_action_set_state (simple, g_variant_new_boolean (!flag));
   g_variant_unref (v);
 }
+static void
+on_show_percentage_action_activated (GSimpleAction * simple,
+                                     GVariant      * parameter G_GNUC_UNUSED,
+                                     gpointer        unused    G_GNUC_UNUSED)
+{
+  GVariant * v = g_action_get_state (G_ACTION (simple));
+  gboolean flag = g_variant_get_boolean (v);
+  g_simple_action_set_state (simple, g_variant_new_boolean (!flag));
+  g_variant_unref (v);
+}
 
 static void
 init_gactions (IndicatorPowerService * self)
 {
   GSimpleAction * a;
   gboolean show_time;
+  gboolean show_percentage;
   priv_t * p = self->priv;
 
   GActionEntry entries[] = {
@@ -777,18 +852,18 @@
 
   /* add the header action */
   a = g_simple_action_new_stateful ("_header", NULL, create_header_state (self));
-  g_simple_action_group_insert (p->actions, G_ACTION(a));
+  g_action_map_add_action (G_ACTION_MAP(p->actions), G_ACTION(a));
   p->header_action = a;
 
   /* add the power-level action */
   a = g_simple_action_new_stateful ("battery-level", NULL, g_variant_new_uint32(0));
   g_simple_action_set_enabled (a, FALSE);
-  g_simple_action_group_insert (p->actions, G_ACTION(a));
+  g_action_map_add_action (G_ACTION_MAP(p->actions), G_ACTION(a));
   p->battery_level_action = a;
 
   /* add the brightness action */
   a = g_simple_action_new_stateful ("brightness", NULL, action_state_for_brightness (self));
-  g_simple_action_group_insert (p->actions, G_ACTION(a));
+  g_action_map_add_action (G_ACTION_MAP(p->actions), G_ACTION(a));
   g_signal_connect (a, "change-state", G_CALLBACK(on_brightness_change_requested), self);
   p->brightness_action = a;
 
@@ -801,9 +876,22 @@
                     G_CALLBACK(on_show_time_action_activated), self);
   g_signal_connect (a, "notify",
                     G_CALLBACK(on_show_time_action_state_changed), self);
-  g_simple_action_group_insert (p->actions, G_ACTION(a));
+  g_action_map_add_action (G_ACTION_MAP(p->actions), G_ACTION(a));
   p->show_time_action = a;
 
+  /* add the show-percentage action */
+  show_percentage = g_settings_get_boolean (p->settings,
+                                            SETTINGS_SHOW_PERCENTAGE_S);
+  a = g_simple_action_new_stateful ("show-percentage",
+                                    NULL,
+                                    g_variant_new_boolean(show_percentage));
+  g_signal_connect (a, "activate",
+                    G_CALLBACK(on_show_percentage_action_activated), self);
+  g_signal_connect (a, "notify",
+                    G_CALLBACK(on_show_percentage_action_state_changed), self);
+  g_action_map_add_action (G_ACTION_MAP(p->actions), G_ACTION(a));
+  p->show_percentage_action = a;
+
   rebuild_header_now (self);
 }
 
@@ -949,7 +1037,7 @@
 {
   IndicatorPowerService * self = INDICATOR_POWER_SERVICE (o);
   priv_t * p = self->priv;
- 
+
   switch (property_id)
     {
       case PROP_DEVICE_PROVIDER:
@@ -1014,10 +1102,18 @@
       g_clear_object (&p->show_time_action);
     }
 
+  if (p->show_percentage_action != NULL)
+    {
+      g_signal_handlers_disconnect_by_data (p->show_percentage_action, self);
+
+      g_clear_object (&p->show_percentage_action);
+    }
+
   g_clear_object (&p->brightness_action);
   g_clear_object (&p->battery_level_action);
   g_clear_object (&p->header_action);
   g_clear_object (&p->show_time_action);
+  g_clear_object (&p->show_percentage_action);
   g_clear_object (&p->actions);
 
   g_clear_object (&p->conn);
@@ -1053,6 +1149,8 @@
                             G_CALLBACK(rebuild_header_now), self);
   g_signal_connect (p->settings, "changed::" SETTINGS_SHOW_TIME_S,
                     G_CALLBACK(on_show_time_setting_changed), self);
+  g_signal_connect (p->settings, "changed::" SETTINGS_SHOW_PERCENTAGE_S,
+                    G_CALLBACK(on_show_percentage_setting_changed), self);
 
   p->own_id = g_bus_own_name (G_BUS_TYPE_SESSION,
                               BUS_NAME,

=== modified file 'tests/test-device.cc'
--- tests/test-device.cc	2013-06-17 23:01:51 +0000
+++ tests/test-device.cc	2013-08-23 01:04:40 +0000
@@ -4,16 +4,16 @@
 Authors:
     Charles Kerr <[e-mail]>
 
-This program is free software: you can redistribute it and/or modify it 
-under the terms of the GNU General Public License version 3, as published 
+This program is free software: you can redistribute it and/or modify it
+under the terms of the GNU General Public License version 3, as published
 by the Free Software Foundation.
 
-This program is distributed in the hope that it will be useful, but 
-WITHOUT ANY WARRANTY; without even the implied warranties of 
-MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR 
+This program is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranties of
+MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
 PURPOSE.  See the GNU General Public License for more details.
 
-You should have received a copy of the GNU General Public License along 
+You should have received a copy of the GNU General Public License along
 with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
@@ -266,7 +266,7 @@
       g_object_set (o, INDICATOR_POWER_DEVICE_KIND, kind,
                        INDICATOR_POWER_DEVICE_STATE, UP_DEVICE_STATE_EMPTY,
                        NULL);
- 
+
       g_string_append_printf (expected, "%s-empty-symbolic;", kind_str);
       g_string_append_printf (expected, "gpm-%s-empty;", kind_str);
       g_string_append_printf (expected, "gpm-%s-000;", kind_str);
@@ -434,7 +434,7 @@
 
       // state unknown
       g_object_set (o, INDICATOR_POWER_DEVICE_KIND, kind,
-                       INDICATOR_POWER_DEVICE_STATE, UP_DEVICE_STATE_UNKNOWN, 
+                       INDICATOR_POWER_DEVICE_STATE, UP_DEVICE_STATE_UNKNOWN,
                        NULL);
       g_string_append_printf (expected, "%s-missing-symbolic;", kind_str);
       g_string_append_printf (expected, "gpm-%s-missing;", kind_str);
@@ -468,7 +468,7 @@
   /**
   ***
   **/
- 
+
   IndicatorPowerDevice * device = INDICATOR_POWER_DEVICE (g_object_new (INDICATOR_POWER_DEVICE_TYPE, NULL));
   o = G_OBJECT(device);
 
@@ -478,8 +478,8 @@
                    INDICATOR_POWER_DEVICE_PERCENTAGE, 50.0,
                    INDICATOR_POWER_DEVICE_TIME, guint64(60*61),
                    NULL);
-  check_strings (device, "(1:01)",
-                         "Battery (1:01 to charge)",
+  check_strings (device, "1:01",
+                         "Battery (1:01 to charge (50%))",
                          "Battery (1 hour 1 minute to charge (50%))");
 
   // discharging, < 12 hours left
@@ -489,7 +489,7 @@
                    INDICATOR_POWER_DEVICE_TIME, guint64(60*61),
                    NULL);
   check_strings (device, "1:01",
-                         "Battery (1:01 left)",
+                         "Battery (1:01 left (50%))",
                          "Battery (1 hour 1 minute left (50%))");
 
   // discharging, > 12 hours left
@@ -605,7 +605,7 @@
           ASSERT_EQ (a, indicator_power_service_choose_primary_device(device_list));
         }
     }
-    
+
   // cleanup
   g_list_free_full (device_list, g_object_unref);
 }