1751784 - Follow-up to bug 1751493: C++ part and folder pane part of live language switching

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, task)

Description

[Rachel Martin]

+++ This bug was initially created as a clone of Bug #1751493 +++

See bug 1751493 comment #8.

Comment 1

[Rachel Martin]

Attachment: WIP: 1751784-refresh-static-cpp-strings-on-locale-change.patch

This refreshes the thread pane content (status column), not the thread pane column headers. It doesn't work for the folder pane although the correct localised special folder names are correctly updated when the locale changes. When a second 3pane window is launched, most the UI including the thread pane headers are in the correct new language, apart from the localised special folder names :-(

Comment 2

[Rachel Martin]

The patch covers all the C++ issues, however, the folder pane has more problems:
Units are cached in an internal object here:
https://searchfox.org/comm-central/rev/6d4243f466f708d8c119bf869eee09231b7cbcaf/mail/base/content/folderPane.js#3986-3992
And the reason the folder tree doesn't update is likely in the cache which even survives a _rebuild():
https://searchfox.org/comm-central/rev/6d4243f466f708d8c119bf869eee09231b7cbcaf/mail/base/content/folderPane.js#1943-1944

Comment 3

[Rachel Martin]

Attachment: Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

This takes care of the C++ bits but there is more work required in the folder pane update. Please find a different developer for that. Refer to comment #2 for the issues with the folder pane.

Comment 4

[Rachel Martin]

Please assign someone else for the second part, the folder pane.

Comment 5

[Rachel Martin]

Comment on attachment 9260501 [details] [diff] [review]
Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

Just for the record, this is based on:
https://hg.mozilla.org/comm-central/rev/8c7735bd0dc3
https://hg.mozilla.org/comm-central/rev/3834c3b93900

Comment 6

[Ben Campbell]

Comment on attachment 9260501 [details] [diff] [review]
Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

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

Looks OK to me.  I'd just suggest a few comments to make it clear that nsMsgDBView requires the new nsMsgDBViewService to set up some of it's static members.

::: mailnews/base/public/nsIMsgDBView.idl
@@ +196,5 @@
>    const nsMsgNavigationTypeValue toggleSubthreadKilled = 23;
>  };
>  
> +/*
> + * The contract ID for this component is @mozilla.org/msgDBView/msgDBViewService;1.

Would be nice to have a note here on what this service does and why it is required.

My take is that we need a separate service to make it accessible from JS, right?
We can't directly call the static fns nsMsgDBView::InitializeLiterals() et al from JS.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +52,5 @@
>  #include "mozilla/intl/AppDateTimeFormat.h"
>  
>  using namespace mozilla::mailnews;
>  nsrefcnt nsMsgDBView::gInstanceCount = 0;
>  

Maybe worth a little breadcrumb comment here saying that these are static vars, set up via nsMsgDBViewService?

@@ +167,1 @@
>      InitDisplayFormats();

A bit out of scope, but could InitDisplayFormats() also be made static too and handled by the nsMsgDBViewService?
Then we could ditch `gInstanceCount` altogether, which would be nice.

Comment 7

[Rachel Martin]

Thanks for the review. We'll address the comments when we're back at the office in March. Meanwhile it would be good if you could find a developer to cover the folder pane part.

Comment 8

[Rachel Martin]

Attachment: Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

(We found a development machine earlier than anticipated.)
This adds comments to the IDL files and replaces the instance count with another static variable.

Overall, this is far from finished: When switching language, the folder pane doesn't update. The thread pane is redrawn, but only partly updates: The content of the (usually unused) status column updates, but the date column does not. We debugged it a bit and saw that mozilla::intl::AppDateTimeFormat::Format() returns the date/time formatted to the original locale even when switching language (and setting the formatting to follow the app locale and not the OS locale). This is visible when changing between English and German, especially when adding a weekday (mail.ui.display.dateformat.thisweek set to 4). Someone should file a bug in M-C saying that the date format doesn't follow the language (or does it in FF?).

Comment 9

[Rachel Martin]

Clearing the folder cache over in bug 1755208 hasn't helped. Looks like at least for IMAP folders the localised pretty name is set during folder discovery at startup:
https://searchfox.org/comm-central/rev/7a0e536f6bbef48d2bd17a410017f2d5ac5e4b0a/mailnews/imap/src/nsImapIncomingServer.cpp#1127
called from here:
https://searchfox.org/comm-central/rev/7a0e536f6bbef48d2bd17a410017f2d5ac5e4b0a/mailnews/imap/src/nsImapProtocol.cpp#5224

nsMsgDBFolder::SetPrettyName() is the function that does the localising:
https://searchfox.org/comm-central/rev/7a0e536f6bbef48d2bd17a410017f2d5ac5e4b0a/mailnews/base/src/nsMsgDBFolder.cpp#3149
and then stores the name against the folder:
https://searchfox.org/comm-central/rev/7a0e536f6bbef48d2bd17a410017f2d5ac5e4b0a/mailnews/base/src/nsMsgDBFolder.cpp#3200

So to re-localise, one would have to call all SetPrettyName() again. We haven't looked at how local folders get localised at startup.

Comment 10

[Rachel Martin]

Attachment: Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

Just removed incorrect comment referring to folder cache. Changes wrt. to original already-r+ patch are:
Comments in IDL files and replacement of instance count with another static variable.

We'll submit part 2 soon with update to folder pane; overall idea:
SetPrettyName() to store original folder name; when locale changes, iterate over all folders in the map in the "folder lookup service", then SetPrettyNameFromOriginal() on all folders in the map.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: live-language-reload.webm

Rachel, here's the screen recording of daily on Linux with your patch applied.
With the live reload prefs set to TRUE, most of the strings change when I switch language.
As you can see, if I open and close a parent root folder, the names of the folders are updated with a slight delay.
I'll try to investigate and see why this happens and what's preventing the FtvItem.getText() from using the refreshed message bundle.
I hope this helps.

Comment 12

[Rachel Martin]

Attachment: Part 2: 1751784-re-localise-folders.patch

This is the cheapest way to refresh the folder pane. Of course it's quite a bit of hackery, why should the FLS have a function to whack all the folders to they change names. We've considered to return an array of all the folders in the map and process them in the mail glue, but that's less efficient. Open to better ideas.

@Alessandro: Thanks for the video. We don't see this sort of refresh. Most likely in your case a new IMAP discovery is run explaining the slight delay and name update, see comment #9 for details.

Comment 13

[Ben Campbell]

Comment on attachment 9263953 [details] [diff] [review]
Part 1: 1751784-refresh-static-cpp-strings-on-locale-change.patch

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

LGTM

Comment 14

[Ben Campbell]

Comment on attachment 9264054 [details] [diff] [review]
Part 2: 1751784-re-localise-folders.patch

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

I think that looks OK.
I'm not too worried about hacks in the FLS, as I'd rather like to get rid of the FLS entirely (or at the very least, have it lookup URIs by asking the appropriate account/server object. I don't think it's worth the FLS having it's own cache of URIs).
So if it does what you need for now, I'm happy with it. We can revisit it in the future.

Comment 15

[Rachel Martin]

Attachment: Part 2: 1751784-re-localise-folders.patch

Updated HG header.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02c86bd08e63
Re-initialize static C++ variables in nsMsgDBFolder.cpp and nsMsgDBView.cpp when locale changes. r=benc

Comment 17

[Rachel Martin]

Please commit part 2 as well.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/76b9b11571b1
Re-localise all folder names when locale changes. r=benc