1118226 - Allow to add number to string for removing selected cookies to improve localizability

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: number support and l10n comments, v1

From bu 1103314 comment 15:
> So I got notified that the plural form can't be used like this because some
> locale require a number in the string. Should I add the number to the English
> string ("Remove 5 Selected"), or keep the English one like it is and add a
> localization note that the localizers can add "#1" if they need the number of
> cookies selected? More background on this: https://groups.google.com/forum/#!msg/mozilla.dev.l10n/sN8OfMIVk5A/vw82p1-BvuAJ
>
> Furthermore, a localization node for the accesskey in the .dtd file with the
> label in the properties has been requested.

flod, your feedback please.

The number of selected cookies is always two if the user selects a cookie from a host in the unfiltered view and then the folder, so
https://hg.mozilla.org/mozilla-central/annotate/2a193b7f395c/browser/components/preferences/cookies.js#l553
should set
> selectedCookieCount += item.cookies.length;
instead of
> selectedCookieCount += 2;

This code is from the new preferences dialog in bug 274712 which seemed to support multiselect in the unfiltered view. This is not valid anymore, at least on Windows 8.1. Can anybody confirm this for all platforms? If this is a general issue, I can also fix that issue.

Comment 1

[Francesco Lodolo [:flod]]

Comment on attachment 8544577 [details] [diff] [review]
number support and l10n comments, v1

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

String-wise it's OK for me, note actually tested.

::: browser/locales/en-US/chrome/browser/preferences/cookies.dtd
@@ +7,5 @@
>  <!ENTITY     cookiesonsystem.label          "The following cookies are stored on your computer:">
>  <!ENTITY     cookiename.label               "Cookie Name">
>  <!ENTITY     cookiedomain.label             "Site"> 
> +<!-- LOCALIZATION NOTE (button.removeSelectedCookies.accesskey):
> +  The label associated with the this accesskey can be found in

stray 'the'

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +99,5 @@
> +# and it will be replaced with the number, e.g.
> +# removeSelectedCookied2=Remove #1 Selected Cookie;Remove #1 Selected Cookies
> +# would be shown as either "Remove 1 Selected Cookie" or e.g.
> +# "Remove 7 Selected Cookies".
> +removeSelectedCookies2=Remove Selected;Remove Selected

The new ID is not necessary but I agree that it doesn't hurt, especially to force people to read the note.

I would simplify it, e.g.

# If you need to display the number of selected elements in your language,
# you can use #1 in your localization as a placeholder for the number.
# For example this is the English string with numbers:   
# removeSelectedCookied2=Remove #1 Selected;Remove #1 Selected

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v2

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8544632 [details] [diff] [review]
patch, v2

Please don't change the string ID

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v3

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Dao, can also please advise the desired workflow for fixing the incorrect cookie count?

From comment 0:

The number of selected cookies is always two if the user selects a cookie from a host in the unfiltered view and then the folder, so
https://hg.mozilla.org/mozilla-central/annotate/2a193b7f395c/browser/components/preferences/cookies.js#l553
should set
> selectedCookieCount += item.cookies.length;
instead of
> selectedCookieCount += 2;

This code is from the new preferences dialog in bug 274712 which seemed to support multiselect in the unfiltered view. This is not valid anymore, at least on Windows 8.1. Can anybody confirm this for all platforms? If this is a general issue, I can also fix that issue.

Should I fix this
a) also in this bug,
b) in a separate bug and let it land in sync.
c) in a separate bug and don't care about a synchronized landing?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Archaeopteryx [:aryx] from comment #5)
> Should I fix this
> a) also in this bug,
> b) in a separate bug and let it land in sync.
> c) in a separate bug and don't care about a synchronized landing?

c :)

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v4, r=dao

Comment 8

[Carsten Book [:Tomcat]]

can we get a try run for this changes ? Thanks!

Comment 9

[Dão Gottwald [:dao]]

This doesn't need a try run. Apart from the line adding the .replace call, these are just comment changes.

https://hg.mozilla.org/integration/fx-team/rev/28fe6535d5fb

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28fe6535d5fb