1320191 - Outlook/Hotmail trash folder is called "Deleted" in English and localised versions of Thunderbird

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[andrei030]

Attachment: Sin título.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

Nothing, clean installation.


Actual results:

Name for "Deleted" folder is displayed in English instead of Spanish language.

Comment 1

[Jorg K (CEST = GMT+2)]

¡Está mal! ;-) Which version are you using?

Comment 2

[Jorg K (CEST = GMT+2)]

Actually, we need the version and the exact localisation, Spanish from Spain or Argentina, Chile, Mexico.
https://www.youtube.com/watch?v=eyGFz-zIjHE - ver a partir del minuto 2:12 ;-)

Comment 3

[RickieES aka Ricardo Palomares]

Yes, specify the regional variant, Thunderbird version and the exact steps you follow to reproduce the error. I've just used Transvision to look up the word and none of the four localizations seem to have that word untranslated.

Also, please note that the en-US word for the folder where deleted items are placed is not named "Deleted", but "Trash". I actually think you're seeing the name of a folder already created in an IMAP account.

Comment 4

[andrei030]

Sorry I didn't specified, the version is the last one, 45.5.0, and the localization es-ES.

Comment 5

[RickieES aka Ricardo Palomares]

Attachment: es-ES name for Trash folder in Local Folders and an IMAP (Gmail) account

So, this is definitely due to your specific IMAP account. Both the local folders and a GMail account Trash folder are translated as "Papelera2.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks for clarifying.

Comment 7

[andrei030]

So I have to deal with it and leave it in English, OK.

Comment 8

[Jorg K (CEST = GMT+2)]

Can't you rename the folder at IMAP level? Use the web interface of the provider to rename it? Or check the subscribe menu.

Comment 9

[andrei030]

It's a software problem, the folder cannot be renamed in any way. If I use the browser to enter my mail account the name is displayed correctly in Spanish. 

All the folders are named automatically and correctly by Thunderbird except the Deleted folder. For example:

Deleted | imap://*@imap-mail.outlook.com/Deleted | Deleted
Inbox | imap://*@imap-mail.outlook.com/INBOX | Bandeja de entrada
Junk | imap://*@imap-mail.outlook.com/Junk | Correo no deseado
Etc. 

If the folders are named by Thunderbird is to be expected that all of them are, and not all except the Deleted one.

I also tried to force rename using about:config, mail.server.server1.trash_folder_name, but it doesn't save my modifications, it keeps switching back to "Deleted" by itself.

Comment 10

[Jorg K (CEST = GMT+2)]

OK, I configured a Hotmail/Outlook/Live account in a Spanish version of TB 45.x and the folder is called "Deleted". In an English version of TB, the folder is also called "Deleted" not "Trash".

Let's look at it from the Thunderbird side again.

Comment 11

[Jorg K (CEST = GMT+2)]

Kent, Magnus, how do the "special" folders get their names? Why would the Outlook/Trash folder be called "Deleted" in English and localised versions of TB?

I did an experiment on Outlook. I created a folder "Deleted" and got told that this created a name conflict. So obviously the Outlook folder is called "Deleted" internally, but since TB recognises it as special folder, why doesn't it assign a special name?

Comment 12

[Magnus Melin [:mkmelin]]

It's a bit complex. RFC 6154 makes us set some flags. See nsMsgDBFolder::SetPrettyName and there around.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch

This works. With this patch, the folder is called "Trash" and would be localised properly.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8814729 [details] [diff] [review]
1320191-deleted-folder.patch

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

I don't think this is right. The "name" should get set correctly to internally "trash" from somewhere... I don't know the details but it's complicated. We map up "our" special names to what the server tells us, if it tells us. Then localize them if necessary.

Comment 15

[andrei030]

Thank you for your attention guys!

Comment 16

[Jorg K (CEST = GMT+2)]

This is indeed complicated.

Following http://kb.mozillazine.org/IMAP_Trash_folder I deleted the preference mail.server.server#.trash_folder_name

with the effect that I now have a Trash folder (which most likely TB will use for deleted items) and a Deleted folder (which most likely Hotmail will use). Most likely the Trash folder would be localised.

Reporter, if you're interested, delete/reset the preference mail.server.server#.trash_folder_name (you need to replace # with the real number) and report back what happens.

I still think there is a problem in reliably detecting the Hotmail Deleted folder. Maybe we need to tweak in nsImapIncomingServer::DiscoveryDone(). Well, on second thought, the folder *is* detected, but it's then not subject to localisation since it's only localised if it was originally called "Trash". Otherwise TB assumes that the name found is the name that the user wants to see.

Comment 17

[Jorg K (CEST = GMT+2)]

Summarising:
The problem seems to be that Micro$oft in their infinite wisdom don't use the standard trash name "Trash" (like all other IMAP services I know) but instead "Deleted". TB only localises if the folder is originally called "Trash" and hands all other names on to the user interface, most likely since it already assumes them to be localised.

So I'd have to see where to tweak this since Magnus didn't like the hack I presented. Looks like we'll have to treat M$ servers differently, just like we treat Gmail differently.

Comment 18

[Magnus Melin [:mkmelin]]

Treating differently probably isn't the thing to do. I'm not sure we actually do that for Gmail folders either, IIRC we're using what server tells us to.

As I recall, some things to be aware of
 - imap servers may tell you the special folders
 - we support imap servers having special folders localized server side (I don't recall what we display for those)
 - thunderbird will let you run localized thunderbird versions, but still with same folder names, so you can switch back and forth (for POP)

Comment 19

[Jorg K (CEST = GMT+2)]

Gmail is treated differently:
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mailnews/imap/src/nsImapIncomingServer.cpp#1547

> we support imap servers having special folders localized server side
Yes. But you can't distinguish between a localisation on the server side and "Deleted". Both are not the standard name "Trash".

> imap servers may tell you the special folders
Yes:
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mailnews/imap/src/nsImapIncomingServer.cpp#1582

The problem is that we only display the localised name if the original name was "Trash" which is not the case for M$. So in my patch I localise when "Trash" or "Deleted" is found. This is the only solution I can think of, unless we want to treat M$ differently. But I wouldn't know how. Gmail at least is recognisable by their extra bit:
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mailnews/imap/src/nsImapCore.h#149
I didn't see anything like this of M$.

So maybe you'd like to reconsider the r- here.

Comment 20

[andrei030]

I've tried to rename or restore "trash_folder_name" before, it always reset back to "Deleted". I've also tried to delete it completely editing prefs.js and editing it and make the file read only, doesn't work, it sets back to "Deleted" all the time.

Comment 21

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 8814729 [details] [diff] [review]
1320191-deleted-folder.patch

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

> -  else if (mFlags & nsMsgFolderFlags::Trash && name.LowerCaseEqualsLiteral("trash"))
> +  else if (mFlags & nsMsgFolderFlags::Trash &&    // Microsoft calls the folder "Deleted".
> +           (name.LowerCaseEqualsLiteral("trash") || name.LowerCaseEqualsLiteral("deleted")))

Oh, it's similar to filename =Unsent Messages, en-US folder name=Outbox, localized "Outbox" in each localized  build.
In this case,  "folder name=Outbox" is wriiten in Unsent Messages.msf, and, IIRC, "localized Outbox of  a localized  build" was written in .msf file in special situation.
I actually saw "related msgStore name=trash" in Trash.msf when "trash" was created by user, and I saw  "localized Trash of  a localized  build" in Trash.msf.
This was a cause of phenomenon of "Trash disappered, I can't delete mail!" which seems to still occur in some user's  non-ordinal environment.

If done on "Deleted", "Sent Items", "Sent Mail", "bin", "Draft"(Yahoo! IMAP)  and so on, I belive localized name for them should be provided, instead of "faked localized name via. faked folder name".

Comment 22

[WADA:World Anti-bad-Duping Agency]

I think we are better distinguish following:
a) Use Deleted as special folder in Thunderbird automatically, as Trash is used as special folder in Thunderbird automatically.
b) Show localized name for Deleted if Deleted is used as "Special Folder of Trash" via. Server Settings.

Comment 23

[WADA:World Anti-bad-Duping Agency]

To Jorg K.
I think method provided by your patch sample is partially correct for enhancement of b) in previous my comment.
  If folder has "Special Folder attribute of Trash"
  && if the folder's prettiestName.toLowerCase() is contained in table of TraseNameSet,
    true==TraseNameSet.has(prettiestName.toLowerCase() of the folder) where TraseNameSet=new Set(["trash","deleted","bin",...])
  Use it as-is in en-US build and use localized name for it in localized build at Folder Pane/Folder Picker.
  After it, all is up to localizer of localized build on localized name of "Deleted", "bin" etc.
  Then, localizer can freely create localized name for "Deleted", "deleted", "DeLeTed", ... .

Comment 24

[Jorg K (CEST = GMT+2)]

Magnus, putting NI for you to reconsider the patch or give some better instructions so we don't forget this.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814729 [details] [diff] [review]
1320191-deleted-folder.patch

Sorry Kent to trouble you with this review.

Maybe we can keep this brief. Use TB on a Hotmail account and observe that the trash folder is called "Deleted".

What happens?

I folder called "Deleted" identifies itself to TB as unique trash at
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mailnews/imap/src/nsImapIncomingServer.cpp#1582.

It sets the preference mail.server.server1.trash_folder_name to Deleted.

OK. So far so good. But what has really happened? TB recognised a trash folder with a non-standard name as trash, like if the IMAP server were saying: My trash is called (Spanish) "Papelera". So then TB will not change the trash name to the localised name here:
https://dxr.mozilla.org/comm-central/rev/f593d2d106c15ad9682a35eaa1a8679e7127acd6/mailnews/base/util/nsMsgDBFolder.cpp#3414
since it's already considered localised.

The only option to force Hotmail's Deleted folder to be localised is what I've proposed in the patch, to consider "Deleted" also as a standard name that will trigger localisation.

This bug has been hanging around for more than six months, so it's time to move it.

Comment 26

[Kent James (:rkent)]

I have fought this issue quite a bit in ExQuilla (compounded by poor IDL character strings that do not properly encode non-ascii characters). What you have to really be careful of is creating duplicate folders.

The problems are, as is typical, the conflation of several concepts.

1) The folder URL comes directly from the file folder name, which must be the mail-expected canonical value (such as inbox, or trash). For all folders except for the canonical ones, the URL is the actual folder name. The canonical ones though expect particular values. "trash" for example. Sometimes if missing, they will get recreated, hence the duplicate folders.

2) Although folders like IMAP and EWS have the concept of a canonical "trash" folder, how is that indicated? You would think it would be using the folder flag, but the flag moves to a different, non-canonical folder when a user specifies a different folder for trash, junk, sent, etc. So at the point we are at here ins SetPrettyName, the folder URL (or name) is the correct way to indicate the canonical folder, not the flag. Checking for "deleted" seems strange to me.

So getting this correct is much harder than you think, because the underlying design is flawed so we are putting kludge on top of kludge.

The correct thing to do is to fix the underlying conflation problem. I am not saying we should not attempt a kludge, but this is why it is hard to review this apparently simple change.

For this particular case as I understand it, the question I would ask is why does a folder get created with the URL (name) "deleted"? Isn't imap recognizing it as canonical?

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks for the detailed reply.

(In reply to Kent James (:rkent) from comment #26)
> For this particular case as I understand it, the question I would ask is why
> does a folder get created with the URL (name) "deleted"? Isn't imap
> recognizing it as canonical?
How would IMAP recognise it as canonical and where is that code?

I'm looking at 
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mailnews/imap/src/nsImapIncomingServer.cpp#1532

Let me copy it here and talk about it:

so first we get folders which carry the "trash" flag.

    rv = rootMsgFolder->GetFoldersWithFlags(nsMsgFolderFlags::Trash,
                                            getter_AddRefs(trashFolders));

    if (NS_SUCCEEDED(rv) && trashFolders)
    {
      uint32_t numFolders;
      trashFolders->GetLength(&numFolders);
      nsAutoString trashName;

here we get the trash folder name from the server specific preference.
We end up in nsImapIncomingServer::GetTrashFolderName()

      if (NS_SUCCEEDED(GetTrashFolderName(trashName)))
      {

loop over the folders and process each one

        for (uint32_t i = 0; i < numFolders; i++)
        {
          nsCOMPtr<nsIMsgFolder> trashFolder(do_QueryElementAt(trashFolders, i));
          if (trashFolder)
          {
            // If we're a gmail server, we clear the trash flags from folder(s)
            // without the kImapXListTrash flag. For normal servers, we clear
            // the trash folder flag if the folder name doesn't match the
            // pref trash folder name.
            if (isGMailServer)
            {
              nsCOMPtr<nsIMsgImapMailFolder> imapFolder(do_QueryInterface(trashFolder));
              int32_t boxFlags;
              imapFolder->GetBoxFlags(&boxFlags);
              if (boxFlags & kImapXListTrash)
              {
                continue;
              }
            }
            else
            {

we're not doing Gmail

              // trashName is the leaf name on the folder URI, which will be
              // different from the folder GetName if the trash name is
              // localized.
              nsAutoCString trashURL;
              trashFolder->GetFolderURL(trashURL);
              int32_t leafPos = trashURL.RFindChar('/');
              nsAutoCString unescapedName;

get the folder name from the URL.
              MsgUnescapeString(Substring(trashURL, leafPos + 1),
                                nsINetUtil::ESCAPE_URL_PATH, unescapedName);
              nsAutoString nameUnicode;

              if (NS_FAILED(CopyMUTF7toUTF16(unescapedName, nameUnicode)) ||
                  trashName.Equals(nameUnicode))
              {

If it equals what we already have in the preference, we're done.

                continue;
              }
              if (numFolders == 1)
              {
                // We got here because the preferred trash folder does not
                // exist, but a folder got discovered to be the trash folder.

We found a folder which is the unique trash, so store it's name in the preference and done.

                SetUnicharValue(PREF_TRASH_FOLDER_NAME, nameUnicode);
                continue;
              }
            }

If we get here, we'll just clear the "trash" flag.

            trashFolder->ClearFlag(nsMsgFolderFlags::Trash);
          }
        }
      }
    }
  }

So the code above will locate a unique folder with the trash flag, and store it's URL name in the preference.

That folder might be called "Trash", or "Deleted" or "Papelera". Trash will subject that to localisation, "Deleted" or "Papelera" will be assumed to be localised already and NOT trigger the code at:

https://dxr.mozilla.org/comm-central/rev/f593d2d106c15ad9682a35eaa1a8679e7127acd6/mailnews/base/util/nsMsgDBFolder.cpp#3414

So I'm not sure what you mean by "canonical" name or where that would be detected. 

The idea of the patch is to allow "Deleted" as well as "Trash" as folder names for the trash folder so that the localised name will be used.

For a Hotmail user of a Spanish TB this will happen: TB queries Hotmail and retrieves these special folders:
Inbox, Junk, Deleted, etc.

Inbox and Junk match TB's naming scheme and so the code around the patch uses kLocalizedInboxName, for Junk it uses kLocalizedJunkName. But "Deleted" doesn't match, so it's passed on as already localised. So the Spanish user sees a bunch of localised names, with a non-localised one staring at him in the middle.

So if there's something about canonical folders I'm missing, please let me know. If not, the patch must be correct.

Aceman said he knows something about this, so NI'ing him. If I read Wada's comment #22 and comment #23 correctly, he agrees with me.

Comment 28

[Kent James (:rkent)]

What I mean by "canonical trash folder", after looking more at IMAP specs and our code, is a folder with the \TRASH attribute per rfc-6154. You can search for kImapXListTrash in our code. Both WADA and Magnus have previously mentioned this issue, but I don't see any evidence that you understand it.

So for this server case, what I would ask is, is this server giving rfc-6154 responses for the folder, and in particular does this Deleted folder get properly flagged as the \TRASH folder by the server? If so, then our problem is that we are not properly noting that.

Could we have an IMAP log for the server during startup?

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: imap.log - IMAP log when setting up Hotmail account for the first time

Here are the lines that refer to the "Deleted" folder:

2017-08-08 10:40:57.636000 UTC - [Unnamed thread 00000143C66B1000]: I/IMAP 00000143C32D1000:imap-mail.outlook.com:A:CreateNewLineFromSocket: * LIST (\HasNoChildren \Trash) "/" Deleted
2017-08-08 10:40:58.095000 UTC - [Unnamed thread 00000143C66B1000]: I/IMAP 00000143C32D1000:imap-mail.outlook.com:A:CreateNewLineFromSocket: * LSUB (\HasNoChildren \Trash) "/" Deleted

Clearly the folder called "Deleted" has the \Trash flag.

More debugging to come in the next comment.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: trash-folder-debugging.patch - Debug patch

Here is a debug patch that shows that Hotmail's deleted folder is properly recognised as trash, it has the kImapXListTrash flag and in turn we set nsMsgFolderFlags::Trash on the folder.

The debug I see is:
=== We recognised Deleted as the official trash folder!!

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch (v1, rebased)

So here we have the very first patch again, but I needed to rebase it.

So let's look at the facts one more time:

Hotmail has a trash folder called "Deleted".
Hotmail communicates that fact by giving us \Trash on that folder.
We detect that folder as the trash folder.
So far, so good?

All the code I quoted in comment #27 works fine, too. There is a unique trash folder and we store its name into mail.server.server1.trash_folder_name.
I can see that being set to "Deleted".

The problem is in the code I'm proposing to change, let's look at it again:
-  else if (mFlags & nsMsgFolderFlags::Trash && name.LowerCaseEqualsLiteral("trash"))
+  else if (mFlags & nsMsgFolderFlags::Trash && hasTrashName(name))
     rv = SetName(kLocalizedTrashName);

If the folder has the trash flag, which our folder has, and it's called "Trash", when we use the localised name. OK, so if I have a "normal" IMAP server whose trash folder is called Trash, then my Spanish user will see "Papelera" (paper basket) instead of Trash.

With Hotmail's abnormal IMAP server, the localised name is not used, since "Trash" is not matched. So we en_US users see "Deleted" as the trash name, and our Spanish friends also see "Deleted" in between the other localised folders whose names matched.

So, if we want "Deleted" to get localised, we need to admit it next to "Trash" in the code I'm trying to change.

Or said differently: Localised names are only used, if the folders match Inbox, Junk, Drafts, Sent, Trash, etc. As soon as a different name is used, like "Deleted", Thunderbird assumes that the name is already localised and doesn't touch it further. We need to break that.

Comment 32

[Jorg K (CEST = GMT+2)]

I'll put the NI on again, since this has been sitting with r? for a while now.

Comment 33

[Magnus Melin [:mkmelin]]

Thanks for debugging, but I think it's still wrong. 

I mean, if you know it has the trash flag you don't need to check the name (which you would need to to also for any other localized server folder names). But I'm not sure that's not good either, since you may want to know what folder is the trash folder. Also, I know I've had some server that had something like Deleted and also Trash, and what would happen if you also have the folder *named* (localized) Trash? That could look very strange.

Comment 34

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #33)
> Thanks for debugging, but I think it's still wrong.
So, if it's still wrong, then what is right?

> I mean, if you know it has the trash flag you don't need to check the name
> (which you would need to to also for any other localized server folder
> names).
Well, looking at:
  else if (mFlags & nsMsgFolderFlags::Trash && name.LowerCaseEqualsLiteral("trash"))
    rv = SetName(kLocalizedTrashName);
are you saying to make it just:
  else if (mFlags & nsMsgFolderFlags::Trash)
    rv = SetName(kLocalizedTrashName);
?
The result would be that if a Spanish mail provider had "Basura" (rubbish) with \Trash, then the Spanish user would see "Papelera" since we'd always localise.

I know that the patch is on thin ice, we just add Deleted since Hotmail is using it, but as WADA pointed out (comment #23), some crazy server could call the trash "Bin", and we'd then show that unlocalised. This patch is a compromise to get localised Hotmail users a decent UI without doing damage elsewhere.

> But I'm not sure that's not good either, since you may want to know
> what folder is the trash folder.
I don't follow that. "you may want to know what folder is the trash folder". The one with the special icon, typically also recognisable by it's name. I agree that needlessly forcing localised names is not such a good idea, so once again, here I'm proposing a compromise.

Note the following: The localised name for "Trash" in en-GB is "Deleted", so we're not upsetting any Brits by showing them "Trash" where they saw deleted before. Only en-US Hotmail users will see "Trash" whereas on the server they see "Deleted".

Another option would be to only force the translation of "Deleted" is the app locale isn't English ;-)

> Also, I know I've had some server that had
> something like Deleted and also Trash, and what would happen if you also
> have the folder *named* (localized) Trash? That could look very strange.
I don't follow that. If the server has two \Trash folders, when we both localise them. But that shouldn't happen.
If you have
  Trash \Trash
  Deleted (no flag)
you get (assuming Spanish): Papelera and Deleted.

Or if a Hotmail user creates a Trash folder,
  Trash (no flag)
  Deletes \Trash
that becomes Trash and Papelera. Where is the problem?

Comment 35

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #34)
> are you saying to make it just:
>   else if (mFlags & nsMsgFolderFlags::Trash)
>     rv = SetName(kLocalizedTrashName);
> ?

It's an alternative, I'd say preferable to hardcoding a check for the name Deleted. 

But you would potentially end up with two "Trash" folders.

> > But I'm not sure that's not good either, since you may want to know
> > what folder is the trash folder.
> I don't follow that. "you may want to know what folder is the trash folder".
> The one with the special icon, typically also recognisable by it's name. I

What I mean is the folder have a name on the folder and you may want to know what that is, especially when talking to other people who maybe aren't using Thunderbird.

> agree that needlessly forcing localised names is not such a good idea, so
> once again, here I'm proposing a compromise.
> 
> Note the following: The localised name for "Trash" in en-GB is "Deleted", so
> we're not upsetting any Brits by showing them "Trash" where they saw deleted
> before. Only en-US Hotmail users will see "Trash" whereas on the server they
> see "Deleted".
> 
> Another option would be to only force the translation of "Deleted" is the
> app locale isn't English ;-)
> 
> > Also, I know I've had some server that had
> > something like Deleted and also Trash, and what would happen if you also
> > have the folder *named* (localized) Trash? That could look very strange.
> I don't follow that. If the server has two \Trash folders, when we both
> localise them. But that shouldn't happen.

Only one folder has the \Trash flag. What I mean is you could have a normal folder Trash, and then the Deleted folder with the \Trash flag, Then with your patch you end up showing two Trash folders in the UI.

> If you have
>   Trash \Trash
>   Deleted (no flag)
> you get (assuming Spanish): Papelera and Deleted.
> 
> Or if a Hotmail user creates a Trash folder,
>   Trash (no flag)
>   Deletes \Trash
> that becomes Trash and Papelera. Where is the problem?

You're assuming the translation is not English. For en-US you get Trash and Trash.

Comment 36

[Jorg K (CEST = GMT+2)]

OK, I'll do a patch where we check the locale.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch (v2)

OK, here with a locale check. So in the unlikely case that the user has a Trash folder and a Deleted \Trash folder, I don't turn the Deleted folder into Trash any more.

So in an English app, the Deleted folder is not shown as Trash, so all the US Hotmail users see what they are used to. For non-English users we assume that we need to use the localised name for Trash and Deleted if they have the trash flag.

Zibi, can you please check the code and tell me whether there's a better way to check for non-English. I wanted to avoid Substr, etc. since most of that doesn't work on nsCString, you need an nsAutoCString for that.

Comment 38

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8895094 [details] [diff] [review]
1320191-deleted-folder.patch (v2)

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

hah, I don't know if there's a simpler way, but there's definitely a better way :)

You can get use `LocaleService::GetAppLocaleAsLangTag` to just get the first from the list (since you don't care about the fallback).

Then:

```
static bool
nonEnglishApp()
{
  nsAutoCString locale;
  LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);
  
  Locale loc = LocaleService::Locale(locale, true); // take a range
  Locale enUSLoc = LocaleService::Locale("en", true); // take a range

  return loc.LanguageMatches(enUSLoc);
}
```

This may look like more code, but I don't think it actually is that much more, while it definitely is way more bullet proof.

Comment 39

[Jorg K (CEST = GMT+2)]

Thanks, but I can't get this to compile. 'Locale' seems to be private.

LocaleService.h:
private:
  /**
   * Locale object, a BCP47-style tag decomposed into subtags for
   * matching purposes.
   *
   * If constructed with aRange = true, any missing subtags will be
   * set to "*".
   */
  class Locale

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch (v2b)

OK, slightly improved locale handling, still now that Zibi suggested, but that doesn't compile, well, I wrestled with it for a while.

'Locale' doesn't appear to be available and LocaleService::Locale() seems to be called 'LocaleService::Locale::Locale':
https://dxr.mozilla.org/comm-central/rev/47248637eafa9a38dade8dc3aa6c4736177c8d8d/mozilla/intl/locale/LocaleService.cpp#725

Anyway, Kent and Magnus need to decide whether they like this approach or not, regardless of the most elegant way to check for English.

Comment 41

[Jorg K (CEST = GMT+2)]

Typo: still NOT that Zibi suggested. Zibi, what do you say?

Comment 42

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ooh, sorry! Yeah. We'll need bug 1349377 first :(

The new patch looks ok, except that I'd change it to 0-2 and "en" to match "en" as well :)

return !Substring(locale, 0, 3).EqualsLiteral("en-");

Comment 43

[Jorg K (CEST = GMT+2)]

OK, I'll do that after the review to avoid more noise. Thanks again.

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8895116 [details] [diff] [review]
1320191-deleted-folder.patch (v2b)

No action from Kent in three months.

Comment 45

[:aceman]

Comment on attachment 8895116 [details] [diff] [review]
1320191-deleted-folder.patch (v2b)

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3386,5 @@
> +{
> +  nsAutoCString locale;
> +  mozilla::intl::LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);
> +
> +  return !Substring(locale, 0, 3).EqualsLiteral("en-");

I would make it locale.equals("en") || StringBeginsWith("en-"). Just checking the first 2 chars for 'en' may not be enough for some future 3 letter locale starting with en, that is NOT English. OR actually use zibi's version after bug 1349377.

@@ +3403,5 @@
> +{
> +  // Microsoft calls the folder "Deleted". If the application is non-English,
> +  // we want to use the localised name instead.
> +  return MsgLowerCaseEqualsLiteral(name, "trash") ||
> +         (MsgLowerCaseEqualsLiteral(name, "deleted") && nonEnglishApp());

Do you need the function twice? Can you have one version and upgrade/downgrade the string at other caller?

@@ +3735,5 @@
>      else if (MsgLowerCaseEqualsLiteral(escapedName, "unsent%20messages"))
>        uri += "Unsent%20Messages";
>      else if (MsgLowerCaseEqualsLiteral(escapedName, "drafts"))
>        uri += "Drafts";
> +    else if (hasTrashName(escapedName))

Actually I wouldn't skip this if app IS English. If folder is Deleted we won't consider it a 'trash' folder, but we probably want to. Or do I misunderstand this? But we need special care if user has both trash and Deleted folders. We probably don't want to have same uri for both.

@@ +3778,5 @@
>      {
>        flags |= nsMsgFolderFlags::Inbox;
>        SetBiffState(nsIMsgFolder::nsMsgBiffState_Unknown);
>      }
> +    else if (hasTrashName(name))

Actually I wouldn't skip this if app IS English. If folder is Deleted we won't consider it a 'trash' folder, but we probably want to. Or do I misunderstand this? But we need special care if user has both trash and Deleted folders. We probably don't want to flag both.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch (v3)

I got rid of the questionable hunks, I'm not sure what nsMsgDBFolder::AddSubfolder() is trying to do with the trash folder.

As for having the same functions for two types, we'd go:
  template <class T>
  static bool
  hasTrashName(const T& name)
but we don't need this now. So just for the record.

Comment 47

[:aceman]

Do we run through nsMsgDBFolder::AddSubfolder() when folders are detected on the server? Or is it called only when the user creates new folders via TB? In that latter case dropping the hunks could be OK.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1320191-deleted-folder.patch (v3b)

Sorry about the noise, I messed up the previous patch, putting the negation into the wrong function.

With the patch and hacking nonEnglishApp() to return "true", running on a Hotmail account, I see a "Trash" folder (localised en-US) instead of the original "Deleted" folder. That works without the two hunks I removed.

I can see two calls
=== nsMsgDBFolder::AddSubfolder Trash
=== nsMsgDBFolder::AddSubfolder Unsent Messages
so I think we don't need the two hunks.

Comment 49

[:aceman]

(In reply to Jorg K (GMT+2) from comment #48)
> I can see two calls
> === nsMsgDBFolder::AddSubfolder Trash
> === nsMsgDBFolder::AddSubfolder Unsent Messages
> so I think we don't need the two hunks.

So do you see this on each TB startup?

Comment 50

[Jorg K (CEST = GMT+2)]

Yes.

Comment 51

[:aceman]

When inside AddSubfolder, does Trash already have the nsMsgFolderFlags::Trash flag?

Comment 52

[Jorg K (CEST = GMT+2)]

Can you ask all the questions at the same time :-(
=== nsMsgDBFolder::AddSubfolder Trash
=== 104
=== nsMsgDBFolder::AddSubfolder Unsent Messages
=== 804

Yes. 4 is Mail, 100 is Trash and 800 is Queue (in hex), see nsMsgFolderFlags.idl.

Comment 53

[:aceman]

(In reply to Jorg K (:jorgk, GMT+1) from comment #48)
> I can see two calls
> === nsMsgDBFolder::AddSubfolder Trash
> === nsMsgDBFolder::AddSubfolder Unsent Messages
> so I think we don't need the two hunks.

What is the URI of the "Deleted" folder? If you remove the hunks (at least the one generating the URI), will it end in /Deleted? Is it OK for other code if the Trash folder's URI doesn' end in /Trash ? Yeah meaybe you can later set any folder to be the Trash, but for the initial settings. If we always lookup the Trash folder via the flag, things could work.

Comment 54

[Jorg K (CEST = GMT+2)]

As I said, nsMsgDBFolder::AddSubfolder doesn't appear to be called with "Deleted", so I cannot answer the question. Reading the code, without the additional hunks it would end in /Deleted.

Note that we don't have any complaints from Hotmail/Live/Outlook users that their trash isn't working. The only complaint we have is that there is a non-localised "Deleted" folder in amongst the other folders with localised names. So with the patch we use the localised trash name also if the folder has the trash flag and is called "Deleted". Other working behaviour isn't changed.

What's the remaining doubt?

Comment 55

[:aceman]

Comment on attachment 8924731 [details] [diff] [review]
1320191-deleted-folder.patch (v3b)

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

I just wonder that we want to localize the Deleted folder as if it was Trash, but do not apply all the other handling to it (like setting the trash flag and it not running through AddSubfolder()), as if it wasn't Trash.

But if you have a server with the folder and it appears to work for you, we can try it.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfa1e1483a5e
Cater for Outlook's/Hotmail's 'Deleted' folder. r=aceman

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8924731 [details] [diff] [review]
1320191-deleted-folder.patch (v3b)

Let's get this uplifted to see what users say. This can't be uplifted to TB 52 due to the mozilla::intl::LocaleService stuff which doesn't exist in mozilla52.

Comment 58

[Jorg K (CEST = GMT+2)]

Beta (TB 58):
https://hg.mozilla.org/releases/comm-beta/rev/760c2d834c8225c22223ac6d09226f80707560a8

Comment 59

[Jorg K (CEST = GMT+2)]

Reporter, can you please try this in TB 58 beta2 available here:
https://www.mozilla.org/en-US/thunderbird/channel/

It's taken a year to fix, but we finally got there.