Open Bug 1359410 Opened 3 years ago Updated 4 days ago

TB 52.0.1 doesn't accept accounts order in mail.accountmanager.accounts

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED
Thunderbird 55.0

People

(Reporter: worm6666, Assigned: protz)

References

(Blocks 2 open bugs)

Details

(Keywords: addon-compat, Whiteboard: [add-on: Folderpane Tools][addon: Manually sort folders])

Attachments

(3 files, 9 obsolete files)

120.80 KB, image/jpeg
Details
44.04 KB, image/jpeg
Details
26.85 KB, patch
Details | Diff | Splinter Review
Attached image Accounts order
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

installed latest update from within TB


Actual results:

sort order of accounta is invalid = doesn't respect values order in variable "mail.accountmanager.accounts"


Expected results:

after any change of variable values (acconts identifiers) - the order stays alphabetical with local accounts at the end
sorry, the Expected results:

should be: TB should sort accounts in order like defined in the variable "mail.accountmanager.accounts"
It's not as easy as that, but there is an add-on that will allow you to re-order accounts: "Manually sort folder".
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
This add-on I use for years... Direct for that case. But now it doesn't work anymore... Or better to say - it works, but not like before.

Before upgrade I had this order:

email1
local1
local2
email2
email3

Now, it is hard sorted into two groups - mailserver and local, and the plugin can arrange only mailserver accounts.

For example:

email2
email1
email3
local1
local2

But what is weird, the variable "mail.accountmanager.accounts" has no ifluence to the order of accounts... :-(

Probably that is the source of this problem... ???
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Thunderbird doesn't offer reordering of accounts and we don't guarantee that add-ons will keep working forever. So this bug is invalid.

That said, we can have our accounts expert the the add-on author look at the problem.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago3 years ago
Flags: needinfo?(jonathan.protzenko)
Flags: needinfo?(acelists)
Resolution: --- → INVALID
Well, it seems that it is a error in another add-on: "Folderpane Tools"

"Manually sort folder" doesn't allow to mix mailserver and local accounts order...
Thanks for letting us know.

https://addons.mozilla.org/en-US/thunderbird/addon/folderpane-tools/ says it's not working for TB 52.
Flags: needinfo?(jonathan.protzenko)
Flags: needinfo?(acelists)
Whiteboard: [add-on: Folderpane Tools]
(In reply to worm6666 from comment #3)
> This add-on I use for years... Direct for that case. But now it doesn't work
> anymore... Or better to say - it works, but not like before.
> 
> Before upgrade I had this order:
> 
> email1
> local1
> local2
> email2
> email3

Which TB version was this?
 
> Now, it is hard sorted into two groups - mailserver and local, and the
> plugin can arrange only mailserver accounts.

> But what is weird, the variable "mail.accountmanager.accounts" has no
> ifluence to the order of accounts... :-(

It does influence the order inside the groups. E.g. you can get
email2
email3
email1
local1

But this change was done in Thunderbird 19 (bug 749200) so I wonder why you report it only now.


This is not INVALID, but at worst WONTFIX as we ignore the order intentionally (since bug 749200), see e.g. bug 896511.

But now I think if we can support ordering in some way (make the addons work).
Status: RESOLVED → REOPENED
Depends on: 749200
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: INVALID → ---
Well, I do updates regularly and sometimes I check it from "Help -> About" menu, and I had perfect fitted email client to My needs before last update. Also I reported this first time when I spotted some visual change in accounts order after the latest update - I tested plugins to ON/OFF (reinstall, uninstall,...), modify system variables (about:config), all without success.

The version before was: 45.8.0 (20170305125302)

What can I mention more is, that when I reinstall OS or move to new one, I use MozBackup-1.5.1 to transfer all things to new PC, and before backup and recover I do always update so both PC's have same version of TB. Never had some troubles...

I don't want bother somebody, I had only feeling to I should report this... It is not so comfortable for Me when managing messages, but I can live with it. :-)
Attached image Updates history
Attachment #8861416 - Attachment filename: 2017-04-25_15-50-00.jpg → Accounts order
Attachment #8861416 - Attachment description: 2017-04-25_15-50-00.jpg → Accounts order
Attachment #8861416 - Attachment filename: Accounts order → 2017-04-25_15-50-00.jpg
Attached patch patch (obsolete) — Splinter Review
This patch adds optional support for reordering accounts, as it existed before bug 749200. If the new pref is set, then order of accounts in mail.accountmanager.accounts is obeyed again.

Test this by setting the pref mail.accountmanager.accounts.ordered and changing order of the accound ids in mail.accountmanager.accounts.

This patch has no effect by default and should only allow users to manually sort the accounts or is intended to be activated by addons. There is no UI in base TB yet, which is only requested in bug 244347.

This feature is for powerusers and there seems high demand for it (see bug 244347) and there are multiple addons for this (more or less broken after bug 749200).
Attachment #8862594 - Flags: ui-review?(richard.marti)
Attachment #8862594 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8862594 [details] [diff] [review]
patch

Looks good
Attachment #8862594 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8862594 [details] [diff] [review]
patch

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

I suppose. Though in general I don't like adding things for add-ons only. If it's in high demand, implement reordering in core.
Attachment #8862594 - Flags: review?(mkmelin+mozilla) → review+
I understand. This is a stop-gap solution until bug 244347 gets implemented. But even then, for that bug we will need a toggle (pref) whether to use the automatic ordering or if the user wants to be in charge. So the pref added here could be it. Until then, salvage the implementations already done in addons and let them work again.
Assignee: nobody → acelists
Blocks: 244347
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Product: Thunderbird → MailNews Core
Whiteboard: [add-on: Folderpane Tools] → [add-on: Folderpane Tools][addon: Manually sort folders]
Target Milestone: --- → Thunderbird 55.0
Version: 52 Branch → 52
Ccing protz and ratty as the maintainers of the "Folderpane Tools" and "Manually sort folders" addons who may be interested in using this pref to make the addons work again.
Flags: needinfo?(philip.chee)
Flags: needinfo?(jonathan.protzenko)
Keywords: addon-compat
Aceman asked me to try this before landing, so let's clear the flag for now.
Flags: needinfo?(jorgk)
Keywords: checkin-needed
Comment on attachment 8862594 [details] [diff] [review]
patch

I don't understand why we're doing this.

"Manually sort folders" 1.1.1 manages to move Local Folders and News to the top without a problem.

Aceman, please try the add-on yourself.

Folderpane Tools is deprecated, https://addons.mozilla.org/en-US/thunderbird/addon/folderpane-tools/ says:
This extension is no longer being maintained. Please use this extension as an alternative:
"Manually sort folders".

Why don't we talk to the author of "Manually sort folders" first to see whether they have any requirement and then implement that requirement.

In this current form, the patch doesn't help anyone and adds yet another undocumented preference. I hope we don't expect the user to use it and reorder the accounts himself in mail.accountmanager.accounts.
Flags: needinfo?(jorgk)
Attachment #8862594 - Flags: feedback-
(In reply to Jorg K (GMT+2) from comment #16)
> "Manually sort folders" 1.1.1 manages to move Local Folders and News to the
> top without a problem.

Yes, it moves a *single* account to the top via making it the default. I haven't found a way to put Local, then News, then others, in this order. Also reading the "additional information" block in the settings of the addon seems to imply it can't do everything.
Those Local or News accounts are not suited to be the default in TB. Using the standard APIs the account manager would prevent that. But the addons is manipulating the prefs directly, bypassing the account manager and setting the prefs to basically illegal values. I would be wary using that addon, it gets you into unsupported territory. But yes, using any addon means that :) But in this case it does not only extend existing features, it violates internal assumptions of existing features.
You can see it even offers to make a IM/chat account the default, which is totally bogus.
Also those hacks may break apart after bug 880602 is done, which has almost landed. Maybe you could help me debugging the remaining Windows-only crash there.
 
> Aceman, please try the add-on yourself.

Ok, I did. I only checked the account reordering features.
 
> Folderpane Tools is deprecated,
> https://addons.mozilla.org/en-US/thunderbird/addon/folderpane-tools/ says:
> This extension is no longer being maintained. Please use this extension as
> an alternative:
> "Manually sort folders".

Also "manually sort folder" does not look more maintained. It produces JS warnings in the Error console and also uses Application.* which spews deprecation in the Error console. It will stop working when we remove STEEL.
 
> Why don't we talk to the author of "Manually sort folders" first to see
> whether they have any requirement and then implement that requirement.

Sure, I asked them now if this is all that is needed. If something more is needed we can provide it in a new bug. The bug here is clearly defined to sort using the mail.accountmanager.accounts pref.

> In this current form, the patch doesn't help anyone and adds yet another
> undocumented preference.

The patch does enable features that the addon does NOT and does not introduce dangerous hacks the addon does. 

> I hope we don't expect the user to use it and
> reorder the accounts himself in mail.accountmanager.accounts.

At least the reporter knows how to do that.
I'm happy to take patches to Manually Sort Folders that make it more in line with what Thunderbird expects. I know that there have been more restrictions over time; it used to be the case that Thunderbird was happy with whatever account order we would throw at it.

The addon has a steady user base that was around 30k ADU last I checked. I just haven't had to fix anything: it kept working. If you or someone else can provide patches to make it no longer depend on STEEL, then I'd most gratefully merge them in :)
Flags: needinfo?(jonathan.protzenko)
Duplicate of this bug: 1587088

Sorry for reviving an old thread. I updated the addon for Thunderbird 68 and a lot of people have reported that the behavior is not the one they initially wanted.

Is it correct that all I need to do is:

  • stop overriding the default account
  • flip the new boolean pref to true to true
  • reorder the account order in the "accounts" configuration variable (the one I'm already modifying)?

Thanks,

Jonathan

Flags: needinfo?(philip.chee)

I don't think the "new pref" actually exists yet does it? There is a patch on this bug to add it but I don't believe it has actually been accepted into the released code...

(In reply to Jonathan Protzenko [:protz] from comment #20)

Is it correct that all I need to do is:

  • stop overriding the default account
  • flip the new boolean pref to true to true
  • reorder the account order in the "accounts" configuration variable (the one I'm already modifying)?

Yes, this is correct.
Your addon will set the pref mail.accountmanager.accounts.ordered to true and allow the user to reorder the accounts in the dialog you already have, saving the order in mail.accountmanager.accounts.

The pref mail.accountmanager.accounts.ordered didn't land in Thunderbird yet as there was no user of it. So you need to apply the patch in this bug, modify your addon to use it.

As described in comment 17, the addon couldn't achieve any order of the accounts, only move one account to the top (setting it as default). Using the new pref, you should get the benefit of being able to achieve any order as then TB will skip its internal forced sorting of accounts.

If you report back that it works and does what is needed (changing the order using your addon), we can land the patch in TB.

Protz, any luck with updating the addon?

(In reply to Jonathan Protzenko [:protz] from comment #18)

If you or someone else
can provide patches to make it no longer depend on STEEL, then I'd most
gratefully merge them in :)

You just convert Application.platformIs* calls to the ones in AppConstants.platform.
Calls to Application.prefs.get() can be replaced by Services.prefs.get*Pref().
Application.console.log() can be replaced by console.log.
Also Application.restart is now BrowserUtils.restartApplication(); from "resource://gre/modules/BrowserUtils.jsm" .

I was able to confirm the patch works (I've rebased it on the latest revision of comm-central). I need to update my addon to use MailExtensions so I'm thinking of adding new MailExtension API entry points while I'm at it, for this feature.

Is it ok to submit a combined patch set?

I think so, thanks.

Here's an extension of your patch, with suitable MailExtension APIs. I have a rewrite of Manually Sort Folders that uses these APIs for sorting the accounts, so things seem to be working fine.

Some remarks:

  • I could not force a rebuild of the folder pane, so now Thunderbird needs to be restarted for changes to be taken into account; previously, it was enough to do:
          // Force a rebuild of the account tree.
          let mainWindow = Services.wm.getMostRecentWindow("mail:3pane");
          mainWindow.gFolderTreeView.mode = mainWindow.gFolderTreeView.mode;

but this no longer seems to work. Any advice?

  • There were some issues with the schema description, for which I filed bug 1606570, but it's non-blocking
  • I designed the MailExtension API to be nicer than just a raw wrapper around the prefs. Let me know what you think.
  • I had to set all of the API functions to be async but I don't understand why; is this a requirement of all WebExtensions?

Thanks,

Jonathan

Assignee: acelists → jonathan.protzenko
Attachment #8862594 - Attachment is obsolete: true
Attachment #9118282 - Flags: review?(acelists)
Comment on attachment 9118282 [details] [diff] [review]
enable custom sorting of the folder pane + add suitable MailExtension APIs

I think WE API's should go Geoff's way. folderUtils.jsm  and mailnews.js look OK to me, r+ on those, or Aceman might want to take another look.
Attachment #9118282 - Flags: review?(geoff)

Oops, the hunks in folderUtils.jsm and mailnews.js already had r=mkmelin in attachment 8862594 [details] [diff] [review]. But you lost the comment in the C++ file.

Here's a patch with the C++ comment reinstated. The r? is only for the extra MailExtensions.

Geoff: the API is crafted to i) perform proper input validation to avoid inconsistent states and ii) to prevent clients from doing something nonsensical, e.g. leaving accounts.ordered = false but still setting the order of accounts (which will not produce the intended results).

Hope this helps.

Attachment #9118282 - Attachment is obsolete: true
Attachment #9118282 - Flags: review?(geoff)
Attachment #9118282 - Flags: review?(acelists)
Attachment #9118284 - Flags: review?(geoff)
Attachment #9118284 - Flags: feedback?(acelists)
Comment on attachment 9118284 [details] [diff] [review]
enable custom sorting of the folder pane + add suitable MailExtension APIs

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

I have some changes to the API design I'd like you to make. There's also some formatting problems which `mach eslint --fix path/to/file` should be able to take care of (mostly missing curly brackets).

::: mail/components/extensions/parent/ext-accounts.js
@@ +71,5 @@
>            }
>            return null;
>          },
> +        async hasCustomSort() {
> +          return Services.prefs.getBoolPref("mail.accountmanager.accounts.ordered");

I don't think this is necessary. If the list function was changed to use allAccountsSorted then extension UI can display the accounts in the current order. (Bonus: any extension could display accounts in the current order.) Surely the user is more interested in that than whether there is a custom order or not.

@@ +74,5 @@
> +        async hasCustomSort() {
> +          return Services.prefs.getBoolPref("mail.accountmanager.accounts.ordered");
> +        },
> +        async clearCustomSort() {
> +          Services.prefs.setBoolPref("mail.accountmanager.accounts.ordered", false);

I'd rename setCustomSort to something like setOrder, make the argument optional, and then if it isn't supplied reset the pref. Thus eliminating this function too.

@@ +78,5 @@
> +          Services.prefs.setBoolPref("mail.accountmanager.accounts.ordered", false);
> +        },
> +        async setCustomSort(accountIds) {
> +          // Some input validation.
> +          let unique = {};

Using a Set to track this would be tidier.

@@ +84,5 @@
> +          for (let account of fixIterator(
> +            MailServices.accounts.accounts,
> +            Ci.nsIMsgAccount
> +          )) {
> +            let i = accountIds.indexOf(account.key);

accountIds.includes(account.key)
Attachment #9118284 - Flags: review?(geoff)

Thanks for the good review. I'll submit an updated version shortly.

This patch takes into account all review comments and I was able to confirm that the API works and can be driven from an addon.

I have documented that this function is really two APIs in one, but that the behavior differs on whether the argument has length 0 or not.

ESLint said: ✖ 0 problems (0 errors, 0 warnings)

My SSH access to push to comm-central expired years ago, so if the patch looks good, I'll need someone to put in the landing queue for me.

Thanks,

Jonathan

Attachment #9118284 - Attachment is obsolete: true
Attachment #9118284 - Flags: feedback?(acelists)
Attachment #9118353 - Flags: review?(geoff)
Attachment #9118353 - Flags: feedback?(acelists)
Blocks: 1606664
Comment on attachment 9118353 [details] [diff] [review]
updated patch following review comments

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

I've played with this a little bit. It's a real shame that a restart is required to update the order. It shouldn't be too difficult to re-order m_accounts when the prefs change, but that's probably a story for another bug.
Other than that, almost there!

::: mail/components/extensions/parent/ext-accounts.js
@@ +68,5 @@
>            }
>            return null;
>          },
> +        async setOrder(accountIds) {
> +          if (accountIds.length == 0) {

Set optional: true in the JSON file, and accountIds will be null if not specified. That's better than an empty array.

@@ +81,5 @@
> +            for (let account of fixIterator(
> +              MailServices.accounts.accounts,
> +              Ci.nsIMsgAccount
> +            )) {
> +              if (!accountIds.includes(account.key)) {

Nit: flip this if/else around.

@@ +102,5 @@
> +            // Don't forget to preserve the accounts that are not returned by list
> +            // above.
> +            Services.prefs.setStringPref(
> +              "mail.accountmanager.accounts",
> +              accountIds.concat(missing).join(",")

I think it would be nice to order any missing accounts in the default way before adding them back to the list.

::: mailnews/base/util/folderUtils.jsm
@@ +177,5 @@
>      accountList = accountList.filter(a => a.incomingServer.type != "im");
>    }
>  
> +  if (Services.prefs.getBoolPref("mail.accountmanager.accounts.ordered"))
> +    return accountList;

Nit: still missing curly brackets.
Attachment #9118353 - Flags: review?(geoff)

Thanks for the review.

I think it would be nice to order any missing accounts in the default way
before adding them back to the list.

The API demands that all accounts returned by browser.accounts.list() be present in accountIds; this leaves in the "missing" accounts list only those that have been discarded by accounts.list(), i.e. IM accounts, which don't appear in the folder pane. Why does the order matter?

Nit: still missing curly brackets.

ESLint seemed happy with this file. Is there another set of guidelines that I should follow?

I'll fix the other two points.

ESLint seemed happy with this file. Is there another set of guidelines that I should follow?

Ah that was in the other file and this came from Aceman's original patch. I can re-format that file but I suspect it'll generate a big diff, so I'll just add curly braces.

You were right that setOrder accepts any subset of accounts -- I think I used to enforce that all accounts returned by list() be passed to setOrder, but this patch no longer does.

All other things have been fixed.

Attachment #9118353 - Attachment is obsolete: true
Attachment #9118353 - Flags: feedback?(acelists)
Attachment #9118447 - Flags: review?(geoff)
Attachment #9118447 - Flags: feedback?(acelists)
Comment on attachment 9118447 [details] [diff] [review]
hopefully final version of the patch

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

Thank you, looks like nice accessor API to the mail.accountmanager.accounts.ordered pref.

::: mail/components/extensions/parent/ext-accounts.js
@@ +68,4 @@
>            }
>            return null;
>          },
> +        async setOrder(accountIds) {

Can you check accountIds is a JS array so that TB does not throw JS errors later on?

@@ +79,5 @@
> +
> +          // Some input validation.
> +          const unique = new Set();
> +          const missing = [];
> +          for (let account of fixIterator(

Could this also check if all ids in accountIds actually exist in MailServices.accounts.accounts ?
And also that accountIds does not contain less account ids than there exist in MailServices.accounts.accounts ?
Basically accountIds must be a permutation of ids in MailServices.accounts.accounts, not a smaller subset. Otherwise accounts will get lost from the profile.

@@ +103,5 @@
> +          );
> +          // Don't forget to preserve the accounts that are not returned by list
> +          // above.
> +          Services.prefs.setStringPref(
> +            "mail.accountmanager.accounts",

I'm not sure how safe this is as the pref will be saved again by the account manager backend when accounts are manipulated and that will have its own idea of the order (stored in m_accounts of nsMsgAccountManager.cpp).

You would need to force nsMsgAccountManager::LoadAccounts() to reread the account list, but that does not look safe again. So restarting TB seems like the easiest way. Do you force TB restart from your addon after setOrder() is called? Could that restart be done in this WE function so that all addons inherit it automatically?

::: mail/components/extensions/schemas/accounts.json
@@ +42,5 @@
> +      },
> +      {
> +        "name": "setOrder",
> +        "type": "function",
> +        "description": "This function has two behaviors. When no argument is passed, it resets the account sort order to Thunderbird's default. When passed a list of account IDs (as returned by the list method), it override's Thunderbird default account order with the one provided.",

"overrides" ?
Attachment #9118447 - Flags: feedback?(acelists) → feedback+

(In reply to Jonathan Protzenko [:protz] from comment #36)

You were right that setOrder accepts any subset of accounts -- I think I used to enforce that all accounts returned by list() be passed to setOrder, but this patch no longer does.

OK, I see you accumulate accounts not in the accountIds array in the 'missing' array and then tack them on the tail of the list. That could work, just document it. But still check accountIds for invalid account ids.

(In reply to Jonathan Protzenko [:protz] from comment #26)

Some remarks:

  • I could not force a rebuild of the folder pane, so now Thunderbird needs to be restarted for changes to be taken into account; previously, it was enough to do:
    // Force a rebuild of the account tree.
    let mainWindow = Services.wm.getMostRecentWindow("mail:3pane");
    mainWindow.gFolderTreeView.mode = mainWindow.gFolderTreeView.mode;
    but this no longer seems to work. Any advice?

Reading 'set mode(aMode)' function, it is not apparent why this woudn't work. There is no check if you are setting the same mode again, preventing rebuild.

Can you try mainWindow.gFolderTreeView._rebuild() just to see if that works?

(In reply to Geoff Lankow (:darktrojan) from comment #33)

I've played with this a little bit. It's a real shame that a restart is
required to update the order. It shouldn't be too difficult to re-order
m_accounts when the prefs change, but that's probably a story for another
bug.

It does look difficult using existing methods. Tearing down all the accounts (firing all the observers) and then re-reading the pref seems to be very unsafe.

I think we would have to add a special method for this, that just silently re-reads the pref and reorders m_accounts internally, without exposing that this happened to any UI/listeners/observers.

@@ +102,5 @@

  •        // Don't forget to preserve the accounts that are not returned by list
    
  •        // above.
    
  •        Services.prefs.setStringPref(
    
  •          "mail.accountmanager.accounts",
    
  •          accountIds.concat(missing).join(",")
    

I think it would be nice to order any missing accounts in the default way
before adding them back to the list.

No, the "default way" is only ordered in the UI (allAccountSorted() function). The mail.accountmanager.accounts pref contains the accounts basically in the order they were created and we do not need to touch that. It is correct if here we append the missing accounts in the same order they already are in mail.accountmanager.accounts. The setOrder() function just pops some out to the front of the list, but the rest should stay in the order they are (like a stable sort).

(In reply to :aceman from comment #37)

Could this also check if all ids in accountIds actually exist in
MailServices.accounts.accounts ?
And also that accountIds does not contain less account ids than there exist
in MailServices.accounts.accounts ?
Basically accountIds must be a permutation of ids in
MailServices.accounts.accounts, not a smaller subset. Otherwise accounts
will get lost from the profile.

Also check if any passed in account id isn't in the list twice.

I wonder if we should better pass the list to some method of nsIMsgAccountManager that validates it properly and re-initializes m_accounts instead of having some random JS code (albeit in the WE api) manipulate the mail.accountmanager.accounts pref directly possibly clashing with account manager's idea of it.

I'll fix the documentation and force a restart after setting the accounts.

The code already errors out if i) some accountId is duplicated or if ii) some accountId is invalid. After the loop has run, unique contains the set of all accounts that have been found in accountIds and that are valid. If there's less elements in the set than accountsIds.length, either there's a duplicate or some accountId is invalid.

Setting this in C++ seems like a tremendous amount of work that will be less maintainable in the future, so I'm inclined to just stick with the WebExtension API.

Thanks for the review!

For the record I tried _rebuild as well and couldn't get it to work. I couldn't figure out why, but we can always improve the API later and skip the restart.

(In reply to Jonathan Protzenko [:protz] from comment #41)

Setting this in C++ seems like a tremendous amount of work that will be less maintainable in the future, so I'm inclined to just stick with the WebExtension API.

I think there could be both, the WE API allowing to specify only some accounts and then fills in the rest, also manages the mail.accountmanager.accounts.ordered pref, and then the C++ method that takes the final list, does some sanity checks (which then wouldn't need to be in the WE API) and reorders accounts in the nsIMsgAccountManager, saves the pref and thus avoids the need of the restart.

(In reply to Jonathan Protzenko [:protz] from comment #42)

For the record I tried _rebuild as well and couldn't get it to work. I couldn't figure out why, but we can always improve the API later and skip the restart.

May depend on what exactly you have tried. Changing the order of the accounts couldn't work (rebuild even if working didn't change the appearence) as the account manager didn't change its internal order of the accounts after you just call setOrder() in the WE API. Only the change to mail.accountmanager.accounts.ordered value could be picked up by the tree rebuild.

Attached patch with the feedback from aceman (obsolete) — Splinter Review
Attachment #9118447 - Attachment is obsolete: true
Attachment #9118447 - Flags: review?(geoff)
Attachment #9119118 - Flags: review?(geoff)
Attachment #9119118 - Flags: feedback+

I've added a patch taking into account your comments. I would suggest landing this first, then I'm happy to open a new bug for the C++ side of things that will eventually allow skipping the restart.

Thanks,

Jonathan

Attachment #9119118 - Attachment is patch: true
Comment on attachment 9119118 [details] [diff] [review]
with the feedback from aceman

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

::: mail/components/extensions/parent/ext-accounts.js
@@ +60,4 @@
>      return {
>        accounts: {
>          async list() {
> +          return allAccountsSorted()

Why are you changing this to return the sorted list?
Wouldn't both variants be useful to addons?

::: mail/components/extensions/schemas/accounts.json
@@ +42,5 @@
> +      },
> +      {
> +        "name": "setOrder",
> +        "type": "function",
> +        "description": "This function has two behaviors. When no argument is passed, it resets the account sort order to Thunderbird's default. When passed a list of account IDs (as returned by the list method), it overrides Thunderbird default account order with the one provided, then restarts Thunderbird.",

You still didn't document that you can pass less accounts than the full list.

(In reply to :aceman from comment #46)

Why are you changing this to return the sorted list?
Wouldn't both variants be useful to addons?

An unsorted list can happen to be sorted, so what's the use case for getting an explicitly unsorted list?

You still didn't document that you can pass less accounts than the full list.

I think that's a corner case and not the intended usage for the API, so I'd rather keep the documentation concise rather than go and explain the operational semantics of the API in the case where not all accounts are passed.

An unsorted list can happen to be sorted, so what's the use case for getting an explicitly unsorted list?

Agreed.

Comment on attachment 9119118 [details] [diff] [review]
with the feedback from aceman

Magnus and I just talked about this and we have decided that a WebExtension should not have the ability to restart Thunderbird. You'll need to either fix the underlying problem or tell the user to restart Thunderbird after making the changes.

Since your earlier patch is basically the same thing without the restart, I'll review it instead.
Attachment #9119118 - Flags: review?(geoff) → review-
Comment on attachment 9118447 [details] [diff] [review]
hopefully final version of the patch

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

I'm happy with this as long as you reword the documentation.

::: mail/components/extensions/parent/ext-accounts.js
@@ +68,4 @@
>            }
>            return null;
>          },
> +        async setOrder(accountIds) {

@aceman It's either an array of strings or null. A check has already been performed by the WebExtensions mechanism by this point.

::: mail/components/extensions/schemas/accounts.json
@@ +42,5 @@
> +      },
> +      {
> +        "name": "setOrder",
> +        "type": "function",
> +        "description": "This function has two behaviors. When no argument is passed, it resets the account sort order to Thunderbird's default. When passed a list of account IDs (as returned by the list method), it override's Thunderbird default account order with the one provided.",

Too wordy. Something like "Sets the order of accounts in the user interface. A restart is required to complete this operation." is all you need.

@@ +48,5 @@
> +        "parameters": [
> +          {
> +            "name": "accountIds",
> +            "type": "array",
> +            "optional": true,

"description": "A list of account identifiers as returned by the list method. Skip this argument to reset the order to the default."
Attachment #9118447 - Flags: review+
Attachment #9118447 - Attachment is obsolete: false

Magnus and I just talked about this and we have decided that a WebExtension should not have the ability to restart Thunderbird. You'll need to either fix the underlying problem or tell the user to restart Thunderbird after making the changes.

I'm surprised by this decision. Looking at https://thunderbird-webextensions.readthedocs.io/en/latest/, my understanding is that since Thunderbird is overhauling its addon system, the Thunderbird team is committing to providing suitable API entry points so that existing addons can continue to provide equivalent functionality. There's even a suggestion to file a bug, so that APIs can be added.

My understanding is also that APIs should provide well-designed functionality. Here, :aceman explicitly pointed out the danger of not restarting Thunderbird:

(In reply to :aceman from comment #37)

I'm not sure how safe this is as the pref will be saved again by the account manager backend when accounts are manipulated and that will have its own idea of the order (stored in m_accounts of nsMsgAccountManager.cpp).

Since not restarting Thunderbird is dangerous, two options have been discussed:
i) an extensive fix requiring work on the C++ and JS sides, and
ii) a safe workaround based on a restart (see comment #37).

The first one is out of scope for me: I am no longer a Thunderbird module owner, and I'm only trying to keep the 4-th most popular addon (200,000 active daily users) alive, since I'm getting emails in my inbox every day. I have very limited cycles to spend on this (I am no longer in grad school!).

The second one seems like a fine temporary thing: it's not a general-purpose restart API, but a temporary quirk of behavior that will be fixed the day the Thunderbird team overhauls the setOrder API to no longer require a restart.

Declining both i) and ii) means that you're asking the addon to be aware of a severe flaw in the API, and as such, to display a big large banner after every change saying "everything will be unsafe unless you restart Thunderbird now". While I understand that you don't want to expose restart functionality to addons, I would very strongly advocate for ii) to be implemented for now, and for this restriction to be lifted once a Thunderbird-side fix can be devised.

What do you two think?

I think it's simply the case that if we don't have a backend that can handle something properly, we shouldn't expose an API for that until we do.
But, I'm not sure fixing the backend would necessarily be too hard.

What are the implications for https://addons.thunderbird.net/en-US/thunderbird/addon/just-restart_tb68 (I favorite of mine)

Not doable as an MailExtension.

Attached patch 1359410-reorderBackend.patch (obsolete) — Splinter Review

Right, it may be hard using existing methods, but if we are allowed to make a new method that notifies the account manager of the change, it could work.

Protz, please try the attached patch and from your new function setOrder() call MailServices.accounts.reorderAccounts(finalList) instead of setting the mail.accountmanager.accounts pref and restarting. The finalList is the array you construct from accountIds and missing arrays concatenated. It should contain all existing accounts. You can keep managing mail.accountmanager.accounts.ordered in setOrder exactly as it is.
You still need to force refreshing the folder pane, but maybe now the foldertree.mode = mode could work.

Attachment #9120873 - Flags: feedback?(jonathan.protzenko)

(In reply to :aceman from comment #55)

Protz, please try the attached patch and from your new function setOrder() call MailServices.accounts.reorderAccounts(finalList) instead of setting the mail.accountmanager.accounts pref and restarting. The finalList is the array you construct from accountIds and missing arrays concatenated. It should contain all existing accounts. You can keep managing mail.accountmanager.accounts.ordered in setOrder exactly as it is.
You still need to force refreshing the folder pane, but maybe now the foldertree.mode = mode could work.

This worked as advertised and now a restart is no longer needed. Thanks very much!

Here's a combined patch that has all the changes in. I was able to confirm via the addon that this has the desired effect and merely setting the mode property of the folder tree view refreshes the UI.

Thanks very much Aceman for a most helpful patch!

Geoff, I've requested another review -- I feel like this patch has the potential to make everyone on this bug happy :)

Attachment #9118447 - Attachment is obsolete: true
Attachment #9119118 - Attachment is obsolete: true
Attachment #9120873 - Attachment is obsolete: true
Attachment #9120873 - Flags: feedback?(jonathan.protzenko)
Attachment #9121216 - Flags: review?(geoff)
Attached patch 1359410-accounts-setorder-1.diff (obsolete) — Splinter Review

I started reviewing this and kept thinking how badly it needed a test … ended up just writing a test. So here's that test and the changes I was going to ask for in review. (Interdiff should work.)

There's one thing that seems a bit weird but probably doesn't matter. When clearing the custom order, accounts won't go back into the original order (ie. the order they were created) if they're of the same type, e.g. two IMAP accounts.

R? aceman on the new bits, otherwise it's r+ from me.

Attachment #9121216 - Attachment is obsolete: true
Attachment #9121216 - Flags: review?(geoff)
Attachment #9121935 - Flags: review?(acelists)
Attachment #9121935 - Flags: feedback?(jonathan.protzenko)

Side note: mozilla::Swap just got removed in favour of std::swap so this won't build against the latest m-c.

Comment on attachment 9121935 [details] [diff] [review]
1359410-accounts-setorder-1.diff

Looks good to me. Thanks very much for writing tests, much appreciated.
Attachment #9121935 - Flags: feedback?(jonathan.protzenko) → feedback+
Comment on attachment 9121935 [details] [diff] [review]
1359410-accounts-setorder-1.diff

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

Thanks for adding tests. Sadly I can't run mochitest locally and I can't follow the flow in these too async tests.

::: mail/components/extensions/parent/ext-accounts.js
@@ +22,5 @@
> +  "Services",
> +  "resource://gre/modules/Services.jsm"
> +);
> +
> +const { allAccountsSorted, compareAccounts } = ChromeUtils.import(

allAccountsSorted() does not seem to be needed by the added code.

@@ +104,5 @@
> +
> +          // Don't forget to preserve the accounts that are not returned by list
> +          // above.
> +          const allAccounts = accountIds.concat(
> +            missing.sort(compareAccounts).map(a => a.key)

Why are you actually sorting the accounts here in any way? Didn't we say we want to keep the order of the accounts not specified, as it currently is?
If there are accounts are "account1,account2,account3" and you call setOrder(["account2"]), then we want to get "account2,account1,account3" not any other sorted order (you could get "account2,account3,account1", if account3 is IMAP and account1 is Local Folders).

::: mail/components/extensions/test/browser/browser_ext_accounts_order.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* globals gFolderTreeView */
> +

Please add some description of the test file and reference the bug number.

@@ +22,5 @@
> +      }
> +
> +      async function doCheck(testName) {
> +        browser.test.log(`Test: ${testName}`);
> +        let expectedOrder = await awaitMessage("doCheck");

I don't understand what this will contain.

@@ +196,5 @@
> +  // Reset the order, check that it is reset.
> +  await extension.awaitMessage("doCheck");
> +  checkOrder(false, ...testAccountIds);
> +
> +  // Set a new order with one account missing.

Which account is missing in the check? There are 4 in the call below.

@@ +218,5 @@
> +    testAccountIds[2],
> +    testAccountIds[3]
> +  );
> +
> +  // Set a new order with faulty data. The order should not change.

What is faulty in the following 3 calls to checkOrder() ? And they also look identical.

(In reply to :aceman from comment #61)

Why are you actually sorting the accounts here in any way? Didn't we say we
want to keep the order of the accounts not specified, as it currently is?
If there are accounts are "account1,account2,account3" and you call
setOrder(["account2"]), then we want to get "account2,account1,account3" not
any other sorted order (you could get "account2,account3,account1", if
account3 is IMAP and account1 is Local Folders).

We want to get any remaining accounts in the order they would ordinarily show in the UI. (That is, it should appear as though account2 has been removed from where it is in the list and put at the top and no other changes happened.) Putting the accounts back in numerical order is of no use to the user.

I don't understand what this will contain.

Ugh, the way these tests have to be written is brain-melting. We go back and forth between the extension calling browser.accounts.setOrder and the main process checking the order is correct. It might be easier to make sense of it if you deal with each part separately, knowing that every time you reach awaitMessage, the other part does something.

@@ +196,5 @@

  • // Reset the order, check that it is reset.
  • await extension.awaitMessage("doCheck");
  • checkOrder(false, ...testAccountIds);
  • // Set a new order with one account missing.

Which account is missing in the check? There are 4 in the call below.

@@ +218,5 @@

  • testAccountIds[2],
  • testAccountIds[3]
  • );
  • // Set a new order with faulty data. The order should not change.

What is faulty in the following 3 calls to checkOrder() ? And they also look
identical.

These are the expected orders.

(In reply to Geoff Lankow (:darktrojan) from comment #62)

We want to get any remaining accounts in the order they would ordinarily show in the UI. (That is, it should appear as though account2 has been removed from where it is in the list and put at the top and no other changes happened.) Putting the accounts back in numerical order is of no use to the user.

I think you achieve this also when keeping the order in the pref as it is. If mail.accountmanager.accounts.ordered was true before and after the call to setOrder with 1 account, then it should happen what you say. If it was false and is now true after the call, then yes, the order will totally change in the UI, but I don't think the expectation is that the 1 account gets to the top and the others are unchanged in the UI at the expense of totally reordering the pref. Where did we agree on that?

I did not propose numerical order in this case, just that the given account is moved to the front in the pref and others are left as they are, in the pref, not in the UI.

This is basically a question for Protz on what the API should do, what is useful for the addons.

(In reply to Geoff Lankow (:darktrojan) from comment #58)

There's one thing that seems a bit weird but probably doesn't matter. When clearing the custom order, accounts won't go back into the original order (ie. the order they were created) if they're of the same type, e.g. two IMAP accounts.

Yes, we do not store the original order anywhere yet so we can't restore it on setOrder(null). But sorting the existing account ids in numerical order (the number after 'account' string) would basically restore them to the order of creation that we normally have.

That makes sense except for the fact that you're then displaying the accounts to the user in the order set. Order of creation doesn't mean a lot to the user.

Of course not. By default we never show the order of creation (even though it is in the pref) and reorder the accounts in the UI with the compareAccounts algorithm. But when an addon turns off this default algo, it is a question what it wants to do. Originally I let it freely do and manage the whole pref and that would be honored in the UI. Now when it is possible to call setOrder with only a subset of accounts, it needs to be specced out how the rest of them will be ordered. But I would not be against the current reordering with compareAccounts, if you add the resetting to numerical order on setOrder(null). As that is where the user started before messing with the order.

I think this API is good as it is and would advocate for it being merged.

  • The question of what happens when calling setOrder() without any argument is the important one, and I believe it the code as it is ensures that the accounts are reset to the default sort order, which is what we want.
  • I will always provide all the accounts, never a subset of them. So, let's not pay too much attention to what happens when only a subset of the accounts is passed -- I believe it's totally irrelevant and if any future addon (not mine) disagrees about the API's choice for the accounts that are missing from the argument list, they can always provide more account-ids to enforce a given sort order.

Worded differently: I would love for this to be merged now so that I can start pointing people who write to me daily to an alpha build of my addon telling them to use Thunderbird Daily or wait for the next release.

Attachment #9121935 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9121935 [details] [diff] [review]
1359410-accounts-setorder-1.diff

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

::: mail/components/extensions/parent/ext-accounts.js
@@ +76,5 @@
>            }
>            return null;
>          },
> +        async setOrder(accountIds) {
> +          let setPref = !!accountIds;

since setPref is used just once, just inline it.

@@ +83,5 @@
> +          }
> +
> +          // Some input validation.
> +          const unique = new Set();
> +          const missing = [];

we've usually used let for these things

@@ +99,5 @@
> +          if (unique.size != accountIds.length) {
> +            throw new ExtensionError(
> +              "Invalid account list (duplicates, or invalid IDs)"
> +            );
> +          }

Looks like you'd run into this for the IM account case? For that, unique and accountIds would be different size

@@ +110,5 @@
> +
> +          Services.prefs.setBoolPref(
> +            "mail.accountmanager.accounts.ordered",
> +            setPref
> +          );

why do we need a pref for this?

@@ +116,5 @@
> +
> +          // Notify the UI to rebuild the tree.
> +          for (let w of Services.wm.getEnumerator("mail:3pane")) {
> +            // eslint-disable-next-line no-self-assign
> +            w.gFolderTreeView.mode = w.gFolderTreeView.mode;

calling _rebuild() would be better. That should probably be rebuild()

::: mailnews/base/public/nsIMsgAccountManager.idl
@@ +236,5 @@
>    // Used to sort servers (accounts) for e.g. the folder pane
>    long getSortOrder(in nsIMsgIncomingServer server);
> +
> +  /**
> +   * Sets new order of all accounts.

remove the all

@@ +238,5 @@
> +
> +  /**
> +   * Sets new order of all accounts.
> +   *
> +   * @param accounts  An array of string account keys.

@param accounts -  Account keys in the new preferred order.

::: mailnews/mailnews.js
@@ +466,5 @@
>  pref("mail.accountmanager.accounts", "");
>  
> +// If false, we ignore the order of accounts defined in mail.accountmanager.accounts
> +// and nicely group accounts by type in the UI.
> +pref("mail.accountmanager.accounts.ordered", false);

I don't quite like that there'd be two ways to sort. The grouping by account type is a bit odd I think: the order the account were in the list makes more sense, and then if the default is changed, we could just move that to the top of the account ids in that list. Then we should just otherwise too allow rearranging the order by dnd
Attachment #9121935 - Flags: review?(mkmelin+mozilla)
Attachment #9121935 - Flags: review?(acelists)

Thanks for the review, Magnus!

Looks like you'd run into this for the IM account case? For that, unique and accountIds would be different size

This test works fine in the presence of an IM account. It only catches two cases: caller passed duplicate accountIds, or caller passed an accountId that corresponds to no known account. Geoff, can you confirm that this is covered by your tests?

I don't quite like that there'd be two ways to sort. The grouping by account type is a bit odd I think: the order the account were in the list makes more sense, and then if the default is changed, we could just move that to the top of the account ids in that list. Then we should just otherwise too allow rearranging the order by dnd

The scope of this bug is exclusively to provide a MailExtension API entry point for "Manually Sort Folders", currently the fourth most-popular addon with 200,000 users, so that it can keep working with future versions of Thunderbird. From what I read on MDN, the process is that if some API is missing, addon authors can ask for new MailExtension APIs to support their use-cases. Rather than just asking, I worked out a prototype patch, which was then improved tremendously by Geoff and Aceman. We then reached consensus on what the API should look like. The API is sound (addons cannot break the internal invariants), it has no effect on the default behavior of Thunderbird, and 100% foots the bill for the purposes of what my addon needs.

I understand and appreciate that having a built-in solution would be much better, but to me this means opening up a different bug, and finding someone ready to commit the cycles and make this a built-in feature of Thunderbird. In the meanwhile, landing an API we reached consensus on would i) allow me to move on to other missing MailExtension APIs that my addon needs, ii) provide a great way to keep a very popular addon working and iii) would, I believe, be very much in the spirit of what MailExtensions are meant to be.

Thanks!

Comment on attachment 9121935 [details] [diff] [review]
1359410-accounts-setorder-1.diff

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

::: mail/components/extensions/parent/ext-accounts.js
@@ +76,5 @@
>            }
>            return null;
>          },
> +        async setOrder(accountIds) {
> +          let setPref = !!accountIds;

We need the value from this point to use later, by which time it might have changed.

@@ +99,5 @@
> +          if (unique.size != accountIds.length) {
> +            throw new ExtensionError(
> +              "Invalid account list (duplicates, or invalid IDs)"
> +            );
> +          }

No you wouldn't. `unique` is a set of valid keys that are in `accountIds`. IM account keys won't be in it.

This isn't substantially different from the previous patch but it does fix the review comments.

Attachment #9121935 - Attachment is obsolete: true
Attachment #9127709 - Flags: review?(mkmelin+mozilla)

(In reply to Jonathan Protzenko [:protz] from comment #69)

The scope of this bug is exclusively to provide a MailExtension API entry point for "Manually Sort Folders", currently the fourth most-popular addon with 200,000 users, so that it can keep working with future versions of Thunderbird. From what I read on MDN, the process is that if some API is missing, addon authors can ask for new MailExtension APIs to support their use-cases.

That's true but I guess we should just fix bug 244347.
The problem with adding a pref to do a special behaviour is that then correctly fixing bug 244347 would be much harder.

Comment on attachment 9127709 [details] [diff] [review]
1359410-accounts-setorder-2.diff

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

This still uses the pref - is there really a good reason to? The other code is useful. Maybe we can take that code, then fix bug 244347. 
Of course, if that is fixed, it's unclear if this API is needed.
Attachment #9127709 - Flags: review?(mkmelin+mozilla)

Thanks Magnus for your feedback.

I would like to confirm what you are suggesting:

  • always observe the order of accounts in mail.accountmanager.accounts, i.e. get rid of the current behavior which is to group accounts by type; this entails getting rid of the ordered pref
  • add new code to change that order when the default account is changed, so that the default account makes it to the top of the list
  • keep the API entry point for my addon to allow users to arbitrarily reorder accounts.

:aceman and Geoff, what are your thoughts?

I am fine with the three points above -- I don't believe bug 244347 will be fixed anytime soon (it's 16 years old), but the three points above will allow my addon to continue to work, and people will have a way through the addon to edit the sort behavior if they are so inclined.

Thanks,

Jonathan

I see Magnus suggested to use tree._rebuild(). I'm not sure I'm happy about that, whether the addon does not peek too much into the internals. Yes, that would be fine in the past, but today Mozilla pretends the addons are not supposed to do that and isolates them in WE as much as possible.
I'm not sure Magnus proposed to drop the automatic ordering of accounts by type (added in bug 749200) and using the pref order directly.
Implementing bug 244347 in TB would mean to add much of the UI the addon currently has to TB. Nobody has decided on that yet. Maybe that would be worth for 200000 users, more rarely used features got implemented already.

Also, dropping the patch as it is and reworking it as you proposed would probably render it unusable for TB 68 due to higher risk for all users, not just the addon users.

Hi Magnus, can we try to recap where we stand on this bug?

My impression is that aceman and I both agree that fixing the underlying bug 244347 is not feasible and that we should try to get an addon solution to this for the time being. Do you concur?

Regarding the point of not having a supplemental pref -- as aceman says, this is quite risky and risks changing the default behavior for users, knowing that they are notoriously conservative on this. I like the idea of having a flag that guarantees that nothing changes and that no user is upset, then the flag can be activated via the addon. We can always refine later and get rid of the flag, I see nothing that prevents a future simplification from happening. Thoughts?

Thanks,

Jonathan

Flags: needinfo?(mkmelin+mozilla)

Why is fixing bug not feasible? I've actually commented there earlier today. The plan is to fix that now.

Flags: needinfo?(mkmelin+mozilla)

That sounds great, I'll let the Thunderbird team fix the underlying bug then, and will update my addon page to say that the account sorting functionality is no longer available as the Thunderbird team plans on implementing it soon, with a link to bug 244347.

Are there similar plans to allow sorting folders in the folder pane? This is the main functionality of my addon, so if Thunderbird is adding reordering of accounts now as a built-in feature, perhaps it should implement reordering of folders too.

Thanks,

Jonathan

Flags: needinfo?(mkmelin+mozilla)

No plans to allow sorting of folders. That sound like something well suited for an (your) add-on to provide.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.