Open Bug 974259 Opened 7 years ago Updated 8 months ago

Add helper functions for common operations to PluralForm for convenience in pluralization

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: manishearth, Unassigned, Mentored)

Details

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

Attachments

(5 files, 1 obsolete file)

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
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
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.
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
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
Whiteboard: [good first bug][lang=js][mentor=manishearth] → [good first bug][lang=js]
Status: ASSIGNED → NEW
hey, 
 i would like work on this bug. can you please assign this to me ??
You already have one other bug assigned to you -- start work on this first :)
Hello there!
New contributor, would love to work on this bug. Can it please be assigned to me?
(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.
(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
Attached image Mentor IRC chat
Tackling a different 'good first bug" - will return to this if it is still available.
Assignee: jencazares → nobody
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.
Hi!
I am a newbie here. Can I work on this?
Assignee: nobody → pp2105
Status: NEW → ASSIGNED
Attached patch Bug974259.patch (obsolete) — Splinter Review
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
How do I find all the files where to apply this function?
No activity for nearly two years, taking (and broadening the scope a little).
Assignee: pp2105 → moz-ian
Summary: Consolidate PluralForm and GetStringFromName into a single function for convenience in pluralization → Add helper functions for common operations to PluralForm for convenience in pluralization
Comment on attachment 8916800 [details]
Bug 974259 - Add helper functions to PluralForm.jsm for common tasks.

Hey Manishearth, looking for some feedback on this approach.

Some notes/thoughts:
There are two types of bundle in the Fx frontend, <xul:stringbundles>, with a getString() method, and nsIStringBundles, with the previously mentioned GetStringFromName(), so my method takes both.

Very frequently .get(num, words) is immediately or soon after chained with .replace("#1", num), so I added a function for that, and another for the multi-replace case.
There are a few places in the code where the usual #1 hasn't been used, so they can't switch, at least not without string changes/renaming, which seems unnecessarily disruptive to translators. (specifically, a couple used "#S", and one used the getFormattedString() placeholder "%1$S")
I'm slightly curious why #1 exists separately from %1$S in the first place.  Maybe for brevity?  Is it okay to codify this separation?

It occurs to me that getReplaceAll() could be combined with getReplace() by having the array as an optional argument and branching on its presence, but I don't know which would be preferred style for Fx code.
(getReplaceAll just copies/moves code from PlacesUIUtils.getPluralString() https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/browser/components/places/PlacesUIUtils.jsm#296)

Would probably be good to get some feedback from a few Fx frontend engineers.
Attachment #8916800 - Flags: feedback?(manishearth)
Attachment #8702146 - Attachment is obsolete: true
I'm sorry, I'm a little busy right now, and I also haven't touched firefox frontend code in years. Looking quickly at your code and comment, it all looks okay, but I really can't comment on the preferred style/etc.
Attachment #8916800 - Flags: feedback?(manishearth)
I'd like for either stas or gandalf to have a look at this to see how the proposed APIs lend themselves to a rewrite into fluent in the near future.
Needinfo for comment #27.
Flags: needinfo?(stas)
Flags: needinfo?(gandalf)
We have fluent in m-c already (Fx 58 is shipping the first string!) and we're on the manual landing path. My hope is to get first-migration milestone (bug 1407256) in 59, and Preferences (bug 1415730) in 59/60.

Once this is completed we should be ready to open up a little and let people start using Fluent in more places around the code and migrate existing code to it.

That puts this bug in a weird place. If you think it gives front-end an immediate value and is not adding too much complexity (I'm not familiar with StringBundle enough to evaluate myself), then I'd say go for it.

If you're ok waiting till Fx 60/61 when we migrate to Fluent and get all the benefits of Intl.PluralRules, then let's do that.
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #29)
> That puts this bug in a weird place. If you think it gives front-end an
> immediate value and is not adding too much complexity (I'm not familiar with
> StringBundle enough to evaluate myself), then I'd say go for it.
> 
> If you're ok waiting till Fx 60/61 when we migrate to Fluent and get all the
> benefits of Intl.PluralRules, then let's do that.

I doubt strings are added/changed enough for this to provide much immediate value so it can wait.
Assignee: moz-ian → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(stas)
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
You need to log in before you can comment on or make changes to this bug.