Fix i18n number formats in tray power displays

ash/system/chromeos/power/power_status_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/system/chromeos/power/power_status_view.h"
 
 #include "ash/shell.h"
 #include "ash/shell_delegate.h"
 #include "ash/system/chromeos/power/power_status.h"
 #include "ash/system/chromeos/power/tray_power.h"
 #include "ash/system/tray/fixed_sized_image_view.h"
 #include "ash/system/tray/tray_constants.h"
-#include "base/strings/string_number_conversions.h"
+#include "base/i18n/number_formatting.h"
 #include "base/strings/utf_string_conversions.h"
 #include "grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/time_format.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
 
@@ skipping 61 matching lines @@
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   base::string16 battery_percentage;
   base::string16 battery_time_status;
 
   if (status.IsBatteryFull()) {
     battery_time_status =
         rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_BATTERY_FULL);
   } else {
     battery_percentage = l10n_util::GetStringFUTF16(
         IDS_ASH_STATUS_TRAY_BATTERY_PERCENT_ONLY,
-        base::IntToString16(status.GetRoundedBatteryPercent()));
+        base::FormatNumber(status.GetRoundedBatteryPercent()));

[jungshik at Google, 2016/05/10 08:50:44]

To be 100% i18n-safe, you should use MessageFormatter instead of hardcoding '%'
sign. (I should have sent out a PSA on this API a long time ago). 

To use MessageFormatter, IDS_ASH_STATUS_TRAY...PERCENT_ONLY should be something
like this. 

{1,number,percent} 

(assuming that we just want integer percent. That is, 49% instead of 48.8%). 

Actually, "{1,number,percent}" does not need any translation at all. Then, just
remove IDS_ASH_STATUS_...PERCENT_ONLY and pass the above string directly to
MessageFormatter. 

And, you have to pass 0.488 instead of 48 to the API. Does |status| have a
method to get a raw fraction (instead of rounded percent)?

So, simply doing this will do all the work for you: 

battery_percent = base::MessageFormatter::FormatWithNumberedArgs(
    "{1,number,percent}",  fractional_battery_charging_status); 

See an example at https://codereview.chromium.org/1958073003

[jungshik at Google, 2016/05/10 09:28:17]

Oops. The 1st argument should be |base::ASCIIToUTF16("{1,number,percent}")|

[Greg Levin, 2016/05/10 23:33:43]

Done.

     if (status.IsUsbChargerConnected()) {
       battery_time_status = rb.GetLocalizedString(
           IDS_ASH_STATUS_TRAY_BATTERY_CHARGING_UNRELIABLE);
     } else if (status.IsBatteryTimeBeingCalculated()) {
       battery_time_status =
           rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_BATTERY_CALCULATING);
     } else {
       base::TimeDelta time = status.IsBatteryCharging() ?
           status.GetBatteryTimeToFull() : status.GetBatteryTimeToEmpty();
       if (PowerStatus::ShouldDisplayBatteryTime(time) &&
           !status.IsBatteryDischargingOnLinePower()) {
         int hour = 0, min = 0;
         PowerStatus::SplitTimeIntoHoursAndMinutes(time, &hour, &min);
-        base::string16 minute = min < 10 ?
-            base::ASCIIToUTF16("0") + base::IntToString16(min) :
-            base::IntToString16(min);
-        battery_time_status =
-            l10n_util::GetStringFUTF16(
-                status.IsBatteryCharging() ?
-                IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL_SHORT :
-                IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT,
-                base::IntToString16(hour),
-                minute);
+        base::string16 minute =
+            min < 10 ? base::FormatNumber(0) + base::FormatNumber(min)
+                     : base::FormatNumber(min);

[Daniel Erat, 2016/05/03 19:17:33]

i'm also curious about the i18n-ness of this zero-padding. from some searching,
it _looks_ to me like the three languages mentioned in the bug report all use
base-10 numerals, but do they zero-pad minutes?

[stevenjb, 2016/05/03 19:49:09]

It seems like if we are going to switch this to FormatNumber we should add a
helper that uses NumberFormat::setMinimumIntegerDigits(). Otherwise I'm not sure
if this change is especially helpful or even correct.

[jungshik at Google, 2016/05/10 08:50:44]

There are two ways to handle this better depending on whether ICU's
MeasureFormat supports a compact 'h:mm' format as opposed to 'h hours and m
minutes'. 

If it does, use that directly. (let me test it and come back here later).  Well,
I've just checked the API. It does. (
http://icu-project.org/apiref/icu4c/measfmt_8h.html#details )


However, using  base/i18n/message_formatter might be simpler here. Your messages
would look like these (in grd file):


{<ph name="HOUR">{1, number, integer}<ex>5</ex></ph>:<ph
name="MINUTE">{2,number,*0##}<ex>07</ex></ph> until full}

{<ph name="HOUR">{1, number, integer}<ex>5</ex></ph>:<ph
name="MINUTE">{2,number,*0##}<ex>07</ex></ph> left}

Linguists can only change ':', 'until full/left' and the order of fields (hour
first or minute first) so that you don't have to worry about '{2,number,*0##}'
getting munged by mistake. 

string16 pattern = l10n_util::GetString(is_charging ? 
   IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL_SHORT : IDS_ASH...LEFT_SHORT)
battery_time_status = base::i18n::MessageFormatter(pattern, hour, min); 

[Greg Levin, 2016/05/10 23:33:43]

I went with the second option.  Two points, though:

1) I don't see a lot of MessageFormatter format strings in grpd files, although
I may not be looking hard enough.  Wouldn't it be equivalent to keep the
existing format strings, and use MessageFormatter to generate hour and minute
strings separately?

2) The '0' in '*0##' isn't being translated.  In a language with non-Arabic
numeric glyphs, 3:00 appears with one 0 and one <0 glyph>.  Is there another way
to specify the minimum number of digits?  Alternately, could I leave
instructions in the string's desc field to translate that 0 to the correct
glyph?

[Greg Levin, 2016/05/11 19:43:02]

EDIT: I updated it to use the new TimeDurationFormat().  Tested in English,
Romanian (different punctuation), and Bengali (different numeric glyphs), and
they *seem* to look fine.

+        battery_time_status = l10n_util::GetStringFUTF16(
+            status.IsBatteryCharging()
+                ? IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL_SHORT
+                : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT,
+            base::FormatNumber(hour), minute);
       }
     }
     battery_percentage = battery_time_status.empty() ?
         battery_percentage : battery_percentage + base::ASCIIToUTF16(" - ");
   }
   percentage_label_->SetVisible(!battery_percentage.empty());
   percentage_label_->SetText(battery_percentage);
   time_status_label_->SetVisible(!battery_time_status.empty());
   time_status_label_->SetText(battery_time_status);

[jungshik at Google, 2016/05/10 08:50:44]

wow...  has appending ' - ' to battery_percentage been tested in RTL locales
(he, fa, ar)?   It's not related to this bug. (so that you don't have to fix it
here). 

I guess labels are shown right-to-left, but '37% - ' may remain as it is instead
of becoming (visually) ' - 37%'.  Visually, we may get something like this in
RTL locale. 

<2:35 until full> <37% -> 

Anyway, it's a separate issue. (one solution is to combine two labels into one).

[Greg Levin, 2016/05/10 23:33:43]

Good catch... just checked, and that's exactly what it's doing in RTL.  In
fact...
https://bugs.chromium.org/p/chromium/issues/detail?id=126770 :-)

If you've no objections, I'll take over that bug and fix it once this one's
done.

 }
 
 void PowerStatusView::ChildPreferredSizeChanged(views::View* child) {
   PreferredSizeChanged();
 }
 
 gfx::Size PowerStatusView::GetPreferredSize() const {
   gfx::Size size = views::View::GetPreferredSize();
   return gfx::Size(size.width(), kTrayPopupItemHeight);
 }
 
 int PowerStatusView::GetHeightForWidth(int width) const {
   return kTrayPopupItemHeight;
 }
 
 void PowerStatusView::Layout() {
   views::View::Layout();
 
   // Move the time_status_label_ closer to percentage_label_.
   if (percentage_label_ && time_status_label_ &&
       percentage_label_->visible() && time_status_label_->visible()) {
     time_status_label_->SetX(percentage_label_->bounds().right() + 1);
   }
 }
 
 }  // namespace ash