Consolidate PluralForm and GetStringFromName into a single function for convenience in pluralization

ASSIGNED
Assigned to

Status

()

Core
Internationalization
ASSIGNED
4 years ago
a year ago

People

(Reporter: manishearth, Assigned: Pradyot Prakash, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
There are a bunch of places[1][2], more buried in [3], where a string from a dtd or .properties file contains a `#1`, which is meant to be substituted later on (usually with a number).

In JavaScript these are fetched using GetStringFromName, and then replace()d on the spot.

Perhaps we should add an extra optional argument for substitution? Either a vararg (splat) that takes in a list of substitutions and substitutes them for `#1`,`#2`,`#3`, etc, or have an optional array argument.

This would make these replacements much easier and more compact.

 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js#900
 [2]: http://mxr.mozilla.org/mozilla-central/source/accessible/src/jsat/OutputGenerator.jsm#247
 [3]: http://mxr.mozilla.org/mozilla-central/search?string=%231&find=%5C.properties%24&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
(Reporter)

Comment 1

4 years ago
Looks like this already exists in the form of the formatStringForName.

However, that only works for strings using the %1,%2 syntax.


Strings using #1, etc are formatted using PluralForm.get() (example[1]), like this[2] one.

Maybe we should have an extra function under StringBundle or PluralForm that combines these two actions, something like

StringBundle.prototype.getPluralFromName = (name,number) -> PluralForm.get(number,this.GetStringFromName(name))

 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js#897
 [2]: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/syncSetup.properties#24
Summary: Give GetStringFromName an optional second argument → Consolidate PluralForm and GetStringFromName into a single function for convenience in pluralization
Component: General → Internationalization
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 2

4 years ago
Another way to do this is to take PluralForm and give it another method `PluralForm.formatPluralStringFromName(number,name,bundle)`. This puts the method in a logically better place, but we have to give it an extra argument.

The relevant PluralForm code is here[1]


Once this is done we can file a separate bug that uses this function everywhere.


(assigning and setting as good first bug as per IRC discusion)

 [1]: http://dxr.mozilla.org/mozilla-central/source/intl/locale/src/PluralForm.jsm
Assignee: nobody → mathnerd314...gph+mozilla
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js][mentor=manishearth]
I don't think I'll have time to work on this anytime soon. It's still a good first bug though.
Assignee: mathnerd314...gph+mozilla → nobody
Status: ASSIGNED → NEW
:manishearth I'd like to try out this bug.
(Reporter)

Comment 5

4 years ago
Alright, assigned.

We're going to go with making `PluralForm.formatPluralStringFromName(number,name,bundle)`, you can look at the mxr/dxr links above to get an idea of how the two functions are used and how we need to merge them.

Let me know (either here or in IRC) if you need help!
Assignee: nobody → ranveeraggarwal
Status: NEW → ASSIGNED
(Reporter)

Comment 6

3 years ago
Resetting since Ranveer is busy right now and others should have a chance to take this bug. Ranveer, I'll get you a different bug later :)
Assignee: ranveeraggarwal → nobody
Mentor: manishearth@gmail.com
Whiteboard: [good first bug][lang=js][mentor=manishearth] → [good first bug][lang=js]
Status: ASSIGNED → NEW

Comment 7

3 years ago
hey, 
 i would like work on this bug. can you please assign this to me ??
(Reporter)

Comment 8

3 years ago
You already have one other bug assigned to you -- start work on this first :)

Comment 9

3 years ago
Hello there!
New contributor, would love to work on this bug. Can it please be assigned to me?
(Reporter)

Comment 10

3 years ago
(In reply to Yennisaur from comment #9)
> Hello there!
> New contributor, would love to work on this bug. Can it please be assigned
> to me?

Sure, have you started work on it yet? Ping me here or on IRC if you would like some help understanding this.

Comment 11

3 years ago
(In reply to Manish Goregaokar [:manishearth] from comment #10)
> (In reply to Yennisaur from comment #9)
> > Hello there!
> > New contributor, would love to work on this bug. Can it please be assigned
> > to me?
> 
> Sure, have you started work on it yet? Ping me here or on IRC if you would
> like some help understanding this.

My current understanding is that a method needs to be developed to combine two functions with multiple variables, the variables would then have different outcomes based on numeric assignment. I've found the code and have seen the bug fixes mentioned earlier in this thread. I have yet to start working, as I am currently reviewing the code in its entirety. Am I on the right track?
Assigning to Yennisaur while working on it as part of the Ascend Project.  Yenni is actively working on a patch this week and next.
Assignee: nobody → jencazares

Comment 13

3 years ago
Created attachment 8504267 [details]
Mentor IRC chat

Comment 14

3 years ago
Tackling a different 'good first bug" - will return to this if it is still available.
Assignee: jencazares → nobody
(Reporter)

Comment 15

2 years ago
Updated links:

PluralForm.jsm: https://dxr.mozilla.org/mozilla-central/source/intl/locale/PluralForm.jsm

Example from comment 0: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js#889

Steps remain the same, see comment 2. Feel free to take the alternate route of stringBundle.getPluralStringFromName(...) mentioned in comment 1.
(Assignee)

Comment 16

2 years ago
Hi!
I am a newbie here. Can I work on this?
(Reporter)

Updated

2 years ago
Assignee: nobody → pp2105
Status: NEW → ASSIGNED
(Assignee)

Comment 17

2 years ago
Created attachment 8702146 [details] [diff] [review]
Bug974259.patch
(Reporter)

Comment 18

2 years ago
Comment on attachment 8702146 [details] [diff] [review]
Bug974259.patch

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

This does the main change and looks correct, however you also need to make this get used everywhere you can.

(in mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js, as well as any other files you find that use the pattern)

::: intl/locale/PluralForm.jsm
@@ +168,5 @@
>        GetStringFromName("pluralRule"));
> +  },
> +
> +  /**
> +   * Consolidated function with PluralForm 

nit: extra whitespace
(Assignee)

Comment 19

2 years ago
How do I find all the files where to apply this function?
You need to log in before you can comment on or make changes to this bug.