Closed Bug 1610445 Opened 5 years ago Closed 5 years ago

make account manager live in a tab (about:accountsettings)

Categories

(Thunderbird :: Account Manager, task, P2)

Product:
Thunderbird
Component:
Account Manager
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 74.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug-1610445_account-manager-in-tab-3.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
Bug-1610445_follow-up_prefs-from-chat-account-0.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

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

Bug 1096006 was ultimately backed out, but the goal to have the account manager be in a tab still remains. This is the bug to track that work: make the account manager open as a tab instead of a separate dialog, and related work.

Apparently there's already a prototype implementation by alta88 for this, see attachments of bug 1468167: attachment 8985174 [details], attachment 8985198 [details] and attachment 8985554 [details]. Apparently all done in HTML instead of XUL. Should we use that instead of reinventing it?

I'm not sure how much can be reused from what, certainly a bunch of the code from bug 1096006 will likely be needed. I think it would make sense to do this incrementally: just make the current account manager UI live in a tab.

Summary: make account manager live in a tab (about:accounts) → make account manager live in a tab (about:accountmanager)
Assignee: nobody → khushil324
Attached patch Bug-1610445_account-manager-in-tab-0.patch (obsolete) (deleted) — — Splinter Review
Attachment #9123487 - Flags: review?(mkmelin+mozilla)
Attachment #9123487 - Attachment description: Bug-1610445_account-manager-in-tab.patch → Bug-1610445_account-manager-in-tab-0.patch
Attachment #9123487 - Attachment filename: Bug-1610445_account-manager-in-tab.patch → Bug-1610445_account-manager-in-tab-0.patch
Status: NEW → ASSIGNED
Comment on attachment 9123487 [details] [diff] [review] Bug-1610445_account-manager-in-tab-0.patch Review of attachment 9123487 [details] [diff] [review]: ----------------------------------------------------------------- This alone isn't enough. It shows in a tab, sure. But basically nothing works: no dialogs open and there are still the ok/cancel buttons you'll need to get rid of (and make it instant apply) (Some things might be available in the backed out patch). Also tests need to be updated for sure.
Attachment #9123487 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9124365 [details] [diff] [review] Bug-1610445_account-manager-in-tab-1.patch Review of attachment 9124365 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, just some nits and one problem: For RSS, the Manage Subscriptions... button doesn't work (error in console) JavaScript error: chrome://messenger-newsblog/content/newsblogOverlay.js, line 303: NS_ERROR_ILLEGAL_VALUE: ::: mail/components/im/content/imAccounts.js @@ +4,5 @@ > > /* globals MozElements */ > // imStatusSelector.js > /* globals statusSelector */ > +// mailnews/base/prefs/content/accountUtils.js I think we don't need comments like this ::: mail/components/newmailaccount/content/accountProvisioner.js @@ +1,5 @@ > /* 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/. */ > > +// mailnews/base/prefs/content/accountUtils.j please remove. also: typo ::: mail/test/browser/account/browser_actions.js @@ +82,5 @@ > + * @param accountKey the key of the account to select. > + * @param isSetAsDefaultEnabled true if the menuitem should be enabled, false otherwise. > + * @param isRemoveEnabled true if the menuitem should be enabled, false otherwise. > + * @param isAddAccountEnabled true if the menuitems (Add Mail Account+Add Other Account) > + * should be enabled, false otherwise. please fix the documentation style while you're touching this. e.g. @param {boolean} isAddAccountEnabled - true if the menuitems (Add Mail Account+Add Other Account) is expected to be enabled. ::: mail/test/browser/account/browser_mailAccountSetupWizard.js @@ +129,5 @@ > actual: incoming.username, > expected: user.email.split("@")[0], > }, > + // This was creating test failure but we were not using Assert.equal to > + // test it so it was not showing any oranges. We should deal with that somehow then. If it already exists on trunk, maybe in a follow-up
Attachment #9124365 - Flags: review?(mkmelin+mozilla) → feedback+

Maybe it would also be preferable to make it about:accountsettings instead

Summary: make account manager live in a tab (about:accountmanager) → make account manager live in a tab (about:accountsettings)
Comment on attachment 9124547 [details] [diff] [review] Bug-1610445_account-manager-in-tab-2.patch Review of attachment 9124547 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some doc style to fix (I've only mentioned each thing once), and the question about the test failure ::: mail/test/browser/account/browser_actions.js @@ +77,5 @@ > > /** > * Check that the account actions for the account are enabled or disabled appropriately. > * > + * @param {Object} tab - the account manager tab. Please capitalize and skip the alignment, like @param {Object} tab - The account manager tab. ::: mail/test/browser/account/browser_mailAccountSetupWizard.js @@ +128,5 @@ > "incoming server username": { > actual: incoming.username, > expected: user.email.split("@")[0], > }, > + // This was creating test failure. so what about this? ::: mail/test/browser/shared-modules/AccountManagerHelpers.jsm @@ +74,3 @@ > * > + * @param {Object} tab - The account manger tab controller that opened. > + * @param {number} rowIndex - The row to click. I think it should be {Number}
Attachment #9124547 - Flags: review?(mkmelin+mozilla) → review+

(In reply to Magnus Melin [:mkmelin] from comment #9)

so what about this?

I don't have much idea about this failure. It is related to basic server configuration. Do you know any experts? I will open a bug and we can discuss it over there.

(In reply to Magnus Melin [:mkmelin] from comment #9)

I think it should be {Number}

JSDoc suggests using {number} : https://jsdoc.app/tags-param.html
And we are using {number} and {Number} both at various places that look ugly. So what do you suggest so we can have some consistency?

AIUI, {Number} is more correct because that is the build in type. It probably doesn't matter much.

Attachment #9124547 - Attachment is obsolete: true
Attachment #9124754 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/2dbbeabc26b1
make account manager live in a tab - about:accountsettings . r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Landed. Now we just need to make it prettier ;)

Target Milestone: --- → Thunderbird 74.0

Do we have any mock up or anything?

No, but Alex will take a look at some point.

I'll file a bug as a follow up of this and do some UI.
I think we should be able to tackle this pretty quickly with Richard as well, in order to not leave it in this state for too long.

"Properties" button is not working in chat accounts dialog i.e it is not opening up the account manager.

Attachment #9125388 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9125388 [details] [diff] [review] Bug-1610445_follow-up_prefs-from-chat-account-0.patch Review of attachment 9125388 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I do notice that if the account settings tab is already open, it's not necessarily going to the right account (shows a mail one for me). But I think that's a separate bug.
Attachment #9125388 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/739f20b22fcc follow up - fix properties button not working in chat accounts dialog. r=mkmelin DONTBUILD
Regressions: 1614469
Depends on: 1615417
Regressions: 1620206
Regressions: 1625576
Regressions: 1653588
Regressions: 1657050

quite many regressions. Do we need some tests in this area?

Flags: needinfo?(mkmelin+mozilla)

Yes, in general we should add more tests.

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

Attachment

General

Creator:
Created:
Updated:
Size: