If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use plurals for toolbox button tooltip

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Developer Tools: Graphic Commandline and Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Firefox 21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
See bug 788445 comment 35.

I'm going to submit a patch.
(Assignee)

Comment 1

5 years ago
Created attachment 706406 [details] [diff] [review]
proposed fix

Quick fix for the issue reported by Francesco.
Attachment #706406 - Flags: review?(paul)

Updated

5 years ago
Attachment #706406 - Flags: review?(paul) → review+
(Assignee)

Comment 2

5 years ago
Thanks!
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/1a0ac09a4049
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1a0ac09a4049
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
First of all I'm really sorry that this bug fell out of my radar.

I'm pretty sure that's not the right way to do this. Why did you strip the numbers from the strings? Usually it should be

toolboxToggleButton.errorsCount=#1 error;#1 errors

ccing Axel, because I think this is broken, for example for rtl languages

P.S. if these strings need to be fixed, you'll also need new key names
Oh wait

toolboxToggleButton.tooltiptext=%S %S, %S %S.\nClick to toggle the developer tools.

Is this something like "1 error, 2 warnings. \nClick to toggle the developer tools."?

For sure this string needs a proper localization comment, and I think you need ordered arguments if you're not going to change the other two strings (I still think that's a better solution).
(Assignee)

Comment 7

5 years ago
toolboxToggleButton.errorsCount=error;errors
toolboxToggleButton.warningsCount=warning;warnings
toolboxToggleButton.tooltiptext=%S %S, %S %S.\nClick to toggle the developer tools.

I've seen similar examples in MDN. Now you can change the strings as a localizer, as you see needed. How does it break RTL?

Also, ordered arguments are up to the localizer: he can switch in his own .properties to use %1$S and %2$S. Is there something preventing localizers from doing so?

Francesco: sorry for the trouble - I did what I believed to be fine. Is it ok for me to always ask you for reviews on l10n patches? To avoid such issues in the future.
(In reply to Mihai Sucan [:msucan] from comment #7)
> I've seen similar examples in MDN. Now you can change the strings as a
> localizer, as you see needed. How does it break RTL?

How does a RTL language write this kind of sentence? "1 error" or "error 1"? I'm Italian and unfortunately not proficient in any RTL language.

Peeking for example at Hebrew, I guess that's the second one
http://mxr.mozilla.org/l10n-central/source/he/browser/chrome/browser/syncSetup.properties#24

> Also, ordered arguments are up to the localizer: he can switch in his own
> .properties to use %1$S and %2$S. Is there something preventing localizers
> from doing so?

Good question. What happen to compare-locales if the original string has %S and the localized one %1$S? Not sure if that generates a warning (Axel can answer that for sure).

> Francesco: sorry for the trouble - I did what I believed to be fine. Is it
> ok for me to always ask you for reviews on l10n patches? To avoid such
> issues in the future.

You can CC me and hope I'll have time to check out the bug, not really a reviewer ;-)

Comment 9

5 years ago
Comment on attachment 706406 [details] [diff] [review]
proposed fix

Review of attachment 706406 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/devtools/toolbox.properties
@@ +6,5 @@
> +# that allows users to open/close the developer tools. You can find this button
> +# on the developer toolbar.
> +toolboxToggleButton.errorsCount=error;errors
> +toolboxToggleButton.warningsCount=warning;warnings
> +toolboxToggleButton.tooltiptext=%S %S, %S %S.\nClick to toggle the developer tools.

This is actually going to confuse localizers, as we have some strings where the number was not included at all.

The best way to implement this would be to do

# LOCALIZATION NOTE (toolboxToggle.errors): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 number of errors in the current page
toolboxToggle.errors = #1 error;#1 errors
# LOCALIZATION NOTE (toolboxToggle.warnings): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 number of warnings in the current page
toolboxToggle.warnings = #1 warning;#1 warnings
# LOCALIZATION NOTE (toolboxToggleButton.tooltip): This string is shown
# as tooltip in the developer toolbar to open/close the developer tools.
# It's using toolboxToggle.errors as first and toolboxToggle.warnings
# as second argument to show the number of errors and warnings.
toolboxToggleButton.tooltip = %1$S, %2$S\nClick to toggle developer tools.

That's:

Switching on plurals support in compare-locales for errors and warnings (that's keying off the plurals mdn link being in the comment before the entity, best hack I came up with so far), and uses numbered arguments with explanation who's who for the result.

Note, compare-locales can cope with ordered and non-ordered params allright, IIRC, it just needs to be one or the other, as supported by our printf impl. Also, I don't expect RTL problems here.
Attachment #706406 - Flags: feedback-
(Assignee)

Comment 10

5 years ago
(In reply to Axel Hecht [:Pike] from comment #9)
> Comment on attachment 706406 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 706406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/devtools/toolbox.properties
> @@ +6,5 @@
> > +# that allows users to open/close the developer tools. You can find this button
> > +# on the developer toolbar.
> > +toolboxToggleButton.errorsCount=error;errors
> > +toolboxToggleButton.warningsCount=warning;warnings
> > +toolboxToggleButton.tooltiptext=%S %S, %S %S.\nClick to toggle the developer tools.
> 
> This is actually going to confuse localizers, as we have some strings where
> the number was not included at all.
> 
> ...

Thanks for your feedback Axel!

It seems plural forms are hard to get right. Can you please update the MDN page to suggest the correct way of doing things? I just checked and there's an example with "download.properties" that shows something similar to what I did. Maybe you can add a section for developers like us who do not work with these things too often - a "check list", to avoid future confusion. Thanks!

I'm going to submit an updated patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
Created attachment 714564 [details] [diff] [review]
changes as suggested by axel

Axel: please confirm this patch is fine. Thanks!
Attachment #714564 - Flags: review?
Attachment #714564 - Flags: feedback?(l10n)
(Assignee)

Updated

5 years ago
Attachment #714564 - Flags: review? → review?(paul)

Comment 12

5 years ago
(I'll review that as soon we have a f+ from l10n)
Waiting for Axel, this looks good to me.

Only doubt: you lost a "." before the new line. Is that wanted?
(Assignee)

Comment 14

5 years ago
(In reply to Francesco Lodolo [:flod] from comment #13)
> Waiting for Axel, this looks good to me.
> 
> Only doubt: you lost a "." before the new line. Is that wanted?

It seems this was suggested by Axel, and it might make sense, looking at the tooltip. I have no strong feelings in either direction.

Comment 15

5 years ago
Comment on attachment 714564 [details] [diff] [review]
changes as suggested by axel

Review of attachment 714564 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to change the keys of all these entities to expose the changes to your localizers on central, but apart from that, this looks good.

I didn't intend to suggest anything about the '.' on the first line, I don't have an opinion either way myself.
Attachment #714564 - Flags: feedback?(l10n) → feedback-
(Assignee)

Comment 16

5 years ago
Comment on attachment 714564 [details] [diff] [review]
changes as suggested by axel

I did change the keys for all of the entities involved. Please double check or point me to the entity that wasn't renamed.

Thank you!
Attachment #714564 - Flags: feedback- → feedback?(l10n)

Comment 17

5 years ago
Comment on attachment 714564 [details] [diff] [review]
changes as suggested by axel

Review of attachment 714564 [details] [diff] [review]:
-----------------------------------------------------------------

oops, yeah, sorry.
Attachment #714564 - Flags: feedback?(l10n) → feedback+

Updated

5 years ago
Attachment #714564 - Flags: review?(paul) → review+
(Assignee)

Comment 18

5 years ago
Comment on attachment 714564 [details] [diff] [review]
changes as suggested by axel

marking which patch needs to land
Attachment #714564 - Flags: checkin?
(Assignee)

Comment 19

5 years ago
Thank you Axel, Paul and Francesco.
Whiteboard: [land-in-fx-team]
Attachment #706406 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/5d7a14c71f51
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Attachment #714564 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/5d7a14c71f51
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.