1440490 - Move l10n related functions into calL10NUtils.jsm

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This one is most tricky, as I've introduced a few more helper functions aside from the actual move. I've tried to automate as much as possible, but these patches will need actual review.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 1 - Manual Changes - v1

These are the initial changes, moving over the functions, some simple migrations where I thought it wouldn't make sense to create a separate patch, backwards compat.

I will remove calUtils.js in future changeset, I didn't want to bury it here.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 2 - Migrate some functions out of calUtils.jsm - v1

This migrates formatMonth/sortArrayByLocaleCollator/createLocaleCollator over to cal.l10n.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 3 - Migrate calendar.properties strings - v1

I've added a new helper getCalendarString() which takes from calendar.properties. This patch migrates cal.calGetString("calendar", ...) over to cal.l10n.getCalendarString(...). This patch was mostly automated with previous script, so you might get away with giving it only a skim.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 4 - Migrate Lightning strings - v1

This moves Lightning strings to use cal.l10n.getLtnString(). I've migrated both cal.calGetString("lightning", ..., "lightning"), as well as ltn.getString().

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 5 - Migrate dateFormat strings - v1

The dateFormat bundle was used fairly often, so I added a helper which I am migrating to in this patch.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 6 - Migrate wcap provider - v1

This should be a quick one.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 7 - Migrate gdata provider - v1

Probably could have folded this one. Also a quick one.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 8 - Migrate remaining calGetString instances - v1

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 9 - Make use of rest args - v1

This one I am worried about most, as I did it fully manually and it was mostly mechanical. I'd like to use rest args since it makes the params a bit nicer, this patch takes care.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

That is all for this bug. Sorry for the massive amount of patches, I tried to keep each small for easier review.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 4 - Migrate Lightning strings - v2

Missed a few occurrences of removing ltnUtils.jsm.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 8 - Migrate remaining calGetString instances - v2

Comment 14

[[:MakeMyDay]]

Comment on attachment 8953248 [details] [diff] [review]
Part 1 - Manual Changes - v1

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

r+ with comments considered.

::: calendar/base/modules/calL10NUtils.jsm
@@ +12,5 @@
> + * @param aComponent   Stringbundle component name
> + * @param aBundleName  The name of the properties file.  It is assumed that the
> + *                     file lives in chrome://calendar/locale/
> + * @param aStringName  The name of the string within the properties file
> + * @param aParams      (optional) Parameters to format the string

Please add @return to the doc block her.

@@ +31,5 @@
> +        }
> +    } catch (ex) {
> +        let msg = `Failed to read '${aStringName}' from ${propName}.`;
> +        Components.utils.reportError(`${msg} Error: ${ex}`);
> +        return msg;

I think it would be better to return aStringName here instead of the error message - that would be helpful also when creating new ui elements, since you can add the string name as a placeholder (without mixing up the UI due to the whole error msg) and craft the string later.

@@ +40,5 @@
> +
> +var call10n = {
> +    getAnyString: _getString,
> +    getString: _getString.bind(undefined, "calendar"),
> +    getCalendarString: _getString.bind(undefined, "calendar", "calendar"),

Can you make this getCalString? Even if it's similar to the old name, it' shorter and since string definitions tend to exceed our aspired line length limit, every character we can spare counts.

@@ +42,5 @@
> +    getAnyString: _getString,
> +    getString: _getString.bind(undefined, "calendar"),
> +    getCalendarString: _getString.bind(undefined, "calendar", "calendar"),
> +    getLtnString: _getString.bind(undefined, "lightning"),
> +    getDateFmtString: _getString.bind(undefined, "dateFormat"),

Shouldn't that be
getLtnString: _getString.bind(undefined, "lightning", "lightning") and
getDateFmtString: _getString.bind(undefined, "calendar", "dateFormat") ?

@@ +49,5 @@
> +     * Gets the month name string in the right form depending on a base string.
> +     *
> +     * @param aMonthNum     The month numer to get, 1-based.
> +     * @param aBundleName   The Bundle to get the string from
> +     * @param aStringBase   The base string name, .monthFormat will be appended

Please add @return to the doc block.

@@ +64,5 @@
> +    },
> +
> +    /**
> +     * Create a new locale collator
> +     */

same here.

@@ +73,5 @@
> +    },
> +
> +    /*
> +     * Sort an array of strings according to the current locale.
> +     * Modifies aStringArray, returning it sorted.

Please add @param and @return to the doc block

::: calendar/base/src/calUtils.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* Goodbye, calUtils.js. You've been a great friend over the years. We'll miss
> + * you and wish you all the best for your departure in one of the next
> + * changesets. */

Nice :-)

Comment 15

[[:MakeMyDay]]

Comment on attachment 8953257 [details] [diff] [review]
Part 2 - Migrate some functions out of calUtils.jsm - v1

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

r+ with the nits fixed

::: calendar/base/src/calDateTimeFormatter.js
@@ +125,5 @@
>                  let startYear = aStartDate.year;
>                  let endDay = this.formatDayWithOrdinal(endDate.day);
>                  let endYear = endDate.year;
>                  if (aStartDate.year != endDate.year) {
> +                    let startMonthName = cal.l10n.formatMonth(aStartDate.month + 1, "calendar", "daysIntervalBetweenYears");

Can you split this to and move |"calendar", "daysIntervalBetweenYears");| to the new line? Applies also for the other occurrences of formatMonth below.

Comment 16

[[:MakeMyDay]]

Comment on attachment 8953259 [details] [diff] [review]
Part 3 - Migrate calendar.properties strings - v1

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

See also my comment on attachment 8953248 [details] [diff] [review] regarding the naming of cal.l10n.getCalendarString. Since this is a hugh patch and the change is straight forward, you probably can just do a s/cal.l10n.getCalendarString/cal.l10n.getCalString/ on the patch before taking care of the nits below. Afaict, this approach should produce further style nits.

::: calendar/base/content/calendar-task-tree.xml
@@ +588,5 @@
>  
>                  switch (aCol.element.getAttribute("itemproperty")) {
>                      case "title":
>                          // return title, or "Untitled" if empty/null
> +                        return (task.title ? task.title.replace(/\n/g, " ") : cal.l10n.getCalendarString("eventUntitled"));

Can you please split this line?

::: calendar/base/content/import-export.js
@@ +177,2 @@
>                  } else if (failedCount) {
> +                    cal.showError(cal.l10n.getCalendarString("importItemsFailed", [failedCount, lastError.toString()]), window);

Please split this line.

::: calendar/base/src/calDateTimeFormatter.js
@@ +127,5 @@
>                  let endYear = endDate.year;
>                  if (aStartDate.year != endDate.year) {
>                      let startMonthName = cal.l10n.formatMonth(aStartDate.month + 1, "calendar", "daysIntervalBetweenYears");
>                      let endMonthName = cal.l10n.formatMonth(aEndDate.month + 1, "calendar", "daysIntervalBetweenYears");
> +                    return cal.l10n.getCalendarString("daysIntervalBetweenYears", [startMonthName, startDay, startYear, endMonthName, endDay, endYear]);

Please split this line.

@@ +135,4 @@
>                  } else {
>                      let startMonthName = cal.l10n.formatMonth(aStartDate.month + 1, "calendar", "daysIntervalBetweenMonths");
>                      let endMonthName = cal.l10n.formatMonth(aEndDate.month + 1, "calendar", "daysIntervalBetweenMonths");
> +                    return cal.l10n.getCalendarString("daysIntervalBetweenMonths", [startMonthName, startDay, endMonthName, endDay, endYear]);

Here, too.

@@ +158,5 @@
>              } else {
>                  // Spanning multiple days, so need to include date and time
>                  // for start and end
>                  // "5 Jan 2006 13:00 - 7 Jan 2006 9:00"
> +                return cal.l10n.getCalendarString("datetimeIntervalOnSeveralDays", [startDateString, startTime, endDateString, endTime]);

and here.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +458,5 @@
>  
> +    let promptTitle = cal.l10n.getCalendarString(
> +        cal.item.isEvent(window.calendarItem)
> +          ? "askSaveTitleEvent"
> +          : "askSaveTitleTask"

indentation - please make 

cal.item.isEvent(window.calendarItem) ? "askSaveTitleEvent" : "askSaveTitleTask"

in one line.

@@ +463,5 @@
> +    );
> +    let promptMessage = cal.l10n.getCalendarString(
> +        cal.item.isEvent(window.calendarItem)
> +          ? "askSaveMessageEvent"
> +          : "askSaveMessageTask"

same pattern here.

@@ +1547,1 @@
>                        getElementValue("item-title");

indentation

::: calendar/providers/caldav/calDavCalendar.js
@@ +1732,1 @@
>                                   "\n\n" + request.URI.spec;

indentation

::: calendar/resources/content/mouseoverPreviews.js
@@ +179,2 @@
>              } else {
> +                priorityString = cal.l10n.getCalendarString("lowPriority"); // low priority

We don't need the comments here - the string names are obviously already

::: calendar/resources/content/publish.js
@@ +215,5 @@
>          }
>  
>          if (channel && !requestSucceeded) {
> +            Services.prompt.alert(null, cal.l10n.getCalendarString("genericErrorTitle"),
> +                                  cal.l10n.getCalendarString("httpPutError", [channel.responseStatus, channel.responseStatusText]));

Can you cut down the line length here?

Comment 17

[[:MakeMyDay]]

Comment on attachment 8953263 [details] [diff] [review]
Part 5 - Migrate dateFormat strings - v1

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

r+, just style nits here.

::: calendar/base/content/calendar-task-tree.xml
@@ +590,5 @@
>                      case "title":
>                          // return title, or "Untitled" if empty/null
>                          return (task.title ? task.title.replace(/\n/g, " ") : cal.l10n.getCalendarString("eventUntitled"));
>                      case "entryDate":
> +                        return task.recurrenceInfo ? cal.l10n.getDateFmtString("Repeating") : this._formatDateTime(task.entryDate);

Can you split this line - applies to dueDate and completedDate below, too.

Comment 18

[[:MakeMyDay]]

Comment on attachment 8953657 [details] [diff] [review]
Part 4 - Migrate Lightning strings - v2

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

r+ with the style nits fixed

::: calendar/itip/calItipEmailTransport.js
@@ +215,5 @@
>                      parent = cal.window.getCalendarWindow();
>                  }
>                  if (Services.prompt.confirmEx(parent,
> +                                              cal.l10n.getLtnString("imipSendMail.title", null),
> +                                              cal.l10n.getLtnString("imipSendMail.text", null),

you can spare the trailing null here in both lines.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to [:MakeMyDay] from comment #14 - part 1)
> @@ +31,5 @@
> > +        }
> > +    } catch (ex) {
> > +        let msg = `Failed to read '${aStringName}' from ${propName}.`;
> > +        Components.utils.reportError(`${msg} Error: ${ex}`);
> > +        return msg;
> 
> I think it would be better to return aStringName here instead of the error
> message - that would be helpful also when creating new ui elements, since
> you can add the string name as a placeholder (without mixing up the UI due
> to the whole error msg) and craft the string later.

Fine with me, changed that.

> Can you make this getCalString? Even if it's similar to the old name, it'
> shorter and since string definitions tend to exceed our aspired line length
> limit, every character we can spare counts.

Not liking the name similar to the old one, but I don't have a better suggestion so I'm making the change. Maybe next we'll have stringCalGet :D

> Shouldn't that be
> getLtnString: _getString.bind(undefined, "lightning", "lightning") and
> getDateFmtString: _getString.bind(undefined, "calendar", "dateFormat") ?

It should, looks like I fixed this in a later changeset. Moving it up to part 1.


> Please add @return to the doc block.

I've added the file to eslint, and also added docblocks for the shortcut functions.


(In reply to [:MakeMyDay] from comment #15 - part 2)
+                    let startMonthName = cal.l10n.formatMonth(aStartDate.month + 1, "calendar", "daysIntervalBetweenYears");
> 
> Can you split this to and move |"calendar", "daysIntervalBetweenYears");| to
> the new line? Applies also for the other occurrences of formatMonth below.

I went for a split after "calendar", because that fits into 100 characters.

(In reply to [:MakeMyDay] from comment #16 - part 3)

> regarding the naming of cal.l10n.getCalendarString. Since this is a hugh
> patch and the change is straight forward, you probably can just do a
> s/cal.l10n.getCalendarString/cal.l10n.getCalString/ on the patch before
> taking care of the nits below. Afaict, this approach should produce further
> style nits.
You mentioned there will be further nits, did you want to re-review?

btw this caused me some hard merge conflicts. Might have been better to do it at the end with a separate commit. Oh well :)

> ::: calendar/providers/caldav/calDavCalendar.js
> @@ +1732,1 @@
> >                                   "\n\n" + request.URI.spec;
> 
> indentation
This doesn't fit on one line of 100 chars, and the parens end early. Unless you have a suggestion, I'm going to leave it as is.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 1 - Manual Changes - v2

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 2 - Migrate some functions out of calUtils.jsm - v2

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 3 - Migrate calendar.properties strings - v2

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 4 - Migrate Lightning strings - v3

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 5 - Migrate dateFormat strings - v2

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 6 - Migrate wcap provider - v2

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 7 - Migrate gdata provider - v2

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 8 - Migrate remaining calGetString instances - v3

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 9 - Make use of rest args - v2

Comment 29

[[:MakeMyDay]]

Comment on attachment 8962632 [details] [diff] [review]
Part 9 - Make use of rest args - v2

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

Codewise this looks good, with the below nits considered. But I'm not really excited to spread the use of rest args because it reduces the readability imho and I was tempted to r- the patch for that reason.

::: calendar/base/modules/calL10NUtils.jsm
@@ +11,5 @@
>   *
>   * @param {String} aComponent       Stringbundle component name
>   * @param {String} aBundleName      The name of the properties file
>   * @param {String} aStringName      The name of the string within the properties file
> + * @param {...String} aParams       (optional) Parameters to format the string

Can you make the param description more precise here and for the other getString functions in this file. These params are not used to format the string but to fill placeholders therein if any.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +1907,5 @@
>      for (let cloudProvider of cloudFileAccounts.accounts) {
>          // Create a serializable object to pass in a message outside the iframe
>          let itemObject = {};
>          itemObject.displayName = cloudFileAccounts.getDisplayName(cloudProvider);
> +        itemObject.label = cal.l10n.getString("calendar-event-dialog", "attachViaFilelink", itemObject.displayName);

Can you split this line?

::: calendar/providers/gdata/modules/gdataUtils.jsm
@@ +1320,5 @@
>  /**
>   * Get a string from the gdata properties file
>   *
>   * @param aStringName  The name of the string within the properties file
>   * @param ...aParams   Optional parameters to format the string

The above comment regarding the param description applies here as well.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8962632 [details] [diff] [review]
Part 9 - Make use of rest args - v2

I'm fine keeping the parameters an array, luckily it looks like I kept this compatible so we can just not push part 9.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2a4475b8555b
Move l10n related functions into calL10NUtils.jsm - part 1 - initial manual changes. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/1c3ccf7a769f
Move l10n related functions into calL10NUtils.jsm - part 2 - migrate formatMonth/sortArrayByLocaleCollator/createLocaleCollator. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/2530e8642a61
Move l10n related functions into calL10NUtils.jsm - part 3 - migrate getCalString. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/b8fa40ac976b
Move l10n related functions into calL10NUtils.jsm - part 4 - Migrate getLtnString. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/605e1e0382bd
Move l10n related functions into calL10NUtils.jsm - part 5 - migrate getDateFmtString. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/550e60949923
Move l10n related functions into calL10NUtils.jsm - part 6 - migrate WCAP provider. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/cb5c6db5bd6d
Move l10n related functions into calL10NUtils.jsm - part 7 - migrate gdata provider. r=MakeMyday
https://hg.mozilla.org/comm-central/rev/4d0b8fcfc066
Move l10n related functions into calL10NUtils.jsm - part 8 - migrate remaining calGetString instances. r=MakeMyDay

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/releases/comm-beta/10085a3cc8a1
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 1 - initial manual changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/6e8d7aef9631
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 2 - migrate formatMonth/sortArrayByLocaleCollator/createLocaleCollator. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/5250ba1d9d41
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 3 - migrate getCalString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/4151d5bee5b7
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 4 - Migrate getLtnString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/3e0fec7cfb1b
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 5 - migrate getDateFmtString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/2d043b6dc896
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 6 - migrate WCAP provider. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/b2648949aedf
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 7 - migrate gdata provider. r=MakeMyday
https://hg.mozilla.org/releases/comm-beta/7bc3d4ebc70a
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 8 - migrate remaining calGetString instances. r=MakeMyDay

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/10085a3cc8a1
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 1 - initial manual changes. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/6e8d7aef9631
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 2 - migrate formatMonth/sortArrayByLocaleCollator/createLocaleCollator. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/5250ba1d9d41
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 3 - migrate getCalString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/4151d5bee5b7
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 4 - Migrate getLtnString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/3e0fec7cfb1b
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 5 - migrate getDateFmtString. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/2d043b6dc896
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 6 - migrate WCAP provider. r=MakeMyDay
https://hg.mozilla.org/releases/comm-beta/rev/b2648949aedf
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 7 - migrate gdata provider. r=MakeMyday
https://hg.mozilla.org/releases/comm-beta/rev/7bc3d4ebc70a
Bug 1440490 - Move l10n related functions into calL10NUtils.jsm - part 8 - migrate remaining calGetString instances. r=MakeMyDay