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

Status: RESOLVED INACTIVE

(Core :: Internationalization, defect)

Description

[Manish Goregaokar [:manishearth]]

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

Comment 1

[Manish Goregaokar [:manishearth]]

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

Comment 2

[Manish Goregaokar [:manishearth]]

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

Comment 3

[Allan Gardner (:Mathnerd314)]

I don't think I'll have time to work on this anytime soon. It's still a good first bug though.

Comment 4

[Ranveer Aggarwal [:ranWeird]]

:manishearth I'd like to try out this bug.

Comment 5

[Manish Goregaokar [:manishearth]]

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!

Comment 6

[Manish Goregaokar [:manishearth]]

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

Comment 7

[nithin[:lazybug]]

hey, 
 i would like work on this bug. can you please assign this to me ??

Comment 8

[Manish Goregaokar [:manishearth]]

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

Comment 9

[Yennisaur]

Hello there!
New contributor, would love to work on this bug. Can it please be assigned to me?

Comment 10

[Manish Goregaokar [:manishearth]]

(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

[Yennisaur]

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

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Yennisaur while working on it as part of the Ascend Project.  Yenni is actively working on a patch this week and next.

Comment 13

[Yennisaur]

Attachment: Mentor IRC chat

Comment 14

[Yennisaur]

Tackling a different 'good first bug" - will return to this if it is still available.

Comment 15

[Manish Goregaokar [:manishearth]]

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.

Comment 16

[Pradyot Prakash]

Hi!
I am a newbie here. Can I work on this?

Comment 17

[Pradyot Prakash]

Attachment: Bug974259.patch

Comment 18

[Manish Goregaokar [:manishearth]]

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

Comment 19

[Pradyot Prakash]

How do I find all the files where to apply this function?

Comment 20

[Ian Moody [:Kwan] (UTC+1)]

No activity for nearly two years, taking (and broadening the scope a little).

Comment 21

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 974259 - Add helper functions to PluralForm.jsm for common tasks.

PluralForm is commonly used with a string fetched from a stringbundle, and also
frequently has simple replace operations performed on the result, e.g.
PluralForm.get(num, words).replace("#1", num);
Add helper functions for these operations to simplify common use.

Review commit: https://reviewboard.mozilla.org/r/187864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187864/

Comment 22

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 974259 - Use new PluralForm helper functions in code.

Review commit: https://reviewboard.mozilla.org/r/187866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187866/

Comment 23

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 974259 - Add getReplace and getReplaceAll helper functions to devtools's PluralForm.

Review commit: https://reviewboard.mozilla.org/r/187868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187868/

Comment 24

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 974259 - Use the new PluralForm helpers in devtools code.

Review commit: https://reviewboard.mozilla.org/r/187870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187870/

Comment 25

[Ian Moody [:Kwan] (UTC+1)]

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.

Comment 26

[Manish Goregaokar [:manishearth]]

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.

Comment 27

[Axel Hecht [:Pike]]

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.

Comment 28

[Ian Moody [:Kwan] (UTC+1)]

Needinfo for comment #27.

Comment 29

[Zibi Braniecki [:zbraniecki][: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.

Comment 30

[Ian Moody [:Kwan] (UTC+1)]

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