Add AccountManager to the prefs in tab

ASSIGNED
Assigned to

Status

Thunderbird
Account Manager
ASSIGNED
4 years ago
8 months ago

People

(Reporter: Paenglab, Assigned: Paenglab, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 13 obsolete attachments)

2.22 KB, patch
Magnus Melin
: review+
Details | Diff | Splinter Review
33.65 KB, patch
Details | Diff | Splinter Review
2.51 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8519521 [details] [diff] [review]
WIP - AccountPrefsInTab.patch

This is a WIP with the limitation to not select the correct account.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8519522 [details] [diff] [review]
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8547154 [details] [diff] [review]
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.
Attachment #8519521 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8547156 [details] [diff] [review]
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.
Attachment #8547154 - Attachment is obsolete: true

Comment 5

3 years ago
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
(Assignee)

Comment 6

3 years ago
Created attachment 8550725 [details] [diff] [review]
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.
Attachment #8547156 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
tracking-thunderbird38: --- → ?
tracking-thunderbird_esr38: --- → ?
(Assignee)

Comment 7

3 years ago
Created attachment 8565600 [details] [diff] [review]
[checked in for tb38] AccountPrefsInTab-strings.patch

Strings only patch for check-in before string freeze.
Attachment #8565600 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 8

3 years ago
Created attachment 8565601 [details] [diff] [review]
WIP - AccountPrefsInTab.patch

Main patch without the strings. Comment 6 is still valid.
Attachment #8550725 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8565603 [details] [diff] [review]
WIP - AM-Subdialogs.patch

Subdialog patch without the strings.
Attachment #8519522 - Attachment is obsolete: true

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
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

3 years ago
When I have tried the tabbed preferences, none of the keyboard shortcuts work. Is that a known issue?
(Assignee)

Comment 13

3 years ago
ALT+SHIFT+key works here.

Updated

3 years ago
Attachment #8565600 - Flags: review?(mkmelin+mozilla) → review+

Comment 14

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

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 15

3 years ago
(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.

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Attachment #8565600 - Attachment description: AccountPrefsInTab-strings.patch → [checked in for tb38] AccountPrefsInTab-strings.patch

Comment 17

3 years ago
(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

3 years ago
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.
tracking-thunderbird38: ? → ---
tracking-thunderbird_esr38: ? → ---

Comment 19

3 years ago
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.
(Assignee)

Comment 20

3 years ago
Created attachment 8606648 [details] [diff] [review]
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.
Attachment #8565601 - Attachment is obsolete: true
Attachment #8606648 - Flags: review?(rkent)
(Assignee)

Comment 21

3 years ago
Created attachment 8606649 [details] [diff] [review]
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.
Attachment #8565603 - Attachment is obsolete: true
Attachment #8606649 - Flags: review?(rkent)
(Assignee)

Comment 22

3 years ago
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.
Attachment #8606650 - Flags: review?(rkent)

Comment 23

3 years ago
Are the subdialogs needed? :(
(Assignee)

Comment 24

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

Comment 25

3 years ago
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

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
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.
(Assignee)

Comment 28

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

removing this patch from this bug.
Attachment #8606650 - Attachment is obsolete: true
Attachment #8606650 - Flags: review?(rkent)

Comment 29

3 years ago
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.

Updated

3 years ago
Attachment #8606648 - Flags: review?(rkent) → review?(mkmelin+mozilla)

Updated

3 years ago
Attachment #8606649 - Flags: review?(rkent) → review?(mkmelin+mozilla)
(Assignee)

Comment 30

3 years ago
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?
Flags: needinfo?(acelists)
(Assignee)

Comment 31

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

Removing the review request as it not works properly.
Attachment #8606648 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

3 years ago
Attachment #8606649 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 32

3 years ago
Created attachment 8657905 [details] [diff] [review]
AccountPrefsInTab.patch

Unbitrotted patch after Bug 1193716 landing.
Attachment #8606648 - Attachment is obsolete: true
(Assignee)

Comment 33

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

Moving this patch to a separate bug.
Attachment #8606649 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1202480

Comment 34

3 years ago
(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?
Flags: needinfo?(acelists)
(Assignee)

Comment 35

3 years ago
(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/richard.marti@gmail.com-401179c3ef11
Flags: needinfo?(acelists)
(Assignee)

Comment 36

3 years ago
Created attachment 8666512 [details] [diff] [review]
AccountPrefsInTab.patch

Unbitrotted patch.
Attachment #8657905 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8668548 [details] [diff] [review]
AccountPrefsInTab.patch

Updated to tip.
Attachment #8666512 - Attachment is obsolete: true

Updated

2 years ago
Blocks: 507640

Updated

2 years ago
Blocks: 1259904

Updated

2 years ago
Blocks: 1307773
I'm afraid I don't have the cycles to participate here, at least currently. :/ Sorry for being a blocker.
Flags: needinfo?(mconley)
(Assignee)

Comment 39

a year ago
Created attachment 8822668 [details] [diff] [review]
AccountPrefsInTab.patch

Patch updated to tip.
Attachment #8668548 - Attachment is obsolete: true

Comment 40

a year ago
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

a year ago
Created attachment 8822930 [details] [diff] [review]
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

a year ago
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

a year ago
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
(Assignee)

Comment 44

a year ago
(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

a year ago
(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

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

Comment 47

8 months ago
(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

8 months ago
(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.
You need to log in before you can comment on or make changes to this bug.