Closed Bug 1473294 Opened 6 years ago Closed 6 years ago

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

Categories

(MailNews Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr6062+ fixed, thunderbird62 wontfix, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr60 62+ fixed
thunderbird62 --- wontfix
thunderbird63 --- fixed

People

(Reporter: MakeMyDay, Assigned: darktrojan)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Summary: Support l10n preferences in extsension preferences loader → Support l10n preferences in extension preferences loader
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?
Flags: needinfo?(makemyday)
Version: unspecified → 58
Flags: needinfo?(makemyday) → needinfo?(geoff)
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.
No, it can't. Please file a new bug with a complete description of the issue.
Filed bug 1481600.
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.
Flags: needinfo?(geoff)
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.
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
Attached patch 1473294-pref-load-1.diff (obsolete) — Splinter Review
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.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8998488 - Flags: review?(jorgk)
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.
Attachment #8998488 - Flags: review?(jorgk) → review?(philipp)
(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.
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.
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.
Made a mistake comparing nsIFiles instead of the paths they represent.
Attachment #8998488 - Attachment is obsolete: true
Attachment #8998488 - Flags: review?(philipp)
Attachment #8999492 - Flags: review?(philipp)
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.
Attachment #8999492 - Flags: review?(philipp) → review+
FixIterator doesn't work with nsIUTF8StringEnumerator.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d06d9961fcf5
Load default pref files in reverse order to match original behaviour. r=philipp DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Flags: needinfo?(geoff)
Flags: needinfo?(geoff)
Attachment #8999865 - Flags: approval-comm-esr60?
Attachment #8999492 - Flags: approval-comm-beta?
Attachment #8999865 - Flags: approval-comm-esr60? → approval-comm-esr60+
Target Milestone: --- → Thunderbird 63.0
Comment on attachment 8999492 [details] [diff] [review]
1473294-pref-load-2.diff

This will go to a TB 60.? beta via the ESR.
Attachment #8999492 - Flags: approval-comm-beta?
Changing summary to hopefully avoid further duplicates.
Summary: Support l10n preferences in extension preferences loader → Support l10n preferences in extension preferences loader - Causes: First day of the week is reset to Sunday after every Thunderbird restart
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: