Closed
Bug 894949
Opened 11 years ago
Closed 11 years ago
Defect - Plural form strings are excessively complex for both devs and translators
Categories
(Firefox for Metro Graveyard :: App Bar, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jwilde, Assigned: jwilde)
References
Details
(Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=3)
Attachments
(1 file, 3 obsolete files)
18.19 KB,
patch
|
mbrubeck
:
review+
flod
:
feedback+
|
Details | Diff | Splinter Review |
As Francesco Lodolo discussed in bug 867543, we need to add extra localization notes in order for localizers translate plural forms:
"Sadly some of our tools (compare-locales) and some external tools used for
localization rely on the presence of this comment to identify a string with
plural form.
# LOCALIZATION NOTE (STRINGID): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
The bad part is that his comment must be added before each string using a
plural form."
Updated•11 years ago
|
Blocks: metrov1defect&change
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=0
Assignee | ||
Comment 1•11 years ago
|
||
p=1
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=0 → feature=defect c=firefox_start u=metro_firefox_user p=1
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #777415 -
Flags: review?(mbrubeck)
Attachment #777415 -
Flags: feedback?(francesco.lodolo)
Comment 3•11 years ago
|
||
Comment on attachment 777415 [details] [diff] [review]
v1
Review of attachment 777415 [details] [diff] [review]:
-----------------------------------------------------------------
Based on bug 883951 comment 43, perhaps we should not be using PluralForm here at all.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Based on bug 883951 comment 43, perhaps we should not be using PluralForm
> here at all.
Sounds good. There's also some UI fugliness with trying to change the pluralization--the width of the buttons "jump" without animation.
Instead of the following:
- Pin top site(s)
- Unpin top site(s)
- Delete top site(s)
- Restore top site(s)
How about:
- Pin to Top Sites
- Unpin from Top Sites
- Delete
- Undo delete
This more closely matches the grammar choices of the Windows 8 start screen, too.
Yuan, does this work?
Flags: needinfo?(ywang)
Assignee | ||
Updated•11 years ago
|
Attachment #777415 -
Flags: review?(mbrubeck)
Attachment #777415 -
Flags: feedback?(francesco.lodolo)
Comment 5•11 years ago
|
||
I think you need to consider also this comment
https://bugzilla.mozilla.org/show_bug.cgi?id=883951#c43
Comment 6•11 years ago
|
||
By the way, some Unpin strings are seriously messed up.
contextAppbar.unpin.tab=Unpin page;Unpin tabs
contextAppbar.unpin.download=Unpin page;Unpin downloads
contextAppbar.unpin.download=Unpin page;Unpin downloads
Comment 7•11 years ago
|
||
(In reply to Jonathan Wilde [:jwilde] from comment #4)
> (In reply to Matt Brubeck (:mbrubeck) from comment #3)
> > Based on bug 883951 comment 43, perhaps we should not be using PluralForm
> > here at all.
>
> Sounds good. There's also some UI fugliness with trying to change the
> pluralization--the width of the buttons "jump" without animation.
>
> Instead of the following:
> - Pin top site(s)
> - Unpin top site(s)
> - Delete top site(s)
> - Restore top site(s)
>
> How about:
> - Pin to Top Sites
> - Unpin from Top Sites
> - Delete
> - Undo delete
>
> This more closely matches the grammar choices of the Windows 8 start screen,
> too.
>
> Yuan, does this work?
The new strings look fine with me. Thanks Jonathan!
Flags: needinfo?(ywang)
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #6)
> By the way, some Unpin strings are seriously messed up.
>
> contextAppbar.unpin.tab=Unpin page;Unpin tabs
> contextAppbar.unpin.download=Unpin page;Unpin downloads
> contextAppbar.unpin.download=Unpin page;Unpin downloads
Good catch! Thanks for sending comment 43 along, too. It looks like we're going to rework the strings to avoid plurals entirely as described above. The current set of strings is just too large and complicated, regardless of the plural logic issues.
If you wouldn't mind, I'll also feedback? you on the patch just to make sure everything checks out before landing.
Comment 9•11 years ago
|
||
(In reply to Jonathan Wilde [:jwilde] from comment #8)
> If you wouldn't mind, I'll also feedback? you on the patch just to make sure
> everything checks out before landing.
I'll be glad to check it.
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #777415 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Summary: Defect - Missing localization notes for plural form strings → Defect - Plural form strings are excessively complex for both devs and translators
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #778213 -
Attachment is obsolete: true
Attachment #778217 -
Flags: review?(mbrubeck)
Attachment #778217 -
Flags: feedback?(francesco.lodolo)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 778217 [details] [diff] [review]
patch v2.1
Review of attachment 778217 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +22,5 @@
>
> +<!-- START AND PANEL UI -->
> +
> +<!ENTITY topSitesHeader.label "Top Sites">
> +<!ENTITY recentHistoryHeader.label "Recent History">
We actually use these labels in a few places that aren't the Start screen. They're pretty generic headers for types of lists/grids of pages now, so renaming to reflect that.
Also, renaming "history" strings to "recentHistory" since I have a hunch we'll end up using "Recent History" for abridged lists on the Start screen and the like and "History" for full lists. This is going to be a bit down the road, but wanted to prep for that before we freeze strings.
Assignee | ||
Comment 13•11 years ago
|
||
Also, MarcoM: bumping this to p=3.
Whiteboard: feature=defect c=firefox_start u=metro_firefox_user p=1 → feature=defect c=firefox_start u=metro_firefox_user p=3
Comment 14•11 years ago
|
||
Comment on attachment 778217 [details] [diff] [review]
patch v2.1
Review of attachment 778217 [details] [diff] [review]:
-----------------------------------------------------------------
Strings look good to me.
::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +34,1 @@
> The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' -->
I'm wondering why not using a different character than ">" to make it more clear that it's not supposed to be localized.
Attachment #778217 -
Flags: feedback?(francesco.lodolo) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #14)
> Comment on attachment 778217 [details] [diff] [review]
> patch v2.1
>
> Review of attachment 778217 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Strings look good to me.
Thanks for the review! :D
> ::: browser/metro/locales/en-US/chrome/browser.dtd
> @@ +34,1 @@
> > The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' -->
>
> I'm wondering why not using a different character than ">" to make it more
> clear that it's not supposed to be localized.
Agreed. Or better yet, since the '>' is intended to represent an arrow pointing to more items, we should just move it into a separate element after the label containing the string and remove the '>' entirely... Our existing RTL logic would handle all of the locale issues automatically, too.
Comment 16•11 years ago
|
||
Comment on attachment 778217 [details] [diff] [review]
patch v2.1
Review of attachment 778217 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/metro/base/content/appbar.js
@@ +223,5 @@
> this.showContextualActions([]);
> },
>
> + _updateContextualActionLabel: function(aButton, aVerb, aSetName) {
> + Util.dumpLn("btn: " + aButton + " verb: " + aVerb + " set name: " + aSetName);
Please remove the debugging output before checkin.
::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +34,1 @@
> The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' -->
In response to flod's comment: Jonathan recently pointed out this Windows 8 symbol font, which has "progressive disclosure arrows" we could use for this purpose:
http://msdn.microsoft.com/en-us/library/windows/apps/jj841126.aspx
We'll try that in a separate bug.
::: browser/metro/locales/en-US/chrome/browser.properties
@@ +15,5 @@
> browser.search.contextTextSearchLabel2=Search %S for "%S"
>
> +# Contextual Appbar - Button Labels
> +
> +# LOCALIZATION NOTE (contextAppbar2.pin.*): "Pins" selected pages so they stay
I'm not sure how the .* will interact with l10n tooling -- flod or Pike, do you know?
Perhaps this should just be a "file-wide comment" with no () portion:
https://developer.mozilla.org/en-US/docs/Localization_notes
Attachment #778217 -
Flags: review?(mbrubeck) → review+
Comment 17•11 years ago
|
||
Smart, I missed the localization comments :-\
The only safe way to ensure compatibility with existing tools is to provide all the keys the comment is referred to.
Comment 18•11 years ago
|
||
The plan is to move the '>' out of the strings and likely into CSS - or some similar solution that will still meet right-to-left considerations.
That work will be a part of the responsive start page work (bug 892512) but may merits its own bug for tracking purposes.
Comment 19•11 years ago
|
||
We had the arrow because we were opening a new panel when clicking the labels. After the fix for bug 892046, they list of items will expand/shrink when you click the labels, so we'll probably change to a '+' or a down arrow. Best to remove from the string anyway.
Assignee | ||
Comment 20•11 years ago
|
||
In terms of plan of action moving forward with this:
- Let's have the '>' get moved out of the strings with bug 892512.
- I'll move the appbar2.pin.* style comments into a file-wide localization comment.
Assignee | ||
Comment 21•11 years ago
|
||
mbrubeck: Had to do some merging into the new snapped states. It passes all tests and seems to work, but wanted to pass it through review again to make sure I didn't mess anything up.
flod: Moved the comments for pin and unpin into a file-wide comment. Could you let me know if it makes sense?
Thanks!
Attachment #780635 -
Flags: review?(mbrubeck)
Attachment #780635 -
Flags: feedback?(francesco.lodolo)
Assignee | ||
Updated•11 years ago
|
Attachment #778217 -
Attachment is obsolete: true
Comment 22•11 years ago
|
||
Comment on attachment 780635 [details] [diff] [review]
patch v2.2
Review of attachment 780635 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/metro/base/content/appbar.js
@@ +223,5 @@
> this.showContextualActions([]);
> },
>
> + _updateContextualActionLabel: function(aButton, aVerb, aSetName) {
> + Util.dumpLn("btn: " + aButton + " verb: " + aVerb + " set name: " + aSetName);
Please remove the debug output before checkin.
Attachment #780635 -
Flags: review?(mbrubeck) → review+
Comment 23•11 years ago
|
||
Comment on attachment 780635 [details] [diff] [review]
patch v2.2
Review of attachment 780635 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me
Attachment #780635 -
Flags: feedback?(francesco.lodolo) → feedback+
Assignee | ||
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•