1255542 - [markup panel] "Show All %S Nodes" (markupView.more.showAll) should use a proper plural form

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Safa Alfulaij]

The bug https://bugzilla.mozilla.org/show_bug.cgi?id=796006 introduced a new string that is "Show all %S nodes", but it's not marked as a plural string which makes translating it impossible and the translation will look bad.
See: http://developer.mozilla.org/en/docs/Localization_and_Plurals

Comment 1

[Francesco Lodolo [:flod]]

Confirming the issue (this is also probably a good candidate for a first bug).

Comment 2

[Michael Kohler [:mkohler]]

Attachment: 0001-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8730020 [details] [diff] [review]
0001-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

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

This looks good to me! Thanks.

::: devtools/client/locales/en-US/inspector.properties
@@ +47,5 @@
>  # When there are too many nodes to load at once, we will offer to
>  # show all the nodes.
>  # Keyboard shortcut for DOM and Style Inspector will shown inside brackets.
>  markupView.more.showing=Some nodes were hidden.
> +markupView.more.showAll=Show #1 node;Show All #1 nodes

I guess the #1 placeholder isn't even needed in the singular case, right?
So the string could be:

Show one more node;Show all #1 nodes

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8730020 [details] [diff] [review]
0001-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

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

::: devtools/client/locales/en-US/inspector.properties
@@ +47,5 @@
>  # When there are too many nodes to load at once, we will offer to
>  # show all the nodes.
>  # Keyboard shortcut for DOM and Style Inspector will shown inside brackets.
>  markupView.more.showing=Some nodes were hidden.
> +markupView.more.showAll=Show #1 node;Show All #1 nodes

Two issues.

1) The comment about the plural form needs to be right before the string (the parser it's quite hackish).

2) You need a new string ID, you can't change the existing string like that
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Example:
# LOCALIZATION NOTE (markupView.more.showAllNodes): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
markupView.more.showAllNodes=Show #1 node;Show All #1 nodes

I also wonder if the case is correct, it should either be "Show All X Nodes", or "Show all x nodes".

Comment 5

[Michael Kohler [:mkohler]]

Attachment: 0002-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8730907 [details] [diff] [review]
0002-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

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

Couple of nits on the comments but the properties file looks good.

::: devtools/client/locales/en-US/inspector.properties
@@ +48,3 @@
>  markupView.more.showing=Some nodes were hidden.
> +
> +# LOCALIZATION NOTE (markupView.more.showAllNodes): Semi-colon list of plural forms.

showAllNodes -> showAll2 since that's the string ID you used

@@ +51,5 @@
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +markupView.more.showAll2=Show one more node;Show all #1 nodes
> +
> +# LOCALIZATION NOTE (inspector.tooltip2)
> +# Keyboard shortcut for DOM and Style Inspector will shown inside brackets.

Either "will show" or "will be shown"

Comment 7

[Michael Kohler [:mkohler]]

Attachment: 0003-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8730912 [details] [diff] [review]
0003-Bug-1255542-markupView.more.showAll-should-use-a-pro.patch

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

Looks good, thanks!
And thank you Francesco for helping with the string changes!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/92f5f3373b71

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92f5f3373b71