834721 - Use plurals for toolbox button tooltip

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Mihai Sucan [:msucan]]

See bug 788445 comment 35.

I'm going to submit a patch.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Quick fix for the issue reported by Francesco.

Comment 2

[Mihai Sucan [:msucan]]

Thanks!

Comment 3

[Mihai Sucan [:msucan]]

https://hg.mozilla.org/integration/fx-team/rev/1a0ac09a4049

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/1a0ac09a4049

Comment 5

[Francesco Lodolo [:flod]]

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

Comment 6

[Francesco Lodolo [:flod]]

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).

Comment 7

[Mihai Sucan [:msucan]]

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.

Comment 8

[Francesco Lodolo [:flod]]

(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

[Axel Hecht [:Pike]]

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.

Comment 10

[Mihai Sucan [:msucan]]

(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.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: changes as suggested by axel

Axel: please confirm this patch is fine. Thanks!

Comment 12

[Paul Rouget [:paul]]

(I'll review that as soon we have a f+ from l10n)

Comment 13

[Francesco Lodolo [:flod]]

Waiting for Axel, this looks good to me.

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

Comment 14

[Mihai Sucan [:msucan]]

(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

[Axel Hecht [:Pike]]

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.

Comment 16

[Mihai Sucan [:msucan]]

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!

Comment 17

[Axel Hecht [:Pike]]

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

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

oops, yeah, sorry.

Comment 18

[Mihai Sucan [:msucan]]

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

marking which patch needs to land

Comment 19

[Mihai Sucan [:msucan]]

Thank you Axel, Paul and Francesco.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5d7a14c71f51

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5d7a14c71f51