1096006 - Add AccountManager to the prefs in tab

Status: RESOLVED WONTFIX

(Thunderbird :: Account Manager, defect)

Description

[Richard Marti (:Paenglab)]

Bug 925746 opens the prefs in a tab. The Account Manager should be integrated in this tab for a easier interaction with the other prefs. The actual design uses two modal dialogs for prefs and AM. This means you can only work in one dialog and need to close this to work on the other. With AM integrated in PrefsInTab you can switch between the options as you like.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: WIP - AccountPrefsInTab.patch

This is a WIP with the limitation to not select the correct account.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: WIP - AM-Subdialogs.patch

This patch opens subdialogs of the Junk-, Composition-, Addressing- and Return receipt settings instead of jumping to the prefs pages and tabs. This helps to not to loose the focus in AM.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: WIP - AccountPrefsInTab.patch

Latest WIP

Opening the AM should now work from everywhere.
What's still not working is the selection of the correct account and jumping directly to a subpage. Every help is appreciated.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: WIP - AM-Subdialogs.patch

Latest WIP of the subdialogs.

Opens dialogs instead of jumping to the page in prefs to make the usage less irritating.

Comment 5

[Jim Porter (:squib)]

It might be helpful to consult the mega-prefs discussion back in 2010, where we tried to merge Prefs and Account Settings: https://mail.mozilla.org/pipermail/tb-planning/2010-December/000632.html

Comment 6

[Richard Marti (:Paenglab)]

Attachment: WIP - AccountPrefsInTab.patch

Here is a description of what I'm doing, or trying to do, with this patch:
Instead of using mailnews/base/prefs/content/AccountManager.xul I added in mail/components/preferences/aboutPreferences.xul a new prefpane (mail/components/preferences/AccountManager.xul) which loads the Account manager contents.

Now, instead of calling the Account Manager:

window.openDialog("chrome://messenger/content/AccountManager.xul",
                  "AccountManager", "chrome,centerscreen,modal,titlebar",
                  { server: aServer, selectPage: selectPage });

I'm calling the preferencesTab:

openPreferencesTab("paneAccount", aServer, selectPage);

in mail/base/content/mail/utilityOverlay.js. Mail/components/preferences/aboutPreferences.js calls then in components/preferences/preferences.js the function selectPaneAndTab(prefWindow, aPaneID, aTabID, aSubdialogID) {} to select the pane and tab. The AM don't use a tab but a server and sometimes a selectPageId. For this I splitted here the calls. For the AM I'm trying now to call the function selectServer(aTabID, aSubdialogID) in mailnews/base/prefs/content/AccountManager.js but failing here with a "function not defined". This is why I now commented this call out. I think this should be working as mail/components/preferences/aboutPreferences.xul is loaded which references to <script type="application/javascript" src="chrome://messenger/content/AccountManager.js"/>.

So, what doing I wrong here?

With calling openPreferencesTab("paneAccount", aServer, selectPage); this should fill in the function in utilityOverlay.js's openPreferencesTab(aPaneID, aTabID, aOtherArgs) {} the 'aServer' into 'aTabID' and over preferences.js's selectPaneAndTab(prefWindow, aPaneID, aTabID, aSubdialogID){} 'aTabID' into  'server' in AccountManager.js's selectServer(server, selectPageId) {} and the 'selectPage' fill in utilityOverlay.js's 'aOtherArgs' and preferences.js's 'aSubdialogID' into AccountManager.js's 'selectPageId'.

Is this all correct (if someone is understanding what I'm writing here) or do I need to give the 'server' and 'selectPageId' differently to the functions?

I hope this helps to find the issues.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: [checked in for tb38] AccountPrefsInTab-strings.patch

Strings only patch for check-in before string freeze.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: WIP - AccountPrefsInTab.patch

Main patch without the strings. Comment 6 is still valid.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: WIP - AM-Subdialogs.patch

Subdialog patch without the strings.

Comment 10

[:aceman]

Richard, didn't you have to solve the same problem in Prefs? There were also ways to open specific panel and tab in pref. In this case, you have always the same panel (the Account manager is a single panel) and a the account is a specific row in it.

I'll try to help you here. I assume this will be off by default for some time yet. You can land the strings and I look at it later.

Comment 11

[Richard Marti (:Paenglab)]

Yes, the account manager is a pane. Switching to it is no problem, but selecting the correct account doesn't work (please look at comment 6). Also jumping then to a subpanel isn't the same as they are in a iframe and should switch in it.

Comment 12

[Kent James (:rkent)]

When I have tried the tabbed preferences, none of the keyboard shortcuts work. Is that a known issue?

Comment 13

[Richard Marti (:Paenglab)]

ALT+SHIFT+key works here.

Comment 14

[Magnus Melin [:mkmelin]]

(Another problem seems to be the prefs tab can't be closed by [x])

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Magnus Melin from comment #14)
> (Another problem seems to be the prefs tab can't be closed by [x])

Oh yes, only when the prefs are opened through "Account Settings" menuitem, then I get:

Sat Feb 21 2015 14:37:50
Error: TypeError: cyclic object value
Source file: chrome://messenger/content/tabmail.xml
Line: 717

After closing/opening TB it works again, also when the Account Manager is the active page.

With already opened prefs it's closeable even when on Account Manager or a other pane an use then the menuitem.

Comment 16

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/35b23ff818fb

Comment 17

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #6)
> Created attachment 8550725 [details] [diff] [review]
> pane and tab. The AM don't use a tab but a server and sometimes a
> selectPageId. For this I splitted here the calls. For the AM I'm trying now
> to call the function selectServer(aTabID, aSubdialogID) in
> mailnews/base/prefs/content/AccountManager.js but failing here with a
> "function not defined".
Yes, I see this problem. But it should only be a matter of how to access that function.

But I see another problem there. When you try to call that function from preferences.js, the account tree is not yet built. It is built on "paneload" event (in AccountManager.xul). But on that same event we call that function. And incidentally it get run before the tree is built. So I think it wouldn't work even if we could access the function.

So I am now trying to create a new event or something that would trigger calling the selectServer function only after all onpaneload code is run (that e.g. builds the tree).

On other pref panels this is not such a problem in practice as their onpaneload code does not modify the accessible elements radically (maybe applications.js does, but a specific application (mime type) is not referenced from the caller who opened the pref dialog). But in theory those should probably be fixed in the same way as the account tree.

Comment 18

[Kent James (:rkent)]

We would not block Thunderbird 38 for this, and no progress has been made recently. We still might be willing to land things that get fixed.

Comment 19

[:aceman]

Yes, whole prefs in tab is disabled by default so we probably do not commit to somehow keep it working 100%. And accounts in tab is a feature on top of that, that is not necessary for 38. But I'll look at it per comment 17 after we have less work on quick fixes for TB38.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: AccountPrefsInTab.patch

With the great help of aceman it is now working as it should.

Kent, I choose you because it has also mailnews code in it. And no hurry it's okay when you review it after TB 38 landing.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: AM-Subdialogs.patch

This is the patch to open global prefs like Junk, Composing and Addressing in a sub-dialog instead of switching to the other pane and after this the user has to switch back to AM.

Comment 22

[Richard Marti (:Paenglab)]

Attachment: enableInContent.patch

This is a patch to enable the in-content prefs. The tests are still using the normal dialog prefs.

If you want, I can move this patch to a new bug to enable the in-content prefs later.

Comment 23

[:aceman]

Are the subdialogs needed? :(

Comment 24

[Richard Marti (:Paenglab)]

To make it at the user easier I would say yes. But let's see what Kent thinks.

Comment 25

[:aceman]

I was under the impression that those will not be needed when we make AM work in the tab.
Maybe it is easier on the users, but harder for the developers. I am opposed to how this is implemented, not how it looks to the user.
I don't think Kent will be one of those suffering from the created code duplication :) You should ask the UI devs.

Can you find a way using overlays so that the code exists only once and is just linked from multiple places?

Comment 26

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #22)
> Created attachment 8606650 [details] [diff] [review]
> enableInContent.patch
> 
> This is a patch to enable the in-content prefs. The tests are still using
> the normal dialog prefs.
> 
> If you want, I can move this patch to a new bug to enable the in-content
> prefs later.

I'd also be for another bug. Let the feature bake for some time. Also I am not sure the AM tests will work in tab with just this simply patch.

Comment 27

[Richard Marti (:Paenglab)]

If we decide to not use the sub dialogs, we should at least use the Return receipt dialog as it makes no sense to switch to a other panel and then show the dialog. We can show it directly.

Comment 28

[Richard Marti (:Paenglab)]

Comment on attachment 8606650 [details] [diff] [review]
enableInContent.patch

removing this patch from this bug.

Comment 29

[Kent James (:rkent)]

I really hate to do this to you, but I've pretty much ignored reviews that were not critical for Thunderbird 38 releases. Looking over it now, most of the patches are associated with user interface setup in Thunderbird. I'm not really the best person to review those.

I'm going to defer these to Magnus.

Comment 30

[Richard Marti (:Paenglab)]

Aceman, something has regressed the opening of the account manager. With no prefs tab open (and before a other pane than AM selected) and then opening the AM there is no AM pane shown. The previously selected pane is also not working, all others are. Closing TB with one of the other pane selected and reopening TB shows this pane correctly. Not closing the tab but let's opening the AM works now. Closing the tab with AM selected and letting open AM again works now, also after closing TB.

Please, if you have time, could you look what's happening?

Comment 31

[Richard Marti (:Paenglab)]

Comment on attachment 8606648 [details] [diff] [review]
AccountPrefsInTab.patch

Removing the review request as it not works properly.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: AccountPrefsInTab.patch

Unbitrotted patch after Bug 1193716 landing.

Comment 33

[Richard Marti (:Paenglab)]

Comment on attachment 8606649 [details] [diff] [review]
AM-Subdialogs.patch

Moving this patch to a separate bug.

Comment 34

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #30)
> Aceman, something has regressed the opening of the account manager. With no
> prefs tab open (and before a other pane than AM selected) and then opening
> the AM there is no AM pane shown. The previously selected pane is also not
> working, all others are.
Which previously selected pane? You said no prefs tab is open.

> Closing TB with one of the other pane selected and
> reopening TB shows this pane correctly. Not closing the tab but let's
> opening the AM works now. Closing the tab with AM selected and letting open
> AM again works now, also after closing TB.
I can't reproduce any problem yet. Was this an intermittent problem that was now fixed permanently by playing with the patch and switching panes?
May the problem be fixed by using the unbitrotted patch?

Comment 35

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #34)
> (In reply to Richard Marti (:Paenglab) from comment #30)
> > Aceman, something has regressed the opening of the account manager. With no
> > prefs tab open (and before a other pane than AM selected) and then opening
> > the AM there is no AM pane shown. The previously selected pane is also not
> > working, all others are.
> Which previously selected pane? You said no prefs tab is open.

This means open the prefs, select for example General and close the tab. Now open the AM. The AM pane is empty and the General and Accounts buttons are both highlighted as selected.

> > Closing TB with one of the other pane selected and
> > reopening TB shows this pane correctly. Not closing the tab but let's
> > opening the AM works now. Closing the tab with AM selected and letting open
> > AM again works now, also after closing TB.
> I can't reproduce any problem yet. Was this an intermittent problem that was
> now fixed permanently by playing with the patch and switching panes?
> May the problem be fixed by using the unbitrotted patch?

Strange, I tried only this patch and it still happens. Please can you try it on XP with this build: https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-401179c3ef11

Comment 36

[Richard Marti (:Paenglab)]

Attachment: AccountPrefsInTab.patch

Unbitrotted patch.

Comment 37

[Richard Marti (:Paenglab)]

Attachment: AccountPrefsInTab.patch

Updated to tip.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

I'm afraid I don't have the cycles to participate here, at least currently. :/ Sorry for being a blocker.

Comment 39

[Richard Marti (:Paenglab)]

Attachment: AccountPrefsInTab.patch

Patch updated to tip.

Comment 40

[Jorg K (CEST = GMT+2)]

This works fine when Lightning is not installed. With Lightning installed, when selecting "Tools > Account Settings", not the AM tab opens but instead the last displayed and previously closed tab, for example "Display". But it opens blank. This is seen in the error console:

[5108] WARNING: Serializing document without system principal: file c:/mozilla-source/comm-central/mozilla/dom/xul/nsXULPrototypeDocument.cpp, line 309
[5108] ###!!! ASSERTION: forward references have already been resolved: 'Error', file c:/mozilla-source/comm-central/mozilla/dom/xul/XULDocument.cpp, line 1105

With the patch from bug 1202480 also applied I saw this before:

[5424] WARNING: Serializing document without system principal: file c:/mozilla-source/comm-central/mozilla/dom/xul/nsXULPrototypeDocument.cpp, line 309
[5424] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file c:/mozilla-source/comm-central/mozilla/dom/xul/nsXULPrototypeCache.cpp, line 323
[5424] ###!!! ASSERTION: forward references have already been resolved: 'Error', file c:/mozilla-source/comm-central/mozilla/dom/xul/XULDocument.cpp, line 1105

This might worth investigating.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: account-in-tab.patch - JK debug

I spent countless hours with this.

I have the impression that there are at least two problems in selectPaneAndTab() and showTab(). Two event listeners never fire so things don't work.

With this patch the following works:
Tools > Options, click on Display.
Open Options tab which was showing Display.
Close tab.
Tools > Options: Opens Display tab.
Close tab.
Tools > Account Settings: Opens AM tab and then fails with:
JavaScript error: chrome://messenger/content/preferences/preferences.js, line 110: TypeError: window.selectServerByKey is not a function

I think we need Aceman who understands his own magic better. At least he could tell me how to get selectServerByKey() to work.

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 8822668 [details] [diff] [review]
AccountPrefsInTab.patch

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

Let me ask some questions here. Sadly a lot of code is not obvious and a lot of new code isn't documented, so I have to reverse-engineer what was on the mind of the author. That's OK for working code, but hard to debug if it's not working, like in this case.

::: mail/base/content/utilityOverlay.js
@@ -292,5 @@
>      otherArgs: otherArgs,
> -    onLoad: function(aEvent, aBrowser) {
> -      let prefWindow = aBrowser.contentDocument.getElementById("MailPreferences");
> -      aBrowser.contentWindow.selectPaneAndTab(prefWindow, paneID, tabID, otherArgs);
> -    }

Why was this replaced with a different scheme, see (**) below.

::: mail/components/preferences/preferences.js
@@ -53,5 @@
>      let tabOnEvent = false;
>      // The prefwindow element selects the pane specified in window.arguments[0]
>      // automatically. But let's check it and if the prefs window was already
>      // open, the current prefpane may not be the wanted one.
> -    if (prefWindow.currentPane.id != prefPane.id) {

Why was this removed (and the indentation not fixed)?

@@ -58,2 @@
>        if (aTabID && !prefPane.loaded) {
>          prefPane.addEventListener("paneload", function showTabOnLoad() {

Here's another "paneLoad" listener. I never saw this being executed. The one down at (**) is. Should that other one have set .loaded to true? I found it being 'false', the listener was attached (note once:true), but doesn't get called.

@@ +83,5 @@
> +  } else {
> +    if (!aPane.getAttribute("scriptLoaded")) {
> +      aPane.addEventListener("scriptload", function scriptLoad() {
> +        aPane.removeEventListener("scriptload", scriptLoad);
> +        selectAMServer(aPane, aTabID, aSubdialogID);

Can you explain and document this trick. I don't see this being called, at least not with Lightning installed.

@@ +93,4 @@
>  }
> +
> +function selectAMServer(aPane, aServerID, aPanelID) {
> +  window.selectServerByKey(aServerID, aPanelID);

Which 'window' is this? In my patch I just tried to call selectAMServer() and it didn't work.

::: mail/components/preferences/preferencesTab.js
@@ -85,5 @@
>      this._setUpCloseWindowListener(aTab);
>  
> -    if ("onLoad" in aArgs) {
> -      aTab.browser.addEventListener("paneload",
> -        function _contentTab_onLoad (event) { aArgs.onLoad(event, aTab.browser); },

(**) Here you changing from the onLoad() to something else.

@@ +90,5 @@
> +      let lastPane = prefTab.lastSelected;
> +      if (!aArgs.paneID && (lastPane == "paneAccount"))
> +        aArgs.paneID = aTab.paneID = prefTab.preferencePanes[0].id;
> +      tabType.shouldSwitchTo(aArgs);
> +      aTab.browser.removeEventListener("paneload", _contentTab_onLoad, true);

(**) Here you changing from the onLoad() to something else. Why? Also, you can attach a listener with once:true, so you don't have to remove it later. Note here is a "paneLoad" listener. This one seems to run, since shouldSwitchTo() is called from here.

::: mailnews/base/prefs/content/AccountManager.js
@@ +158,5 @@
> +      // If we are in a preferences tab, we didn't get arguments via "window".
> +      // We need to notify the caller that the account tree is built so that
> +      // he can now select a specific account if he wants.
> +      prefPane.parentNode._fireEvent("scriptload", prefPane);
> +      prefPane.setAttribute("scriptLoaded", true);

Please explain this magic further. What is _fireEvent() as opposed to fireEvent(). This runs inside an onLoad() function, but what does get loaded? I don't see the listener to the event running.

Comment 43

[Jorg K (CEST = GMT+2)]

Also, since there seem to be conflicts with Lightning, I took a look at what that does. I can's see anything special here, they do XUL straight out of the book, whereas you try to outsmart XUL with your various listeners ;-)

Here's Lightning:
https://dxr.mozilla.org/comm-central/rev/3c3539b7c5d8f3b35b699166a12064c467a3fe2b/calendar/lightning/content/messenger-overlay-preferences.xul#19

Comment 44

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #42)
> Comment on attachment 8822668 [details] [diff] [review]
> AccountPrefsInTab.patch
> 
> Review of attachment 8822668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me ask some questions here. Sadly a lot of code is not obvious and a lot
> of new code isn't documented, so I have to reverse-engineer what was on the
> mind of the author. That's OK for working code, but hard to debug if it's
> not working, like in this case.

This is all code I got from aceman. So it's best when he explains.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #44)
> This is all code I got from aceman. So it's best when he explains.
I know.

Comment 46

[Nomis101]

What is the status of this? I would love the idea to have the AccountManager prefs in a tab.

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to Nomis101 from comment #46)
> What is the status of this? I would love the idea to have the AccountManager
> prefs in a tab.
Working when Calendar/Lightning is not installed, not quite working otherwise, see comment #40 and below. I tried to debug it and got no where, NI still open for Aceman since September 2015, almost two years ago.

Feel free to debug it, I'm sure Richard has a refreshed patch.

Comment 48

[Nomis101]

(In reply to Jorg K (GMT+2) from comment #47) 
> Feel free to debug it, I'm sure Richard has a refreshed patch.
I already have a recent build with this patch applied, so I could start debugging this. But I'm not very experienced with debugging.

Comment 49

[Jorg K (CEST = GMT+2)]

Richard, can you get these patches ready for landing.

Also, Aceman told us in a PM:
===
It was not only Lightning, but also the links from account manager into the preferences, which weren't working right when prefs were in a tab. But maybe they will work now when everything will be forced into a tab.
That was the last missing piece on which the inclusion of account manager into a tab (bug 1096006).
===

Can we check that please.

Comment 50

[Richard Marti (:Paenglab)]

Attachment: AccountManager.patch

This is my latest patch. It has the problem that when the prefs tab is closed, and before was a other than the AM pane selected, and you open the Account Manager, it doesn't switch to the AM pane. With open prefs tab, it switches to the AM-pane.

Comment 51

[Richard Marti (:Paenglab)]

Oh, forgot to ask if you could look why it doesn't switch?

Comment 52

[Jorg K (CEST = GMT+2)]

Hmm, I looked at this ages ago, like in January 2017. For the record, the command that's run from the menu is
  MsgAccountManager(null)
which essentially runs
  openPreferencesTab("paneAccount", aServer ? aServer.key : null, aSelectPage);

I guess most of the JS code here was written by Aceman. How do we guarantee that the prefs tab is actually open before we switch to the AM pane? Looks to me like there should be a check: Prefs tab open? If yes, switch to AM pane. If no, open prefs tab and attach a listener that switches to the AM pane when done.

Comment 53

[Jorg K (CEST = GMT+2)]

Comment on attachment 8984232 [details] [diff] [review]
AccountManager.patch

I'm not sure what other feedback I should give.

Comment 54

[Richard Marti (:Paenglab)]

Attachment: AccountManager.patch

This is the updated patch after landing of bug 1465061. Unfortunately one part I couldn't apply on preferencesTab.js:

@@ -79,43 +79,50 @@
     aTab.paneID = aArgs.paneID;
     aTab.tabID = aArgs.tabID;
     aTab.otherArgs = aArgs.otherArgs;
 
     // Now set up the listeners.
     this._setUpTitleListener(aTab);
     this._setUpCloseWindowListener(aTab);
 
-    if ("onLoad" in aArgs) {
-      aTab.browser.addEventListener("paneload",
-        function _contentTab_onLoad (event) { aArgs.onLoad(event, aTab.browser); },
-        {capture: true, once: true});
-    }
+    let tabType = this;
+    aTab.browser.addEventListener("paneload", function _contentTab_onLoad (event) {
+      let prefTab = aTab.browser.contentDocument.getElementById("MailPreferences");
+      let lastPane = prefTab.lastSelected;
+      if (!aArgs.paneID && (lastPane == "paneAccount"))
+        aArgs.paneID = aTab.paneID = prefTab.preferencePanes[0].id;
+      tabType.shouldSwitchTo(aArgs);
+      aTab.browser.removeEventListener("paneload", _contentTab_onLoad, true);
+    }, true);
+
     // Now start loading the content.

Aceman, please could you fix me this part?

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: AccountManager.patch

I got this from Richard, shuffled some code in preferences.js to fix some linting errors. With the patch, the preferences panes are turned into radio buttons, so certainly some CSS isn't right here any more.

The "Accounts" tab is almost empty, and there are also a bunch of (minor) linting errors I didn't fix.

I hope Aceman can take over here.

Comment 56

[:aceman]

Attachment: AMtabcurrent.patch

OK, this at least loads the AM content (the list of accounts and the right panel for the selected account pane).

Still needs some work like:
-totally ugly styling that affects all prefs
-OK button probably needs removing, but we have a lot of code hooked on it so that needs some thought when to invoke it
-closing prefs tab throws an error

But hopefully you can continue from here :)

Comment 57

[Richard Marti (:Paenglab)]

Hmm, I still see nothing except the empty listbox and a OK button.

Comment 58

[:aceman]

Attachment: AMtabcurrent.patch

Sorry, a typo.

Comment 59

[Richard Marti (:Paenglab)]

Attachment: 1096006-AM-in-prefsTab.patch

(In reply to :aceman from comment #56)

Created attachment 9062008 [details] [diff] [review]
AMtabcurrent.patch

OK, this at least loads the AM content (the list of accounts and the right panel for the selected account pane).

Still needs some work like:
-totally ugly styling that affects all prefs

Fixed

-OK button probably needs removing, but we have a lot of code hooked on it so that needs some thought when to invoke it

The button is now needed to set the changes. Like the other parts of the prefs changes should immediately apply.

-closing prefs tab throws an error

You mean the aTab.browser.contentDocument.getElementById(...).currentPane is undefined ?

• The pref tab is not restored when reopening TB.

• When opening the pref tab I see: TypeError: prefPane.parentNode._fireEvent is not a functionAccountManager.js:177:27

But hopefully you can continue from here :)

This needs someone with more experience than me.

Comment 60

[Jorg K (CEST = GMT+2)]

I think this looks good in general. When Lightning is installed, I can see this problem already mentioned in comment #40.

STR: Open Options to the Display pane (or any other pane apart from Accounts). Close options. Tools > Account Settings. This opens the last opened pane, so Display, not the account settings.

I'd like to push this over the finishing line, even if we have to fix some issued during the beta cycle. Aceman, can you please keep looking at it. Mayby Geoff can also spent a bit of time here, I guess lately he's the guy who understands preferences in conjunction with Lightning best.

Comment 61

[Geoff Lankow (:darktrojan)]

Attachment: 1096006-AM-in-prefsTab-2.diff

I've fixed the load-time and unload-time issues.

Comment 62

[Jorg K (CEST = GMT+2)]

Thanks Geoff, much appreciated. So now two tasks are left: Adjust tests for the new UI, try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe90a2c95c01254e441d8fc0196b06f9ca7e6bc6
and getting rid of the "OK" button.

Any ideas on that?

EDIT: The try run also shows the remaining linting issues nicely.

Comment 63

[Jorg K (CEST = GMT+2)]

A few account related tests failing.

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: 1096006-AM-in-prefsTab-2.diff

Fixed linting errors. I'm not sure what to do in function MsgAccountManager(aSelectPage, aServer). I added XXX TODO.

Comment 65

[:aceman]

Attachment: AMtab-05-08.patch

Changes:

1. when closing the prefs tab having SMTP servers pane selected,
   aTab.browser.contentWindow.getCurrentAccount() was null and then .incomingServer couldn't be retrieved.
2. I reordered some css to load messenger.css before accountManage.css so that the new styles override in some more AM dialogs.

Geoff:
1.After closing the Manage identities->Edit identity dialog with Cancel, I get
ReferenceError: reference to undefined property "contentFrame" in AccountManager.js:324:7
It's because the line document.addEventListener("dialogcancel", onNotAccept); in AccountManager.js adds this listener to all subdialogs that import AccountManager.js (just to get its functions, not the listeners).

2. It seems persisting the tabID (server key) and then restoring it on TB restart does not work anyway, the prefs tab restores on the default account, not the last one.

Comment 66

[:aceman]

Comment on attachment 9062961 [details] [diff] [review]
1096006-AM-in-prefsTab-2.diff

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

::: mail/components/preferences/preferences.js
@@ +141,4 @@
>    }
>  }
>  
> +function selectAMServer(pane, aServerID, aPanelID) {

Why is this taking a 'pane' argument?

::: mail/components/preferences/preferencesTab.js
@@ +133,4 @@
>      return {
>        tabURI: aTab.url,
> +      paneID: currentPane,
> +      tabID: (currentPane == "paneAccount" ? aTab.browser.contentWindow.getCurrentAccount().incomingServer.key : null),

Does restoreTab() know how to specially restore the AM tab with the right server?

::: mail/locales/en-US/chrome/messenger/AccountManager.dtd
@@ +31,5 @@
>       XUL/FE DEVELOPERS: DO NOT MODIFY THIS VALUE. It represents the correct size of
>       this window for en-US. -->
>  <!ENTITY accountManager.size "width: 105ch; height: 55em;">
>  <!ENTITY accountTree.width "width: 32ch;">
> +<!ENTITY accountTreeInTab.width "width: 28ch;">

Why should it be so narrow? Why don't we use the full width of the tab/window now? This would be the single benefit of this whole change and we are not using it?

::: mailnews/base/prefs/content/accountUtils.js
@@ +246,5 @@
> +    // If the Account Manager is already running, just focus the right server.
> +    let existingAccountManager = Services.wm.getMostRecentWindow("mailnews:accountmanager");
> +    if (existingAccountManager) {
> +      existingAccountManager.focus();
> +      existingAccountManager.selectServer(aServer);

Why is this changing? This will only be used by Seamonkey and there we hopefully do not change the behaviour. The AM should select the right server using the {server: aServer} argument in window.arguments.

Comment 67

[:aceman]

(In reply to :aceman from comment #65)

Geoff:
1.After closing the Manage identities->Edit identity dialog with Cancel, I get
ReferenceError: reference to undefined property "contentFrame" in AccountManager.js:324:7
It's because the line document.addEventListener("dialogcancel", onNotAccept); in AccountManager.js adds this listener to all subdialogs that import AccountManager.js (just to get its functions, not the listeners).

For this, we probably can get rid of the ondialogaccept and ondialogcancel in AccountManager.js if we want to achieve all changes apply instantly (which isn't yet true for AM).
We could possibly remove that single user of onCancel in am-offline.js (and the 'rollback' code), but for saving the other changes, do you know how to hook on the tab close event and selecting a different pref pane? We could hook the onAccept code on that.

Comment 68

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #66)

+function selectAMServer(pane, aServerID, aPanelID) {

Why is this taking a 'pane' argument?

Hard to tell where code comes from in this multi-author bug, but wasn't this originally your code?

Comment 69

[:aceman]

(In reply to :aceman from comment #65)

Geoff:
1.After closing the Manage identities->Edit identity dialog with Cancel, I get
ReferenceError: reference to undefined property "contentFrame" in AccountManager.js:324:7

I split this out into bug 1552851 as it is not specific to AM being in a tab.

Comment 70

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #65)

2. It seems persisting the tabID (server key) and then restoring it on TB restart does not work anyway, the prefs tab restores on the default account, not the last one.

That doesn't work for any of the other panes either. They persist the selected tab in prefs, see: mail.preferences.advanced.selectedTabIndex and friends.

(In reply to :aceman from comment #67)

For this, we probably can get rid of the ondialogaccept and ondialogcancel in AccountManager.js if we want to achieve all changes apply instantly (which isn't yet true for AM).
We could possibly remove that single user of onCancel in am-offline.js (and the 'rollback' code), but for saving the other changes, do you know how to hook on the tab close event and selecting a different pref pane? We could hook the onAccept code on that.

It's not that simple. Changes don't have to apply instantly, but they need to have happened if the user switches to another tab, or another window, or if they click on something in the menus. I've been looking for a nice way to handle that, but I've found no better way than to change the 7 pages to save values as they change.

Comment 72

[:aceman]

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

It's not that simple. Changes don't have to apply instantly, but they need to have happened if the user switches to another tab, or another window, or if they click on something in the menus. I've been looking for a nice way to handle that, but I've found no better way than to change the 7 pages to save values as they change.

If somebody knows the events to listen to, we can save the changes on any of them.
So until then, I can save on change to other pref pane (e.g. from Accounts to Advanced) and on close of the whole prefs tab.

For the record, I am working on the patch right now.

Richard, could you find out why the Account actions button has no down arrow (it is a menu) and why the <textbox type="number"> elements are looking so ugly (multiple internal borders) ? These 2 things could be fixed in parallel, even in a separate bug as this affects also other pref panes.

Comment 73

[Richard Marti (:Paenglab)]

Attachment: 1096006-AM-in-prefsTab-3.diff.patch

Fixed the dropdown-arrow on Linux. Mac and Windows aren't affected.

Comment 74

[:aceman]

Comment on attachment 9069241 [details] [diff] [review]
1096006-AM-in-prefsTab-3.diff.patch

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

Thanks, works for me.

::: mail/themes/linux/mail/accountManage.css
@@ +62,5 @@
> +  margin-left: 0;
> +  margin-right: 0;
> +}
> +
> +#accountActionsButton > .button-box > .button-menu-dropmarker {

Why isn't this global for <button type="menu"> ?

Comment 75

[Richard Marti (:Paenglab)]

Because <button type="menu"> was never used in the FX/TB prefs until now?

Comment 76

[Magnus Melin [:mkmelin]]

Though perhaps it would have been better to key it on button[type="menu"] and not the id?

Comment 77

[Richard Marti (:Paenglab)]

Attachment: 1096006-AM-in-prefsTab-3.diff.patch

Made the dropmarker for all button[type="menu"].

Comment 78

[:aceman]

Comment on attachment 9069426 [details] [diff] [review]
1096006-AM-in-prefsTab-3.diff.patch

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

::: mail/themes/linux/mail/preferences/aboutPreferences.css
@@ +28,5 @@
>    margin-top: 11px;
>    margin-bottom: 11px;
>  }
>  
> +button[type="menu"] > .button-box > .button-menu-dropmarker {

So this needs to be moved for the other platforms too.

@@ +32,5 @@
> +button[type="menu"] > .button-box > .button-menu-dropmarker {
> +  margin-inline-end: 4px;
> +  list-style-image: url("chrome://global/skin/icons/arrow-dropdown-12.svg");
> +  -moz-context-properties: fill;
> +  fill: currentColor;

It seems a similar arrow is already applied at https://searchfox.org/comm-central/source/mozilla/toolkit/themes/windows/global/button.css#71, why doesn't our button pick it up and then we do not need this block in our css files?

Comment 79

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #78)

Comment on attachment 9069426 [details] [diff] [review]
1096006-AM-in-prefsTab-3.diff.patch

Review of attachment 9069426 [details] [diff] [review]:

::: mail/themes/linux/mail/preferences/aboutPreferences.css
@@ +28,5 @@

margin-top: 11px;
margin-bottom: 11px;
}

+button[type="menu"] > .button-box > .button-menu-dropmarker {

So this needs to be moved for the other platforms too.

No, Mac and Windows have the needed rules in toolkit. Linux not.

@@ +32,5 @@

+button[type="menu"] > .button-box > .button-menu-dropmarker {

• margin-inline-end: 4px;
• list-style-image: url("chrome://global/skin/icons/arrow-dropdown-12.svg");
• -moz-context-properties: fill;
• fill: currentColor;

It seems a similar arrow is already applied at
https://searchfox.org/comm-central/source/mozilla/toolkit/themes/windows/
global/button.css#71, why doesn't our button pick it up and then we do not
need this block in our css files?

You reference a rule from Windows which doesn't exist on Linux.

Comment 80

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #79)

+button[type="menu"] > .button-box > .button-menu-dropmarker {

So this needs to be moved for the other platforms too.
No, Mac and Windows have the needed rules in toolkit. Linux not.

It seems to me you add those platforms in this same patch, just with the id, not button[type="menu"]...

@@ +32,5 @@
It seems a similar arrow is already applied at
https://searchfox.org/comm-central/source/mozilla/toolkit/themes/windows/
global/button.css#71, why doesn't our button pick it up and then we do not
need this block in our css files?

You reference a rule from Windows which doesn't exist on Linux.

So a toolkit bug?

Comment 81

[Richard Marti (:Paenglab)]

Attachment: 1096006-AM-in-prefsTab-3.1.diff.patch

(In reply to :aceman from comment #80)

(In reply to Richard Marti (:Paenglab) from comment #79)

+button[type="menu"] > .button-box > .button-menu-dropmarker {

So this needs to be moved for the other platforms too.
No, Mac and Windows have the needed rules in toolkit. Linux not.

It seems to me you add those platforms in this same patch, just with the id, not button[type="menu"]...

Okay, Mac had this rule too, fixed. This bug is too old that I have everything in mind what I've done.

@@ +32,5 @@
It seems a similar arrow is already applied at
https://searchfox.org/comm-central/source/mozilla/toolkit/themes/windows/
global/button.css#71, why doesn't our button pick it up and then we do not
need this block in our css files?

You reference a rule from Windows which doesn't exist on Linux.

So a toolkit bug?

They don't use it.

Comment 82

[:aceman]

Attachment: AMtab-06-08.patch

It looks quite good now.
I removed the OK button and now we save the AM settings when:

1. a different prefs pane is selected.
2. the prefs tab is closed.

In the dialog, there were some checks run on the AM data, when the OK button was clicked.
Now those checks run immediately when a different pane inside AM is selected. If there is a big problem an error dialog is shown and the pane switch is prevented.
Those same checks run also on save of the data (at the events described above) but then the error is shown, but the user has no way to fix the data (a different pane is already shown, or the prefs tab closed), so in that case the bad input is reverted to previous value to fix that bad data.
This already happened in some cases in the old AM, I just made this consistent for all fields that are checked.
Before anybody asks, the checks intentionally do not run just on blur of the particular input field, but later to also check/fix fields that the user didn't change in this session. There were some bugs where we needed this.

I included the global/skin and messenger/skin/preferences/preferences.css into the account manager .xul files to get the right padding in the textbox[type="number"] elements that you fixed in the other bug.

So if you can guys, please play with this and check if it looks sane.

This does not update the tests yet, but will be done next.

I also made some cleanups and optimizations, e.g. the AccountManager.xul file does not even need to ship in TB now.
The AccountManager.js file is used in both SM and TB but slightly differently. Also the movement of the checks has the potential to break Seamonkey. Ian, it would be great if you could check that.

Comment 83

[Jorg K (CEST = GMT+2)]

Comment on attachment 9070799 [details] [diff] [review]
AMtab-06-08.patch

This works well enough. I didn't test it thoroughly, but I tried a few error situation, like empty or duplicate user name (on same server and account type), duplicate account name.

I think this is nice enough and could be refined during the beta cycle, if necessary. It would be great to get the tests going, see comment #62.

Also, we need to do the sub-dialogues, bug 1202480.

Comment 84

[Richard Marti (:Paenglab)]

Comment on attachment 9070799 [details] [diff] [review]
AMtab-06-08.patch

Looks good for me. Many thanks.

Comment 85

[Richard Marti (:Paenglab)]

Attachment: AMtab-06-09.patch with less space below headers in AM.

This is the same patch as from aceman. The only change that the header (the fat titles in AM) have the margin-bottom removed to reduce unneeded white space.

I'm not obsoleting aceman's patch to let the open f? active.

Comment 86

[Geoff Lankow (:darktrojan)]

Comment on attachment 9070799 [details] [diff] [review]
AMtab-06-08.patch

I'd really want to see the prefs saved when switching tabs, otherwise users will expect change that doesn't happen. ("I ticked this box then went back to the other tab and it didn't fix anything!")

I think I'd do this by, in preferencesTab.js, listening for TabSelect events on #tabmail-tabs, working out if the we're leaving the preferences tab, and if we are, firing a custom event into the content document.

Other than that I think this is in a good state to land, so we can tick off the remaining issues separately.

Comment 87

[Jorg K (CEST = GMT+2)]

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

I'd really want to see the prefs saved when switching tabs, otherwise users
will expect change that doesn't happen.

Isn't that what it's doing already? What am I missing?

EDIT: Ah, not saving when switching tabs, does save when switching panes.

Comment 88

[Ian Neal]

Comment on attachment 9070799 [details] [diff] [review]
AMtab-06-08.patch

>+++ b/mailnews/base/prefs/content/AccountManager.js
>@@ -134,27 +137,58 @@ function onLoad() {
>     selectPage = window.arguments[0].selectPage;
>   }
> 
>   accountArray = {};
>   gGenericAttributeTypes = {};
> 
>   gAccountTree.load();
> 
>-  setTimeout(selectServer, 0, selectedServer, selectPage);
Should this not be removed but instead moved to the else part of the statement below?
>-
>-  // Make sure the account manager window fits the screen.
>-  document.getElementById("accountManager").style.maxHeight =
>-    (window.screen.availHeight - 30) + "px";
>+  if (document.getElementById("paneAccount")) {
>+    // We are in a preferences tab.
>+    let finishLoading = function(aServer, aPage) {
>+      selectServer(aServer, aPage);
>+      let prefPane = document.getElementById("paneAccount");
>+      if (prefPane) {
>+        // If we are in a preferences tab, we didn't get arguments via "window".
>+        // We need to notify the caller that the account tree is built so that
>+        // he can now select a specific account if he wants.
>+        prefPane.dispatchEvent(new CustomEvent("scriptload"));
>+        prefPane.setAttribute("scriptLoaded", true);
>+      }
>+    };
>+    setTimeout(finishLoading, 0, selectedServer, selectPage);
>+  } else {
>+    // Make sure the account manager window fits the screen.
>+    document.getElementById("accountManager").style.maxHeight =
>+      (window.screen.availHeight - 30) + "px";
>+  }

Comment 89

[:aceman]

Attachment: 1096006-06-17.patch

OK, let's get some traction here.
This seems to more or less work now, so I suggest we get it to more users on trunk to get some testing before beta of 68.

It does not incorporate the suggestion of switching tabs from Geoff, we can do that in an additional patch collecting issues if any crop up.

Comment 90

[:aceman]

Comment on attachment 9072557 [details] [diff] [review]
1096006-06-17.patch

r+ from me for the parts by Paenglab and Darktrojan and eslint fixes by Jorg.

Comment 91

[:aceman]

Attachment: 1096006-06-17-tests.patch

The needed changes to tests so that they find the AM in the tab, not in a dialog.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=09c8eaaeff1bb367cb5ba68f515f557a22f2f8c1

Sadly, I had to disable 3 tests that do not pass yet:
1.function disabled_test_invalid_hostname()
This writes an invalid value into hostname of an account and then switches the settings pane. There are checks that prevent this and revert the click back to the Server settings pane of the tree. We tested manually that this works, but somehow in the test is isn't working right. May just be some timing or event ordering issue.

2.function disabled_test_archive_enabled()
3.function disabled_test_disable_archive()
These fail with similar symptoms to bug 1558955. The test uses a rather rare scenario where it sets one account to archive messages into another one and then the latter one is removed. The former one should recover, but it somehow fails to populate the Archive folder picker, similar to bug 1558955.

So I think we can live with these disabled for a few days, to allow the main patch some baking and user testing on trunk.

Comment 92

[Richard Marti (:Paenglab)]

Comment on attachment 9072557 [details] [diff] [review]
1096006-06-17.patch

I found a issue in IRC: the password field has two borders.

I'll fix this and the not aligned menulist in IMAP/Server settings you found in a follow-up bug.

Comment 93

[Geoff Lankow (:darktrojan)]

Comment on attachment 9072557 [details] [diff] [review]
1096006-06-17.patch

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

R+ with some comments.

::: mail/components/preferences/AccountManager.inc.xul
@@ +1,3 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# 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/.

This file should be named accounts.inc.xul.

Apparently the linter doesn't check inline script tags in XUL files, as there's multiple linting problems.

@@ +7,5 @@
> +    <script src="chrome://messenger/content/am-prefs.js"/>
> +    <script src="chrome://messenger/content/accountUtils.js"/>
> +    <script src="chrome://messenger/content/amUtils.js"/>
> +    <script><![CDATA[
> +      window.addEventListener("paneSelected", function firstBuild() {

Why does this function have a name? Also event is undefined.

@@ +18,5 @@
> +        if (event.detail.oldPane == "paneAccount") {
> +          onAccept(false);
> +        }
> +      });
> +      window.addEventListener("unload", () => { onAccept(false); onUnload()});

I'd rather this function wasn't on one line like this.

::: mail/components/preferences/preferences.js
@@ +146,5 @@
>  }
>  
> +function selectAMServer(serverID, panelID) {
> +  window.selectServerByKey(serverID, panelID);
> +}

Why have a separate function for this?

::: mailnews/base/prefs/content/AccountManager.js
@@ +154,5 @@
> +        prefPane.dispatchEvent(new CustomEvent("scriptload"));
> +        prefPane.setAttribute("scriptLoaded", true);
> +      }
> +    };
> +    setTimeout(finishLoading, 0, selectedServer, selectPage);

Why'd you define a variable with this function?

@@ +265,2 @@
>   */
> +function onAccept(abort) {

Seems like the variable name could be more descriptive. abortOnFailure, maybe?

@@ +515,2 @@
>   */
> +function checkUserServerChanges(silent) {

Same as above, especially as even with silent = true there's alert prompts. Maybe restoreValuesOnFailure?

@@ +608,5 @@
> +
> +/**
> + * Check if the Local Directory path was changed.
> + */
> +function checkLocalPathChanges(silent) {

… and here

@@ +654,5 @@
>  
>  /**
>   * If account name is not valid, alert the user.
>   */
> +function checkAccountNameIsValid(silent) {

… and here.

@@ +659,4 @@
>    if (!currentAccount)
>      return true;
>  
> +  var accountValues = getValueArrayFor(currentAccount);

let

::: mailnews/base/prefs/content/am-identities-list.js
@@ +77,5 @@
>  function openIdentityEditor(identity) {
> +  let args = { identity, account: gAccount,
> +               width: window.arguments[0].width,
> +               height: window.arguments[0].height,
> +               result: false };

Please give each member its own line.

::: mailnews/base/prefs/content/am-main.js
@@ +45,5 @@
>  
> +  var args = { account: gAccount, accountName,
> +               width: document.documentElement.clientWidth,
> +               height: document.documentElement.clientHeight,
> +               result: false };

Please put all these members on their own line.

::: mailnews/base/prefs/content/am-serverwithnoidentities.xul
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

Duplicate line.

Comment 94

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072559 [details] [diff] [review]
1096006-06-17-tests.patch

Wow, let's get an equally detailed review here ;-)

Comment 95

[Ben Bucksch (:BenB)]

Do you plan to merge this to Thunderbird 68? If so, we need to adapt Owl and ExQuilla.

Comment 96

[Jorg K (CEST = GMT+2)]

Yes, I do, see comment #0. Since the stand-alone options windows has gone, the stand-alone AM should go, too. So your add-ons mess in the AM, yes, I vaguely remember seeing this in ExQuilla.

Comment 97

[Ben Bucksch (:BenB)]

your add-ons mess in the AM, yes, I vaguely remember seeing this in ExQuilla.

Yes, we have to, because we add account types, and the account manager (AM) currently hardcodes "imap" and "pop3" etc.
We'll take care of that.

Comment 98

[Jorg K (CEST = GMT+2)]

Good. We should get it landed soon so you have time to try it on Daily and later on beta.

Comment 99

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #97)

Yes, we have to, because we add account types, and the account manager (AM) currently hardcodes "imap" and "pop3" etc.
Hi, if you have some suggestions on how to remove some server type hardcoding, I would be interested in that (please file a bug), like we have in bug 1238271.

Comment 101

[:aceman]

Thanks for the great finds :)

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

::: mail/components/preferences/AccountManager.inc.xul
Apparently the linter doesn't check inline script tags in XUL files, as
there's multiple linting problems.

Good, I'll split it into a separate file with the proper structure as other pref pane files.

@@ +7,5 @@

•  window.addEventListener("paneSelected", function firstBuild() {
  

Why does this function have a name? Also event is undefined.

Remnant from times when we wanted to remove the listener/function.

::: mail/components/preferences/preferences.js

+function selectAMServer(serverID, panelID) {

• window.selectServerByKey(serverID, panelID);
  +}

Why have a separate function for this?

Some remnant, maybe it was longer in the past.

::: mailnews/base/prefs/content/AccountManager.js
@@ +154,5 @@

•    prefPane.dispatchEvent(new CustomEvent("scriptload"));
  

•    prefPane.setAttribute("scriptLoaded", true);
  

•  }
  

• };
• setTimeout(finishLoading, 0, selectedServer, selectPage);

Why'd you define a variable with this function?

You can't define a function not on top-level of a function without a variable (you get JS warning).
Anyway, this all seems to be duplicate of the event account-tree-built I added later so it can all go.
Also, we do not have selectedServer and selectPage when in a tab (as no window argument) so this was bogus.

@@ +659,4 @@

if (!currentAccount)
return true;

• var accountValues = getValueArrayFor(currentAccount);

let

Why? We use 'var' in top level of function. But I would be happy with 'let' everywhere.

Comment 102

[:aceman]

Attachment: 1096006-06-18.patch

Thanks, addressed the review comments.

Comment 103

[:aceman]

Attachment: 1096006-06-18-tests.patch

Some tweaks to the tests too.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=252339490&revision=ad9c121a7289dc9cd44b8378d2c77cedb3530858

Comment 104

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #101)

Why'd you define a variable with this function?

You can't define a function not on top-level of a function without a variable (you get JS warning).
Anyway, this all seems to be duplicate of the event account-tree-built I added later so it can all go.
Also, we do not have selectedServer and selectPage when in a tab (as no window argument) so this was bogus.

What I meant was, why not just do setTimeout(function() …)?

Why? We use 'var' in top level of function. But I would be happy with 'let' everywhere.

The rest of that function is using let. Not major, it just seemed inconsistent.

Comment 105

[Ben Bucksch (:BenB)]

why not just do setTimeout(function() …)?

You mean setTimeout(() => ...) :-)

Comment 106

[Ben Bucksch (:BenB)]

One of the biggest pain points we have is that each pane currently (without this patch) does not re-load and re-initialize when you switch to another server. We e.g. disable UI widgets for features that we cannot support. But we need to make sure to put them back into original state after the user switches to an IMAP account. It would be much easier, if each pane would re-load for each pane change. (I haven't looked at your patch, whether it does that.)

Comment 107

[Geoff Lankow (:darktrojan)]

Comment on attachment 9072726 [details] [diff] [review]
1096006-06-18-tests.patch

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

This looks a lot tidier without so many callbacks and subtests. Nice.

::: mail/test/mozmill/account/test-account-port-setting.js
@@ -62,5 @@
> -function subtest_check_port_number(amc) {
> -  subtest_check_set_port_number(amc, true);
> -}
> -
> -function setupModule(module) {

Thanks for moving this. Seems like it got lost at some point.

::: mail/test/mozmill/account/test-account-values.js
@@ +50,5 @@
>    gPopAccount = MailServices.accounts.createAccount();
>    gPopAccount.incomingServer = popServer;
>    gPopAccount.addIdentity(identity);
>  
> +//  dump("ACE:pop account=" + gPopAccount.incomingServer.serverURI + "=" + gPopAccount.incomingServer.prettyName);

Still needed? If it is, at least fix the indent.

And some more of these further down.

::: mail/test/mozmill/shared-modules/test-account-manager-helpers.js
@@ +64,5 @@
> +                             .getAttribute("PageTag")));
> +}
> +
> +/**
> + * Opens the Account Manager in pre Preferences tab.

Hmm…

@@ +83,5 @@
>    aController.e("advanced-setup_button").click();
> +  var tabmail = mc.e("tabmail");
> +  tabmail.selectTabByMode("preferencesTab");
> +  var tab;
> +  mc.waitFor(() => (tab = tabmail.getTabInfoForCurrentOrFirstModeInstance(tabmail.tabModes.preferencesTab)) != null,

Can we wrap this line somewhere?

::: mail/test/mozmill/shared-modules/test-pref-window-helpers.js
@@ +51,5 @@
>   *
>   * @param aTab  The content tab to close.
>   */
>  function close_pref_tab(aTab) {
> +  fdh.assert_true(!!aTab, "No Preferences tab specified for closing");

The !! is unnecessary.

Comment 108

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072725 [details] [diff] [review]
1096006-06-18.patch

We can go with Geoff's review here, I can't do any better.
So authors:
# User Richard Marti <[e-mail]>, Geoff Lankow <[e-mail]>, aceman <[e-mail]>
r=aceman,darktrojan

Comment 109

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072726 [details] [diff] [review]
1096006-06-18-tests.patch

r=darktrojan will do here.

Comment 110

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2b61ea9688aa
Migrate the Account Manager from a standalone window to a pane in the Preferences tab. r=aceman,darktrojan; ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/b9e033308483
Adapt mozmill tests to Account Manager being in the Preferences tab. r=darktrojan

Comment 111

[:aceman]

Thanks.
I'll file the bugs for the remaining tasks (re-enabling the tests and saving on tab switch).

Comment 112

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072725 [details] [diff] [review]
1096006-06-18.patch

We'll take this for beta 2 or beta 3, of with test patch as well. I haven't tried you whether the patches apply.

Comment 113

[Ben Bucksch (:BenB)]

I just tested trunk, and I'm a bit surprised.

I don't remember where I saw it, but I saw a screenshot (IIRC of actual running code in TB) where each account was listed on the left, on the same level as the "Display", "Privacy" etc. preference panes. Below, there was "Account", followed by an entry for each account directly below.

What I see now is that the account list is another list to the right of the preferences sections. That
a) creates another level of hierarchy, which is difficult for users
b) reduces the horizontal space dramatically.

Comment 114

[Ben Bucksch (:BenB)]

Attachment: Screenshot, new profile, Linux

Notice that the way to add an account is hard to find.
On the screenshot I saw, there was a [+] on the left pane, below the list of accounts. With no accounts, it would be directly below "Accounts"

Comment 115

[Ben Bucksch (:BenB)]

(I didn't make it deliberately tall. That's the default for a new profile on my system.)

Comment 116

[Ben Bucksch (:BenB)]

I filed bug 1560344 about this.

We can't release Thunderbird 68 like this. This is hideous.

Comment 117

[:aceman]

There are some mockups in bug 286664, but with the old Preferences dialog.
Maybe you find something else.
But if we had to have something for TB68 we couldn't do more, just stuck the whole AM into a preferences pane as is.
Any further improvements can now be done freely on trunk.

Thanks for filing the improvement bugs.

Comment 118

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #114)

Notice that the way to add an account is hard to find.
On the screenshot I saw, there was a [+] on the left pane, below the list of accounts. With no accounts, it would be directly below "Accounts"

Isn't this on your 4K screen? Don't all apps look like this, if you do not scale 2x? :)

Comment 119

[Ben Bucksch (:BenB)]

But if we had to have something for TB68 we couldn't do more

Sorry to be so harsh, but this can't ship as-is. Either bug 1560344 can be fixed for TB 68, or this patch here should not land for TB 68.

I really appreciate the work, but the work should continue on trunk only until it's ready.

Comment 120

[Magnus Melin [:mkmelin]]

Agreed that we should backport only when and if it's polished enough.

Comment 121

[Onno Ekker [:nONoNonO UTC+1]]

Attachment: Sample of a tidy account list without tabs/panes per group

Comment 122

[Onno Ekker [:nONoNonO UTC+1]]

Comment on attachment 9073372 [details]
Sample of a tidy account list without tabs/panes per group

Sorry, posted this on the wrong bug

Comment 123

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072725 [details] [diff] [review]
1096006-06-18.patch

Looks like people aren't 100% happy with it yet.

Comment 124

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fac9fd23d58a
Backout of AM in pref tab. a=backout

Comment 125

[Jorg K (CEST = GMT+2)]

According to the discussion in bug 1560344 we don't want "AM in prefs tab" but rather "AM in its own tab". Let's file a bug for that and aim for TB 76.

Comment 126

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+1) (PTO to 26th Jan 2020, sporadically reading bugmail) from comment #125)

According to the discussion in bug 1560344 we don't want "AM in prefs tab" but rather "AM in its own tab". Let's file a bug for that and aim for TB 76.

Did anybody file a bug? Is anybody working on this?

Comment 127

[Magnus Melin [:mkmelin]]

Filed bug 1610445.