862752 - [i18n] wrong pluralization for alarm offsets

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Fabien Cazenave [:kaze]]

I just saw these strings in the apps/calendar/locales/calendar.en-US.properties file:

  alarm-minute-before=1 minute before
  alarm-minutes-before={{value}} minutes before
  alarm-hour-before=1 hour before
  alarm-hours-before={{value}} hours before
  alarm-day-before=1 day before
  alarm-days-before={{value}} days before
  alarm-week-before=1 week before
  alarm-weeks-before={{value}} weeks before
  alarm-minute-after=1 minute after
  alarm-minutes-after={{value}} minutes after
  alarm-hour-after=1 hour after
  alarm-hours-after={{value}} hours after
  alarm-day-after=1 day after
  alarm-days-after={{value}} days after
  alarm-week-after=1 week after
  alarm-weeks-after={{value}} weeks after

This won’t work for a few languages (e.g. Arabic, Polish…) that have more complex pluralization rules. We should rely on our built-in pluralization rule, e.g.:

  alarm-min-before = plural(value)
  alarm-min-before[one]   = 1 minute before
  alarm-min-before[other] = {{value}} minutes before

or even this, to meet the back-end requirements of the Mozilla l10n repositories:

  alarm-min-before = plural(value)
  alarm-min-before[one]   = 1 minute before
  alarm-min-before[two]   = {{value}} minutes before
  alarm-min-before[few]   = {{value}} minutes before
  alarm-min-before[many]  = {{value}} minutes before
  alarm-min-before[other] = {{value}} minutes before

An even better solution might be to rely on the “pretty dates” that `l10n_date.js' proposes — but I’m not sure it’d fit exactly the Calendar needs for this particular case.

Comment 1

[Fabien Cazenave [:kaze]]

Requesting leo+ because Polish is affected.

Comment 2

[Fabien Cazenave [:kaze]]

FTR, this has been introduced by bug 848378 and bug 849908.

Comment 3

[Kevin Grandon :kgrandon]

I will first attempt modifying pretty date to support this use case, and if it's too crazy, we can just leave the duplicated logic in calendar.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Comment 5

[Fabien Cazenave [:kaze]]

My main concern with this patch is that it modifies the shared/locales/date l10n resources.

 • if we’re fine with that, let’s add strings (and code) to suppports weeks / months / years in the shared/locales/date resources and make l10n_date.js truly generic;

 • if we prefer to limit the risk to the Calendar app and if that’s feasible, let’s keep the shared/locales/date resources unchanged and limit the changes to apps/calendar/locales (which is necessary).

In doubt, I’d recommend the latter — but if Staś says it’s OK, the former would be nice to have.

Comment 6

[Staś Małolepszy :stas]

(In reply to Fabien Cazenave [:kaze] from comment #2)
> FTR, this has been introduced by bug 848378 and bug 849908.

Both of these bugs are leo+.  I take it that 1.0.1 is not affected at all?

(In reply to Fabien Cazenave [:kaze] from comment #5)
>  • if we’re fine with that, let’s add strings (and code) to suppports weeks
> / months / years in the shared/locales/date resources and make l10n_date.js
> truly generic;

If the above holds true, I agree with Kazé that this would be a nice-to-have solution on v1-train and master.

Comment 7

[Fabien Cazenave [:kaze]]

(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #6)
> (In reply to Fabien Cazenave [:kaze] from comment #2)
> > FTR, this has been introduced by bug 848378 and bug 849908.
> Both of these bugs are leo+.  I take it that 1.0.1 is not affected at all?

Correct.

> (In reply to Fabien Cazenave [:kaze] from comment #5)
> >  • if we’re fine with that, let’s add strings (and code) to suppports weeks
> > / months / years in the shared/locales/date resources and make l10n_date.js
> > truly generic;
> 
> If the above holds true, I agree with Kazé that this would be a nice-to-have
> solution on v1-train and master.

Nice! Kevin, let’s do that if that’s OK with you.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 738578 [details]
Github pull request pointer

Thanks for the information guys. I've moved the calendar strings into the shared locale files. I believe this is what you were looking for.

Comment 9

[Fabien Cazenave [:kaze]]

Well, not exactly. Sorry for my poor communication skills. :-(

The idea was to make the l10n_date.js lib more generic (which you did) and expose the `.relativeParts()' method, so it can be used in the Calendar app and all other apps that want a specific “relative” date formatting — like the Calendar app.

As these “offset date” string formats are specific to the Calendar app, I’d prefer the corresponding l10n resources to live in the Calendar rather than in /shared/locales/

Comment 10

[Kevin Grandon :kgrandon]

Ok, that makes sense. I've updated the PR and moved the strings into the calendar app. If/when we need these elsewhere we can move them into shared.

I've also added a simple unit test for l10n_date.js. (In the gallery app for now because that's where most tests for shared code is)

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 738578 [details]
Github pull request pointer

Kaze - PR has been updated to address nits. Also adding james for review in case he wants to take a look at the PR. Thanks!

Comment 12

[Fabien Cazenave [:kaze]]

Comment on attachment 738578 [details]
Github pull request pointer

That’s a very good refactoring, I love it. Thanks Kevin!
r=me for the shared/js and shared/locales part.

Comment 13

[James Lal [:lightsofapollo]]

Still some bug in the calendar side I see things like "Go home started undefined" when setting a near event. ( 2 minutes in the future )

Comment 14

[Kevin Grandon :kgrandon]

(In reply to James Lal [:lightsofapollo] from comment #13)
> Still some bug in the calendar side I see things like "Go home started
> undefined" when setting a near event. ( 2 minutes in the future )

Unable to reproduce this currently, both with events starting soon, and in the past. *Continues to look*

Comment 15

[Kevin Grandon :kgrandon]

I'm able to see it now with events starting < 1 minute in the future.

Comment 16

[Kevin Grandon :kgrandon]

Ok - the condition was present when the event started < 1 minute in the future or past. I've fixed the code to always return at least a 'minutes' key, and updated the unit test to cover this.

Comment 17

[James Lal [:lightsofapollo]]

Comment on attachment 738578 [details]
Github pull request pointer

nice work! thanks for fixing the less then 1 minute case...

Comment 18

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/913177e7a8c49cd6f1ebcc5843ec3f2df8901e68

Comment 19

[James Lal [:lightsofapollo]]

Uplifted 913177e7a8c49cd6f1ebcc5843ec3f2df8901e68 to:
v1-train: c7c3a422b0cf6926276711c2a61f1b1ebf742dc3

Comment 20

[John Hammink]

Removing NI.  I'll follow up to see if there is a moztrap testcase covering this.