1473294 - Support l10n preferences in extension preferences loader - Causes: First day of the week is reset to Sunday after every Thunderbird restart

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[[:MakeMyDay]]

Before the old preference system for extensions was removed, there was an option to provide localized preferences by bundling {extensionname}-l10n.js preference files. This was used by Lightning to provide locale specific default settings [1]. The new loader seems to miss support for such a capability.

If this is considered to be implemented, one should also consider to expand this to support an {extensionname}-{locale}.js schema to allow multi-language packaging with localized preferences for each packaged locale explicitely (since we always got bug reports for setting being reset when user switched between a multi xpi from amo and the bundled single locale xpi).

[1] https://searchfox.org/comm-central/source/calendar/locales/en-US/lightning-l10n.js

Comment 1

[[:MakeMyDay]]

This is hitting us for Lightning in TB60 now, latest report bug 1481469. Localized default prefernces are effectively ignored although preferences-l10n.js is packaged in correctly filled. For the user this looks like their preferences were reset. We should try to at least restore the previous behaviour (the additional part in comment 0 would prevent the same UX when switching from a bundled version of Lightning to a ATN version).

Geoff, any chance you can take a look at that?

Comment 2

[Sven Grull]

When I open a meeting (old or new) in Thunderbird 60 the reminders are all gone. Could this also be a result of this bug or is this another bug? I use the German version of Thunderbird.

Comment 3

[[:MakeMyDay]]

No, it can't. Please file a new bug with a complete description of the issue.

Comment 4

[Sven Grull]

Filed bug 1481600.

Comment 5

[Geoff Lankow (:darktrojan)]

From https://searchfox.org/comm-central/source/common/src/extensionSupport.jsm#90 :

> var entries = zipReader.findEntries("defaults/preferences/*.js");
> entries.getNext(); // "defaults/preferences/lightning-l10n.js"
> entries.getNext(); // "defaults/preferences/lightning.js"

The pref files are being read in the wrong order, so the localised prefs are overwritten by the defaults. I *think* the order is supposed to be alphabetical, but I'll look into it.

Comment 6

[Geoff Lankow (:darktrojan)]

There is supposed to be sorting, and we're not doing it, but even if we were, "lightning-l10n.js" is before "lightning.js", so that's weird.

Comment 7

[Geoff Lankow (:darktrojan)]

The supporting appears to be in reverse, that would solve our issue.

https://dxr.mozilla.org/mozilla-esr52/source/modules/libpref/Preferences.cpp#1064

Although it doesn't (didn't) happen at all for packaged extensions. This code is a minefield, no wonder they wanted to get rid of it:

https://dxr.mozilla.org/mozilla-esr52/source/modules/libpref/Preferences.cpp#703

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1473294-pref-load-1.diff

This matches the original behaviour more closely. The original code wrote default prefs to the default branch no matter what, and also it sorted the files in reverse order.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8998488 [details] [diff] [review]
1473294-pref-load-1.diff

Sorry, this stuff is mostly Philipp's baby. Or maybe MMD wants to take the review.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #8)
> This matches the original behaviour more closely. The original code wrote
> default prefs to the default branch no matter what,

I meant to also say, that since we use this code only for legacy extensions, overwriting the default prefs for a session doesn't cause a problem, since the extension exists until at least the end of the session too.

Comment 12

[[:MakeMyDay]]

If tested this while working on bug 1481790. It works well in general, but I noticed one issue (not necessarily caused but at least uncovered by this patch and maybe a calendar but a mailnews issue).

For test purposes I patched the lightning-l10n.js values:

calendar.week.start = 1,
calendar.week.d5fridaysoff = true
calendar.categories.names by adding a letter to the first name

Make sure you have all customized preferences reset. All the changes got picked up and applied, however in the calendar options > Views the order of the workweek is not updated and still starting with Sunday (=0) instead of Monday (=1) as you would expect from the applied localized preferences. It will update accordingly when changing the first day in the UI, but it gets initialized with the wrong value or not.

I have this just checked on a local trunk build (with the pending patches for wx applied), but not on a esr try. so not sure whether this is s race condition with the wx loader or an issue of the pref loader code.

Comment 13

[[:MakeMyDay]]

The issue above is probably related to race condition when doing wx loading and therefore would affect esr60 - let's track that over in bug 1482351.

Can we do a try push for a localized build of esr60? If not, a try push to esr60 with an additional patch for modifying lightning-l10n.js as described above would do the same to confirm that.

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 1473294-pref-load-2.diff

Made a mistake comparing nsIFiles instead of the paths they represent.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8999492 [details] [diff] [review]
1473294-pref-load-2.diff

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

::: common/src/extensionSupport.jsm
@@ +86,3 @@
>          while (entries.hasMore()) {
> +          unsortedEntries.push(entries.getNext());
> +        }

You could do:

let unsortedEntries = [...fixIterator(entries, somethingsomething)];

Fine with skipping this though if you prefer.

Comment 16

[Geoff Lankow (:darktrojan)]

FixIterator doesn't work with nsIUTF8StringEnumerator.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d06d9961fcf5
Load default pref files in reverse order to match original behaviour. r=philipp DONTBUILD

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8999492 [details] [diff] [review]
1473294-pref-load-2.diff

You want this in TB 60, right? Can you do an esr60 patch since this one doesn't apply at all.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: 1473294-pref-load-esr60.diff

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8999492 [details] [diff] [review]
1473294-pref-load-2.diff

This will go to a TB 60.? beta via the ESR.

Comment 23

[Jorg K (CEST = GMT+2)]

TB 60.1 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/8cdf073f28405ff0efc435b87bf4df9bf5c2d94b

Comment 29

[Jorg K (CEST = GMT+2)]

Changing summary to hopefully avoid further duplicates.