Closed Bug 1615417 Opened 1 year ago Closed 1 year ago

Account Manager: Convert the dialogs to subdialogs

Categories

(Thunderbird :: Account Manager, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 75.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 5 obsolete files)

Like the prefs dialogs we should convert the Account Manager dialogs to in-content subdialogs.

Attached patch WIP 1615417-AM-subdialog.patch (obsolete) — Splinter Review

This is a WIP. Only the global return receipts, the two certificate dialogs in "Security" and the "Edit SMTP Server" dialogs are converted.

I have the problem, that the dialogs don't stop the execution of the calling JS until they are closed. This makes that the calling doesn't know when the dialog is closed and the content should be reloaded when something has changed. I tried similar to https://searchfox.org/comm-central/source/mail/components/preferences/general.js#1032 with a closing callback but it seems it doesn't work because I need a parent.gSubDialog.open() to to call the dialog in the iframe.

For example, when I create a new SMTP config with "Add...", the config is created but the listbox doesn't update to show the new entry.

Alessandro, do you have maybe a solution for this issue? Then I can try this with the other dialogs.

Attachment #9126538 - Flags: feedback?(alessandro)
Attachment #9126538 - Attachment is patch: true

I tried updating the editSMTPServer() method to trigger a callback once the dialog is closed, and it seemed to work.

parent.gSubDialog.open(
  "chrome://messenger/content/SmtpServerEdit.xhtml",
  null,
  args,
  testCallback
);

function testCallback() {
  console.log("callback");
}

Was this what you were trying to do?

Another approach we could try is to add observers to those lists and select elements in order to be able to listen to any change and ignore this whole subdialog callback thingy.

Attachment #9126538 - Flags: feedback?(alessandro)

(In reply to Alessandro Castellani (:aleca) from comment #2)

Was this what you were trying to do?

Yes, it was this. I tried it on am-main.js with opening the am-identities-list.xhtml. I'll try the SmtpServerEdit.xhtml this evening.

Attached patch 1615417-AM-subdialog.patch (obsolete) — Splinter Review

This works now, except two dialogs:

Alessandro, Magnus, please can you show me with a small example how to open a subdialog from a subdialog? We could use this in the prefs for the Edit LDAP directories dialog.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9126949 - Flags: feedback?(mkmelin+mozilla)
Attachment #9126949 - Flags: feedback?(alecsandra.sandulescu)
Comment on attachment 9126949 [details] [diff] [review]
1615417-AM-subdialog.patch

Sorry got the wrong person for the f?
Attachment #9126949 - Flags: feedback?(alecsandra.sandulescu) → feedback?(alessandro)
Attached patch 1615417-identities-in-main.patch (obsolete) — Splinter Review

This is the follow-up patch on top of the first patch to move the identities dialog to the main pane. This makes it possible to directly open the Edit Identity dialog as subdialog. But this dialog has other buttons to open additional dialog which needs a solution (this doesn't work actually). I need the solution to open a subdialog from a subdialog.

Alessandro and Magnus, what do you think about this solution? If you think this would get a go, please check if the new description above the identities list would be okay like this.

Attachment #9126538 - Attachment is obsolete: true
Attachment #9126951 - Flags: feedback?(mkmelin+mozilla)
Attachment #9126951 - Flags: feedback?(alessandro)
Comment on attachment 9126951 [details] [diff] [review]
1615417-identities-in-main.patch

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

::: mail/locales/en-US/chrome/messenger/am-identities-list.dtd
@@ +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/. -->
>  
> +<!ENTITY identitiesListDesc.label "Manage the identities for this account. The first identity is used by default and showed above.">

localization key must be changed so localizers can pick up

::: mail/locales/en-US/chrome/messenger/am-identity-edit.dtd
@@ +4,5 @@
>  
>  <!-- LOCALIZATION NOTE (identityDialog.style): This value should be roughly
>       equal to the value of accountManager.size entity minus the value
>       of accountTree.width entity. -->
> +<!ENTITY identityDialog.size    "width: 75ch; height: 56em;">

this key would also need to be bumped

But maybe we don't need to have a localization value for this anymore?

::: mailnews/base/prefs/content/am-identities-list.js
@@ +90,5 @@
>    let indexToSelect = identity
>      ? gIdentityListBox.selectedIndex
>      : gIdentityListBox.itemCount;
>  
> +  if (AppConstants.MOZ_APP_NAME == "thunderbird") {

I don't think we should be special casing thunderbird. 
In the unlikely case SeaMonkey ever try to resurrect things, they would still need to do the same things as Thunderbird.

(Across the patch)

::: mailnews/base/prefs/content/am-main.xhtml
@@ +38,5 @@
> +    <commandset>
> +      <command id="cmd_add"     oncommand="openIdentityEditor(null);"/>
> +      <command id="cmd_edit"    oncommand="onEdit(event);" disabled="true"/>
> +      <command id="cmd_default" oncommand="onSetDefault(event);" disabled="true"/>
> +      <command id="cmd_delete"  oncommand="onDelete(event);" disabled="true"/>

I think aligning attributes like this is futile. Please just one space
Attachment #9126951 - Flags: feedback?(mkmelin+mozilla)
Attachment #9126951 - Flags: feedback?(alessandro)
Comment on attachment 9126949 [details] [diff] [review]
1615417-AM-subdialog.patch

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

All in all, I don't quite like how the identities are now listed at the bottom of the main account page. 
I think it all needs to be rethought. Now the information goes in all directions: first vertical in the tree, then when editing horizontally in tabs. 
Then the account main page is really (almost) the same as what is in an identity dialog.
Attachment #9126949 - Flags: feedback?(mkmelin+mozilla)

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

Comment on attachment 9126951 [details] [diff] [review]
1615417-identities-in-main.patch

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

::: mail/locales/en-US/chrome/messenger/am-identities-list.dtd
@@ +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/. -->

+<!ENTITY identitiesListDesc.label "Manage the identities for this account. The first identity is used by default and showed above.">

localization key must be changed so localizers can pick up

Entity changed from identitiesListManageDesc.label to identitiesListDesc.label.

::: mail/locales/en-US/chrome/messenger/am-identity-edit.dtd
@@ +4,5 @@

<!-- LOCALIZATION NOTE (identityDialog.style): This value should be roughly
equal to the value of accountManager.size entity minus the value
of accountTree.width entity. -->
+<!ENTITY identityDialog.size "width: 75ch; height: 56em;">

this key would also need to be bumped

Also this changed from identityDialog.style to identityDialog.size.

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

Comment on attachment 9126949 [details] [diff] [review]
1615417-AM-subdialog.patch

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

All in all, I don't quite like how the identities are now listed at the
bottom of the main account page.
I think it all needs to be rethought. Now the information goes in all
directions: first vertical in the tree, then when editing horizontally in
tabs.
Then the account main page is really (almost) the same as what is in an
identity dialog.

This patch is for the status quo but only with in-content subdialogs. The other was as proposal for the identities list.

Do you have an answer for how I need to call a subdialog from a subdialog (https://searchfox.org/comm-central/source/mail/components/preferences/subdialogs.js#10)? Then the other patch isn't needed as it is.

Sorry I don't know.

Attached patch 1615417-AM-subdialog.patch (obsolete) — Splinter Review

Nothing special needed for subdialog from subdialog. :-)

This patch changes only that the dialogs open in subdialogs instead separate dialogs. Only the "Cert Picker" and the "LDAP directory edit" dialogs don't open in-contnet because they are invoked through C++.

No special casing for Thunderbird now. SM has to follow with in-content dialogs.

Attachment #9126949 - Attachment is obsolete: true
Attachment #9126951 - Attachment is obsolete: true
Attachment #9126949 - Flags: feedback?(alessandro)
Attachment #9127141 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9127141 [details] [diff] [review]
1615417-AM-subdialog.patch

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

Seems to work, thx! r=mkmelin

::: mailnews/base/prefs/content/AccountManager.xhtml
@@ +21,5 @@
>  #ifdef MOZ_SUITE
>  <script src="chrome://messenger/content/am-help.js"/>
>  #endif
>  <script src="chrome://messenger/content/amUtils.js"/>
> +#ifdef MOZ_THUNDERBIRD

probably not much point having this ifdef thunderbird

@@ +91,3 @@
>      <iframe id="contentFrame" name="contentFrame" flex="1"/>
>    </hbox>
> +#ifdef MOZ_THUNDERBIRD

and this

::: mailnews/base/prefs/content/am-server.js
@@ +262,3 @@
>    );
>  
> +  function onCloseAdvanced() {

please use 
let onCloseAdvanced = function() {

... not to get strict js errors

@@ +269,5 @@
> +        serverSettings.usingSubscription;
> +      document
> +        .getElementById("imap.maximumConnectionsNumber")
> +        .setAttribute("value", serverSettings.maximumConnectionsNumber);
> +      // string prefs

please remove this comment while here

::: mailnews/base/prefs/content/amUtils.js
@@ +267,5 @@
> +  function onCloseSMTPDialog() {
> +    if (args.result) {
> +      gSmtpServerListWindow.refreshServerList(aServer, true);
> +    }
> +  }

and move this function up before it's used.... (+ use let...)
Attachment #9127141 - Flags: review?(mkmelin+mozilla) → review+

There's an error found by lint too.

Attached patch 1615417-AM-subdialog.patch (obsolete) — Splinter Review

Review comments and ESLint failures fixed. I have some test failures: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=289336055.
It looks like the test wants to open the dialog as window, but now it's in-content.

Geoff, Magnus, please could you look what is needed to fix this errors? I don't know this mochitests not at all.

Attachment #9127141 - Attachment is obsolete: true
Attachment #9127316 - Flags: feedback?(mkmelin+mozilla)
Attachment #9127316 - Flags: feedback?(geoff)

I suspect that's not a test failure, but in the archive options something's off with gIdentity

[task 2020-02-18T12:24:08.906Z] 12:24:08 INFO - Console message: [JavaScript Error: "TypeError: can't access property "archiveGranularity", gIdentity is undefined" {file: "chrome://messenger/content/am-archiveoptions.js" line: 19}]

I thought it has something to do with this: https://searchfox.org/comm-central/source/mail/test/browser/account/browser_archiveOptions.js#106 that it waits for a window that never appears.

Comment on attachment 9127316 [details] [diff] [review]
1615417-AM-subdialog.patch

Your change to am-archiveoptions.js makes this line[1] and this line[2] wrong – should  be `{ identity }`. 

(That test is still opening the page as a real dialog, not a subdialog, which probably should be fixed too. Maybe a task for another bug.)

[1] https://searchfox.org/comm-central/rev/329e875e7ecec8349b1a09b7e44f8ec5ffd26fb3/mail/test/browser/account/browser_archiveOptions.js#120
[2] https://searchfox.org/comm-central/rev/329e875e7ecec8349b1a09b7e44f8ec5ffd26fb3/mail/test/browser/account/browser_archiveOptions.js#146

(Thanks Bugzilla for the formatting.)
Attachment #9127316 - Flags: feedback?(geoff) → feedback+
Attachment #9127316 - Attachment is obsolete: true
Attachment #9127316 - Flags: feedback?(mkmelin+mozilla)
Attachment #9127470 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1c9f4fdf7772
Implement the subdialogs in Account Manager. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 75.0
Duplicate of this bug: 1614830
You need to log in before you can comment on or make changes to this bug.