Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1951493002:
Fix i18n number formats in tray power displays (Closed)
|
Created:
4 years, 7 months ago by Greg Levin Modified:
4 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org, derat+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix i18n number formats in tray power displays
BUG=612181
TEST=Switch language to Bengali (India), open systray menu, notice that
power values (% and time remaining) are displayed using Bengali, rather
than Arabic, numberic characters.
Committed: https://crrev.com/5cff4d12a49e6164f4c12f0179cacf46a20d9fa4
Cr-Commit-Position: refs/heads/master@{#394830}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Use MessageFormatter in power_status_view.cc #
Total comments: 4
Patch Set 3 : Move time formatting into TimeDurationFormat() #
Total comments: 12
Patch Set 4 : Address reviews #Patch Set 5 : Add unit test for TimeDurationFormat() #
Total comments: 6
Patch Set 6 : Fix test, address reviews #
Total comments: 1
Patch Set 7 : Switch unit test from Bengali to Persian for Android (and merge) #
Messages
Total messages: 48 (14 generated)
glevin@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@, do you have time to take a quick look, since you're already cc'd on 475351? This is just fixing the power display, not the larger issue of auditing other numeric i18n problems.
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... ash/system/chromeos/power/battery_notification.cc:67: IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL, base::FormatNumber(hour), just to check, is some combination of hours and minutes a reasonable assumption to make for internationally-formatting a duration? there's base/i18n/time_formatting.h, but it doesn't look like it supports durations. https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); 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?
https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... ash/system/chromeos/power/battery_notification.cc:67: IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL, base::FormatNumber(hour), On 2016/05/03 19:17:33, Daniel Erat wrote: > just to check, is some combination of hours and minutes a reasonable assumption > to make for internationally-formatting a duration? there's > base/i18n/time_formatting.h, but it doesn't look like it supports durations. +1. This seems like something that we ought to have a common helper function for, maybe using TimeUnitAmount? https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); On 2016/05/03 19:17:33, Daniel Erat wrote: > 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? 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.
jshin@chromium.org changed reviewers: + jshin@chromium.org
https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... ash/system/chromeos/power/battery_notification.cc:67: IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL, base::FormatNumber(hour), On 2016/05/03 19:49:09, stevenjb wrote: > On 2016/05/03 19:17:33, Daniel Erat wrote: > > just to check, is some combination of hours and minutes a reasonable > assumption > > to make for internationally-formatting a duration? there's > > base/i18n/time_formatting.h, but it doesn't look like it supports durations. > > +1. This seems like something that we ought to have a common helper function > for, maybe using TimeUnitAmount? Do we really need to use "2h 3m" instead of '2 hrs and 3 mins'? If '2 hrs and 3 mins' is ok (for space), you can just use ui::TimeFormat::Detailed(FORMAT_DURATION, LENGTH_SHORT, ...). They'll take care of localized digits and symbols. We can't use MessageFormatter here because there are two numbers that can be subject to plural rules (see go/plural internally at Google; nested plurals are not supported to avoid combinational explosion. ICU supports it, but it's too much burden to translators to handle all the possible combinations). Alternatively, one can use ICU MeasureFormat with UMEASFMT_WIDTH_NARROW. http://icu-project.org/apiref/icu4c/measfmt_8h.html#details I have to check if it gives 'h' and 'm'. It might use apostrophe and double quotation mark (or their better-shaped equivalents). Ok. I confirmed that CLDR/ICU uses 'h' and 'm' in English locale and equivalents in other locales (I spot-checked a few). icu::ErrorCode status = U_SUCCESS; icu::Measure measures[2] = { icu::Measure(Formattable(hours), icu::MeasureUnit::createHour(status), status), icu::Measure(Formattable(minutes), icu::MeasureUnit::createMinute(status), status)}; icu::MeasureFormat measure_format(icu::Locale::getDefault(), UMEASFMT_WIDTH_NARROW, status); icu::UnicodeString formatted; icu::FieldPosition ignore(icu::FieldPosition::DONT_CARE); measure_format.formatMeasures(&measures, minutes == 0 ? 1 : 2, formatted, ignore, status); base::string16 result(formatted.getBuffer(), formatted.length()); With that, IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL needs to be changed to take the formatted time ( 3h 45m). ICU may cache measure format internally (I have to check). If not, you may as well consider caching |measure_format|. https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:95: base::FormatNumber(status.GetRoundedBatteryPercent())); 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 https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); On 2016/05/03 19:49:09, stevenjb wrote: > On 2016/05/03 19:17:33, Daniel Erat wrote: > > 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? > > 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. 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); https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:125: time_status_label_->SetText(battery_time_status); 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).
https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:95: base::FormatNumber(status.GetRoundedBatteryPercent())); On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > 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); Oops. The 1st argument should be |base::ASCIIToUTF16("{1,number,percent}")| > See an example at https://codereview.chromium.org/1958073003
Here's an intermediate patch with just power_status_view.cc. I have a few questions I wanted to put out there while I'm working on battery_notification.cc. Please have another look! https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:95: base::FormatNumber(status.GetRoundedBatteryPercent())); On 2016/05/10 09:28:17, jshin (jungshik at google) wrote: > On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > > 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); > > Oops. The 1st argument should be |base::ASCIIToUTF16("{1,number,percent}")| > > > See an example at https://codereview.chromium.org/1958073003 > Done. https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > On 2016/05/03 19:49:09, stevenjb wrote: > > On 2016/05/03 19:17:33, Daniel Erat wrote: > > > 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? > > > > 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. > > 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); > 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? https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:125: time_status_label_->SetText(battery_time_status); On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > 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). 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.
Ok, here's a working draft. Please have another look! https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/b... ash/system/chromeos/power/battery_notification.cc:67: IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL, base::FormatNumber(hour), On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > On 2016/05/03 19:49:09, stevenjb wrote: > > On 2016/05/03 19:17:33, Daniel Erat wrote: > > > just to check, is some combination of hours and minutes a reasonable > > assumption > > > to make for internationally-formatting a duration? there's > > > base/i18n/time_formatting.h, but it doesn't look like it supports durations. > > > > +1. This seems like something that we ought to have a common helper function > > for, maybe using TimeUnitAmount? > > Do we really need to use "2h 3m" instead of '2 hrs and 3 mins'? If '2 hrs and > 3 mins' is ok (for space), > you can just use ui::TimeFormat::Detailed(FORMAT_DURATION, LENGTH_SHORT, ...). > They'll take care of localized digits and symbols. > > We can't use MessageFormatter here because there are two numbers that can be > subject to plural rules (see go/plural internally at Google; nested plurals are > not supported to avoid combinational explosion. ICU supports it, but it's too > much burden to translators to handle all the possible combinations). > > > Alternatively, one can use ICU MeasureFormat with UMEASFMT_WIDTH_NARROW. > http://icu-project.org/apiref/icu4c/measfmt_8h.html#details > I have to check if it gives 'h' and 'm'. It might use apostrophe and double > quotation mark (or their better-shaped equivalents). Ok. I confirmed that > CLDR/ICU uses 'h' and 'm' in English locale and equivalents in other locales (I > spot-checked a few). > > icu::ErrorCode status = U_SUCCESS; > icu::Measure measures[2] = { > icu::Measure(Formattable(hours), icu::MeasureUnit::createHour(status), > status), > icu::Measure(Formattable(minutes), icu::MeasureUnit::createMinute(status), > status)}; > icu::MeasureFormat measure_format(icu::Locale::getDefault(), > UMEASFMT_WIDTH_NARROW, status); > icu::UnicodeString formatted; > icu::FieldPosition ignore(icu::FieldPosition::DONT_CARE); > measure_format.formatMeasures(&measures, minutes == 0 ? 1 : 2, formatted, > ignore, status); > base::string16 result(formatted.getBuffer(), formatted.length()); > > With that, IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL needs to be changed to > take the formatted time ( 3h 45m). > > ICU may cache measure format internally (I have to check). If not, you may as > well consider caching |measure_format|. Thanks for the code snippet... HUGE help! Got it working in TimeDurationFormat() with both UMEASFMT_WIDTH_NARROW and _NUMERIC (didn't test _WIDE and _SHORT, but assume those would work too). Did you look into caching |measure_format|? Is that something I should add? Do you have an example you could point me to? https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); On 2016/05/10 23:33:43, Greg Levin wrote: > On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > > On 2016/05/03 19:49:09, stevenjb wrote: > > > On 2016/05/03 19:17:33, Daniel Erat wrote: > > > > 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? > > > > > > 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. > > > > 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); > > > > 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? 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. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:161: // write "1:00", instead of "1h", when using NUMERIC width. Yeah, this is hacky. I'd rather have "1:00" than "1h" if possible. Apparently in Romanian (for example), "remaining" has singular and plural forms. "1:00" is arguably plural, which is how our string will be translated; "1h" is singular. But I'm not dead set on this, and open to other ideas.
On 2016/05/10 23:33:43, Greg Levin wrote: > Here's an intermediate patch with just power_status_view.cc. I have a few > questions I wanted to put out there while I'm working on > battery_notification.cc. > Please have another look! > > https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... > File ash/system/chromeos/power/power_status_view.cc (right): > > https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... > ash/system/chromeos/power/power_status_view.cc:95: > base::FormatNumber(status.GetRoundedBatteryPercent())); > On 2016/05/10 09:28:17, jshin (jungshik at google) wrote: > > On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > > > 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); > > > > Oops. The 1st argument should be |base::ASCIIToUTF16("{1,number,percent}")| > > > > > See an example at https://codereview.chromium.org/1958073003 > > > > Done. > > https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... > ash/system/chromeos/power/power_status_view.cc:111: : base::FormatNumber(min); > On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > > On 2016/05/03 19:49:09, stevenjb wrote: > > > On 2016/05/03 19:17:33, Daniel Erat wrote: > > > > 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? > > > > > > 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. > > > > 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); > > > > 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? How did you do the search? :-) All the strings that use MessageFormatter [1] *happen* to use plural/select so that they have 'ICU.Syntax' in the description. (that label is not necessary but is a helpful hint to Google's internal translation pipeline / linguists to alert them that they're dealing with plural/select format). This message does NOT use plural/select so that it's not "ICU Syntax" (as used in Google's translation pipeline). Anyway, I'm not sure what your concern is. "to keep the existing format strings, and use MessageFormatter to generate hour and minute strings separately" would work in a lot of cases, but why would you want to do that? It'd be more verbose. Aha... because of #2 below. > > 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? That's unfortunate. '0' (used for padding) is literally interpreted by ICU formatter instead of having a special meaning of localized digit '0'. I'll file a feature request against ICU for that. In the meantime, what you can do is : 1) Use FormatNumber to format minutes 2) Prepend the result for minutes with FormatNumber(0) if minutes < 10. 3) Pass the hour (integer) and the formatted minute string (obtained in step 2) with the following message to MessageFormatter. {<ph name="HOUR">{0, number, integer}<ex>5</ex></ph>:<ph name="MINUTE">{1}<ex>07</ex></ph> left If you change FormatNumber to support padding (that is, steps 1 and 2 can be done in that API instead of you doing it here), that'd be good, too. Or, you can just use MeasureFormat here, too. That would be cleaner. In that case, you may want to have a helper added to base/i18n because now there are more than one callers for that. And, repeating what I outlined for the other file here again seems a bit clunky. Yeah, I think that's the way to go ! > > https://codereview.chromium.org/1951493002/diff/1/ash/system/chromeos/power/p... > ash/system/chromeos/power/power_status_view.cc:125: > time_status_label_->SetText(battery_time_status); > On 2016/05/10 08:50:44, jshin (jungshik at google) wrote: > > 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). > > 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. Sure, go ahead. Thank you for finding that bug ! :-)
https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:112: : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT); Please use l10n_util::GetStringUTF16()
lgtm (but i am deferring to jungshik for the tricky parts of this :-P)
Thank you for writing a helper API. My previous comment was written without noticing PS #3. Anyway, below is my thought. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:161: // write "1:00", instead of "1h", when using NUMERIC width. On 2016/05/11 19:43:02, Greg Levin wrote: > Yeah, this is hacky. I'd rather have "1:00" than "1h" if possible. Apparently > in Romanian (for example), "remaining" has singular and plural forms. "1:00" is > arguably plural, which is how our string will be translated; "1h" is singular. > But I'm not dead set on this, and open to other ideas. A potential issue with plural is why I just left alone a not-so-pretty (but more likely to be correct in terms of plural) ui/base/l10n/time_format instead of converting to use measure format. You might as well consider extending that API to support 'numeric' (2:07 instead of 2hrs and 7 mins and other forms). https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:172: measure_format.formatMeasures(measures, minutes == 0 ? 1 : 2, formatted, If you always use '2' for the 2nd argument, do you still have a problem of getting '1h' instead of '1:00' ? I guess not. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.h:79: // "3:07". I guess it has to be made clear that it only supports 'hour and minutes'. And, it also supports widths other than numeric.
One more comment. :-) https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/battery_notification.cc:56: IDS_ASH_STATUS_TRAY_BATTERY_PERCENT, percentage); You can change IDS_ASH_...PRECENT from <ph name="percentage">$1<ex>56%</ex></ph> remaining to <ph name="percentage">{0,number,percent}<ex>56</ex></ph> remaining and use MessageFormatter.
Just in case it's not clear, overall, PS #3 looks good except for a potential plural+inflection (of 'left', which is tricky. It'd be interesting to know how '2:45' and '2:01' would change 'left' (equivalent of other languages) and might be rather hard to get right in all cases.
On 2016/05/11 21:33:15, jshin (jungshik at google) wrote: > Just in case it's not clear, overall, PS #3 looks good except for a potential > plural+inflection (of 'left', which is tricky. It'd be interesting to know how > '2:45' and '2:01' would change 'left' (equivalent of other languages) and might > be rather hard to get right in all cases. So, I'm fine with what PS #3 does at least for now.
> Just in case it's not clear, overall, PS #3 looks good except for a > potential plural+inflection (of 'left', which is tricky. It'd be interesting > to know how '2:45' and '2:01' would change 'left' (equivalent of other > languages) and might be rather hard to get right in all cases. The pluralization of "left" (and possibly "until full") is probably wrong. Here's a snippet from an email from the i18n team: Romanian NUMERIC: 0 % — 1 m rămas NUMERIC: 10 % — 1 h rămasă NUMERIC: 11 % — 1:05 rămasă NUMERIC: 50 % — 5 h rămase NUMERIC: 51 % — 5:05 rămase I'm going to file a separate bug to deal with pluralization here, since this issue was more about numeric glyphs. Please have another look! https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:112: : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT); On 2016/05/11 20:32:39, jshin (jungshik at google) wrote: > Please use l10n_util::GetStringUTF16() I had already switched back to GetStringFUTF16() in PS#3. Should I convert the above instances of rb.GetLocalizedString() to GetStringUTF16()? What are the guidelines for using one vs the other? https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/battery_notification.cc (right): https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/battery_notification.cc:56: IDS_ASH_STATUS_TRAY_BATTERY_PERCENT, percentage); On 2016/05/11 21:19:49, jshin (jungshik at google) wrote: > You can change IDS_ASH_...PRECENT from > > <ph name="percentage">$1<ex>56%</ex></ph> remaining > > to > > <ph name="percentage">{0,number,percent}<ex>56</ex></ph> remaining > > and use MessageFormatter. Done. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:161: // write "1:00", instead of "1h", when using NUMERIC width. On 2016/05/11 21:11:45, jshin (jungshik at google) wrote: > On 2016/05/11 19:43:02, Greg Levin wrote: > > Yeah, this is hacky. I'd rather have "1:00" than "1h" if possible. > Apparently > > in Romanian (for example), "remaining" has singular and plural forms. "1:00" > is > > arguably plural, which is how our string will be translated; "1h" is singular. > > But I'm not dead set on this, and open to other ideas. > > A potential issue with plural is why I just left alone a not-so-pretty (but > more likely to be correct in terms of plural) ui/base/l10n/time_format instead > of converting to use measure format. You might as well consider extending that > API to support 'numeric' (2:07 instead of 2hrs and 7 mins and other forms). I looked at it awhile, and will give it a shot if you like, but I'm not convinced it fits in the current architecture: 1) It would only support [FORMAT_DURATION][LENGTH_NUMERIC], just as two-unit times are currently only supported in DURATION (to avoid confusing pluralization issues with "remaining" / "elapsed"?). 2) Formatter uses MessageFormat instead of MeasureFormat. I'm not that familiar with these classes, and how the interact / interchange. I don't know if MessageFormat would do what I currently have MeasureFormat doing, and/or how I would easily rewrite Formatter to use both. One easier option would be to add a third function to TimeFormat: base::string16 TimeFormat::Numeric(const base::TimeDelta& delta); There would be no Format arg (and Length = LENGTH_NUMERIC is built in). It wouldn't deal with days at all (just large hour counts if needed), and output H:MM:SS if SS != 0. I'd mostly just copy my existing TimeDurationFormat() into TimeFormat::Numeric(), and not use the Formatter class at all. Let me know if that makes more sense, API-wise, and I'll move it. Except... it would need the DurationFormatWidth arg, because battery_notification.cc needs the DURATION_WIDTH_NARROW ("3h 7m") format. So maybe call it Duration() instead? But at that point, we would have Duration(DURATION_WIDTH_WIDE, delta) -> "3 hours, 7 minutes" Detailed(FORMAT_DURATION, LENGTH_LONG, -1, delta) -> "3 hours and 7 minutes" which is weird. I could just expose only the NARROW and NUMERIC options. I dunno... what do you think? I'm inclined to leave as is if you don't have a strong opinion. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:172: measure_format.formatMeasures(measures, minutes == 0 ? 1 : 2, formatted, On 2016/05/11 21:11:45, jshin (jungshik at google) wrote: > If you always use '2' for the 2nd argument, do you still have a problem of > getting '1h' instead of '1:00' ? I guess not. <head desk> S'what I get for not reading carefully the code I copy-and-paste. Fixed. https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.h:79: // "3:07". On 2016/05/11 21:11:45, jshin (jungshik at google) wrote: > I guess it has to be made clear that it only supports 'hour and minutes'. And, > it also supports widths other than numeric. Done.
https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:223: #endif I just copied this from other tests in the file. I don't know if it's needed here too.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951493002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks a lot. LGTM with a few minor issues noted below addressed. https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:112: : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT); On 2016/05/12 19:31:43, Greg Levin wrote: > On 2016/05/11 20:32:39, jshin (jungshik at google) wrote: > > Please use l10n_util::GetStringUTF16() > > I had already switched back to GetStringFUTF16() in PS#3. > > Should I convert the above instances of rb.GetLocalizedString() to > GetStringUTF16()? What are the guidelines for using one vs the other? Oh. I didn't realize that there are other uses of rb.Get...() in this file. Yes, please, switch over to l10n_util::Get...(). And, get rid of this line: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); l10n_util::GetStr*() takes care of it. A few cycles might be saved by getting a reference to shared resource bundle and use it for all other strings, but I don't think it's worth extra code lines. ;-) https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:18: #include "ui/base/l10n/time_format.h" This header is not necessary in this file, is it? (well, you didn't add it, but while you're at it, you can remove it :-)). https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/1951493002/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:161: // write "1:00", instead of "1h", when using NUMERIC width. On 2016/05/12 19:31:44, Greg Levin wrote: > On 2016/05/11 21:11:45, jshin (jungshik at google) wrote: > > On 2016/05/11 19:43:02, Greg Levin wrote: > > > Yeah, this is hacky. I'd rather have "1:00" than "1h" if possible. > > Apparently > > > in Romanian (for example), "remaining" has singular and plural forms. > "1:00" > > is > > > arguably plural, which is how our string will be translated; "1h" is > singular. > > > But I'm not dead set on this, and open to other ideas. > > > > A potential issue with plural is why I just left alone a not-so-pretty (but > > more likely to be correct in terms of plural) ui/base/l10n/time_format instead > > of converting to use measure format. You might as well consider extending > that > > API to support 'numeric' (2:07 instead of 2hrs and 7 mins and other forms). > > I looked at it awhile, and will give it a shot if you like, but I'm not > convinced it fits in the current architecture: > > 1) It would only support [FORMAT_DURATION][LENGTH_NUMERIC], just as two-unit > times are currently only supported in DURATION (to avoid confusing pluralization > issues with "remaining" / "elapsed"?). > > 2) Formatter uses MessageFormat instead of MeasureFormat. I'm not that familiar > with these classes, and how the interact / interchange. I don't know if > MessageFormat would do what I currently have MeasureFormat doing, and/or how I > would easily rewrite Formatter to use both. > > One easier option would be to add a third function to TimeFormat: > > base::string16 TimeFormat::Numeric(const base::TimeDelta& delta); > > There would be no Format arg (and Length = LENGTH_NUMERIC is built in). It > wouldn't deal with days at all (just large hour counts if needed), and output > H:MM:SS if SS != 0. I'd mostly just copy my existing TimeDurationFormat() into > TimeFormat::Numeric(), and not use the Formatter class at all. Let me know if > that makes more sense, API-wise, and I'll move it. > > Except... it would need the DurationFormatWidth arg, because > battery_notification.cc needs the DURATION_WIDTH_NARROW ("3h 7m") format. So > maybe call it Duration() instead? But at that point, we would have > > Duration(DURATION_WIDTH_WIDE, delta) -> > "3 hours, 7 minutes" > > Detailed(FORMAT_DURATION, LENGTH_LONG, -1, delta) -> > "3 hours and 7 minutes" > > which is weird. I could just expose only the NARROW and NUMERIC options. I > dunno... what do you think? I'm inclined to leave as is if you don't have a > strong opinion. > Yeah... let's just leave it as it is for now. ICU has a separate Duration Format (but it's deprecated IIRC), but it'd not help much (especially getting the inflection right around the duration ('left', 'until full; hope this one does not inflect differently in any language but well languages are interesting', 'remaining' etc) because basically translators wouldn't know what hour/minutes are in 'duration part' and cannot translate separately. Using plural support in MessageFormat is one way, but when it comes to 'numeric' forms, it's kinda odd, too (anyway, it's possible in theory as I suggested earlier). https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:175: i18n::SetICUDefaultLocale("en_US"); This test file does not use it elsewhere (it'd be nice of you if you add it in other tests in this file :-)), but it's a good idea to add the following before changing the locale. test::ScopedRestoreICUDefaultLocale restore_locale; That way, other tests won't be affected.
https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:175: i18n::SetICUDefaultLocale("en_US"); On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > This test file does not use it elsewhere (it'd be nice of you if you add it in > other tests in this file :-)), but it's a good idea to add the following before > changing the locale. > > test::ScopedRestoreICUDefaultLocale restore_locale; > > That way, other tests won't be affected. Ooops. I meant to add the above comment to a new test you're adding. BTW, other tests in the file already has the line (Scope...Locale). https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:223: #endif On 2016/05/13 14:54:08, Greg Levin wrote: > I just copied this from other tests in the file. I don't know if it's needed > here too. I'm trying to think of any reason this test can fail on Android. Android's ICU data file is trimmed more, but I don't recall dropping measure format related data. Why don't you try it before disabling it? Otherwise, you'll never know. :-)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951493002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
>> Just in case it's not clear, overall, PS #3 looks good except for a
>> potential plural+inflection (of 'left', which is tricky. It'd be interesting
>> to know how '2:45' and '2:01' would change 'left' (equivalent of other
>> languages) and might be rather hard to get right in all cases.
> The pluralization of "left" (and possibly "until full") is probably wrong.
> Here's a snippet from an email from the i18n team:
> Romanian
> NUMERIC: 0 % — 1 m rămas
> NUMERIC: 10 % — 1 h rămasă
> NUMERIC: 11 % — 1:05 rămasă
> NUMERIC: 50 % — 5 h rămase
> NUMERIC: 51 % — 5:05 rămase
Without triggering a combinatorial explosion (involving two plurals), it's
rather hard to get everything right. In this case, perhaps the only sane way is
to resort to a 'tabular' format. That is,
time left: 3:05
time remaining: 2:45
time until full: 35m
time until full: 2h 35m , 2:35, etc
Depending on languages, it'll be longer or shorter than '3:47 until full'.
Although there's no guarantee that translators pay attention to the message
description, we can include detailed instructions to let translators decide
which form to take,
'3:47 until full' vs 'time until full: 3:47'. For languages with little or no
inflection, a more 'friendly' (and potentially shorter) form ( '3:47 until full'
) can be taken. For languages with inflection affecting 'until full, remaining,
left, etc', they can choose a tabular form.
Mac OS X is using '3:45 until full' and '1:15 remaining' in English. It'll be
interesting to see how well they deal with highly inflecting languages.
A not-so-clean alternative (not guaranteed to work in all languages because
there may be languages where 'hour' may be read after 'minutes' for English
'3:04 left/until full' ) is to use a plural rule applied only to minutes.
Something like this can be tried, but it does not take care of zero-padding ('#'
will not be zero-padded for a single digit).
{1, plural,
=1 { <ph name="HOUR"><ex>3</ex>{0, number, integer}</ph>:01 left}
=other {<ph name="HOUR"><ex>3</ex>{0, number, integer}</ph>:# left}
}
Thanks again. LGTM again. https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:175: i18n::SetICUDefaultLocale("en_US"); On 2016/05/13 23:06:15, jshin (jungshik at google) wrote: > On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > > This test file does not use it elsewhere (it'd be nice of you if you add it in > > other tests in this file :-)), but it's a good idea to add the following > before > > changing the locale. > > > > test::ScopedRestoreICUDefaultLocale restore_locale; > > > > That way, other tests won't be affected. > > Ooops. I meant to add the above comment to a new test you're adding. > > BTW, other tests in the file already has the line (Scope...Locale). Sorry. You did use Scoped...Locale in your test from the beginning (a few lines above when you first changed the ICU locale). I just missed it. https://codereview.chromium.org/1951493002/diff/100001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/100001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:224: TEST(TimeFormattingTest, MAYBE_TimeDurationFormat) { This Android test failure is very interesting. I have to find out what's going on ( I'll start with double-checking what ICU data for Android contains.). Anyway, for your CL, this is OK. I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Value of: TimeDurationFormat(delta, DURATION_WIDTH_WIDE) I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Actual: 15 ঘন্টা, 42 মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Expected: bn_narrow I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Which is: ১৫ ঘন্টা এবং ৪২ মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) ../../base/i18n/time_formatting_unittest.cc:257: Failure I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Value of: TimeDurationFormat(delta, DURATION_WIDTH_SHORT) I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Actual: 15 ঘন্টা, 42 মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Expected: bn_narrow I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Which is: ১৫ ঘন্টা এবং ৪২ মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) ../../base/i18n/time_formatting_unittest.cc:258: Failure I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Value of: TimeDurationFormat(delta, DURATION_WIDTH_NARROW) I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Actual: 15 ঘন্টা, 42 মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Expected: bn_narrow I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Which is: ১৫ ঘন্টা এবং ৪২ মিনিট I 31.729s run_tests_on_device(06aa1e1f003b6aa7) ../../base/i18n/time_formatting_unittest.cc:259: Failure I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Value of: TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC) I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Actual: 15:42 I 31.729s run_tests_on_device(06aa1e1f003b6aa7) Expected: bn_numeric I 31.730s run_tests_on_device(06aa1e1f003b6aa7) Which is: ১৫:৪২
Description was changed from ========== Fix i18n number formats in tray power displays BUG=475351 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. ========== to ========== Fix i18n number formats in tray power displays BUG=612181 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. ==========
https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:112: : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT); On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > On 2016/05/12 19:31:43, Greg Levin wrote: > > On 2016/05/11 20:32:39, jshin (jungshik at google) wrote: > > > Please use l10n_util::GetStringUTF16() > > > > I had already switched back to GetStringFUTF16() in PS#3. > > > > Should I convert the above instances of rb.GetLocalizedString() to > > GetStringUTF16()? What are the guidelines for using one vs the other? > > Oh. I didn't realize that there are other uses of rb.Get...() in this file. Yes, > please, switch over to l10n_util::Get...(). And, get rid of this line: > ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); > > l10n_util::GetStr*() takes care of it. A few cycles might be saved by getting a > reference to shared resource bundle and use it for all other > strings, but I don't think it's worth extra code lines. ;-) Done. https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/power_status_view.cc (right): https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/power_status_view.cc:18: #include "ui/base/l10n/time_format.h" On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > This header is not necessary in this file, is it? (well, you didn't add it, but > while you're at it, you can remove it :-)). Done. https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting_unittest.cc:223: #endif On 2016/05/13 23:06:15, jshin (jungshik at google) wrote: > On 2016/05/13 14:54:08, Greg Levin wrote: > > I just copied this from other tests in the file. I don't know if it's needed > > here too. > > I'm trying to think of any reason this test can fail on Android. Android's ICU > data file is trimmed more, but I don't recall dropping measure format related > data. Why don't you try it before disabling it? Otherwise, you'll never know. > :-) Apparently my typo in the last patch was one step ahead of you :-) It inadvertently confirmed that the test does, in fact, fail on Android.: ../../base/i18n/time_formatting_unittest.cc:259: Failure Value of: TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC) Actual: 15:42 Expected: bn_numeric Which is: ১৫:৪২ For some reason, the numerals are rendered in Arabic, rather than in their Bengalese equivalents.
On 2016/05/16 17:12:56, Greg Levin wrote: > https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... > File ash/system/chromeos/power/power_status_view.cc (right): > > https://codereview.chromium.org/1951493002/diff/20001/ash/system/chromeos/pow... > ash/system/chromeos/power/power_status_view.cc:112: : > IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT); > On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > > On 2016/05/12 19:31:43, Greg Levin wrote: > > > On 2016/05/11 20:32:39, jshin (jungshik at google) wrote: > > > > Please use l10n_util::GetStringUTF16() > > > > > > I had already switched back to GetStringFUTF16() in PS#3. > > > > > > Should I convert the above instances of rb.GetLocalizedString() to > > > GetStringUTF16()? What are the guidelines for using one vs the other? > > > > Oh. I didn't realize that there are other uses of rb.Get...() in this file. > Yes, > > please, switch over to l10n_util::Get...(). And, get rid of this line: > > ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); > > > > l10n_util::GetStr*() takes care of it. A few cycles might be saved by getting > a > > reference to shared resource bundle and use it for all other > > strings, but I don't think it's worth extra code lines. ;-) > > Done. > > https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... > File ash/system/chromeos/power/power_status_view.cc (right): > > https://codereview.chromium.org/1951493002/diff/40001/ash/system/chromeos/pow... > ash/system/chromeos/power/power_status_view.cc:18: #include > "ui/base/l10n/time_format.h" > On 2016/05/13 23:00:35, jshin (jungshik at google) wrote: > > This header is not necessary in this file, is it? (well, you didn't add it, > but > > while you're at it, you can remove it :-)). > > Done. > > https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... > File base/i18n/time_formatting_unittest.cc (right): > > https://codereview.chromium.org/1951493002/diff/80001/base/i18n/time_formatti... > base/i18n/time_formatting_unittest.cc:223: #endif > On 2016/05/13 23:06:15, jshin (jungshik at google) wrote: > > On 2016/05/13 14:54:08, Greg Levin wrote: > > > I just copied this from other tests in the file. I don't know if it's > needed > > > here too. > > > > I'm trying to think of any reason this test can fail on Android. Android's ICU > > data file is trimmed more, but I don't recall dropping measure format related > > data. Why don't you try it before disabling it? Otherwise, you'll never > know. > > :-) > > Apparently my typo in the last patch was one step ahead of you :-) > It inadvertently confirmed that the test does, in fact, fail on Android.: > > ../../base/i18n/time_formatting_unittest.cc:259: Failure > Value of: TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC) > Actual: 15:42 > Expected: bn_numeric > Which is: ১৫:৪২ > > For some reason, the numerals are rendered in Arabic, rather than in their > Bengalese equivalents. After my last comment, I realized what's going on. Chrome on Android is NOT localized to Bengali and several Indian languages (it's only localized to Hindi among languages in India). To save space, I dropped Bengali locale data from the Android ICU data file. That's why the above test fails on Android. Given the importance of India and mobile, I think it's time to revisit the list of languages to which Chrome on Android is localized. I'll file a separate bug on this. In the meantime, you can add a comment as to why this is disabled on Android.
> After my last comment, I realized what's going on. Chrome on Android is NOT > localized to Bengali and several Indian languages (it's only > localized to Hindi among languages in India). To save space, I dropped Bengali > locale data from the Android ICU data file. That's why the above test fails on > Android. > > Given the importance of India and mobile, I think it's time to revisit the list > of languages to which Chrome on Android is localized. I'll file a separate bug > on this. > > In the meantime, you can add a comment as to why this is disabled on Android. Would you like me to switch the test to Persian? Is that included on Android? It also uses non-Arabic numbers, so would fill the same roll. (Why are the other tests in this file excluded on Android, btw?)
On 2016/05/17 12:56:34, Greg Levin wrote: > > After my last comment, I realized what's going on. Chrome on Android is NOT > > localized to Bengali and several Indian languages (it's only > > localized to Hindi among languages in India). To save space, I dropped Bengali > > locale data from the Android ICU data file. That's why the above test fails on > > Android. > > > > Given the importance of India and mobile, I think it's time to revisit the > list > > of languages to which Chrome on Android is localized. I'll file a separate bug > > on this. > > > > In the meantime, you can add a comment as to why this is disabled on Android. > > Would you like me to switch the test to Persian? Is that included on Android? > It also uses non-Arabic numbers, so would fill the same roll. Yup. That should work. Farsi data is included on Android. Excluded from Chrome on Android are [bn et gu kn ml mr ms ta te]. > (Why are the other tests in this file excluded on Android, btw?) I don't remember. For other tests (en-GB, etc), they should work unless Android's Java API is for TimeFormatter, but we're not. Those tests should be enabled and see if they pass on Android. Let me make a quick CL to enable them and see.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951493002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951493002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
lgtm
The CQ bit was checked by glevin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, jshin@chromium.org Link to the patchset: https://codereview.chromium.org/1951493002/#ps120001 (title: "Switch unit test from Bengali to Persian for Android (and merge)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951493002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951493002/120001
Message was sent while issue was closed.
Description was changed from ========== Fix i18n number formats in tray power displays BUG=612181 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. ========== to ========== Fix i18n number formats in tray power displays BUG=612181 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix i18n number formats in tray power displays BUG=612181 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. ========== to ========== Fix i18n number formats in tray power displays BUG=612181 TEST=Switch language to Bengali (India), open systray menu, notice that power values (% and time remaining) are displayed using Bengali, rather than Arabic, numberic characters. Committed: https://crrev.com/5cff4d12a49e6164f4c12f0179cacf46a20d9fa4 Cr-Commit-Position: refs/heads/master@{#394830} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5cff4d12a49e6164f4c12f0179cacf46a20d9fa4 Cr-Commit-Position: refs/heads/master@{#394830} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
