779990 - "Advanced..." button in Server Settings is in the wrong place

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[bintoro]

Subsections "Server Settings" and "Message Storage" used to form a single group of settings in Account Settings > Server Settings.

When the old group was split in two, the "Advanced..." button ended up on the wrong side of the division. It is now under Message Storage where it clearly doesn't belong. The correct section for the button would be Server Settings.

See also bug 545194 for a cosmetic problem affecting the same button on Windows. There's also a screenshot of how the settings used to be grouped.

Comment 1

[WADA:World Anti-bad-Duping Agency]

(In reply to ktuure from comment #0)
> the "Advanced..." button ended up on the wrong side of the division.
> It is now under Message Storage where it clearly doesn't belong.

Does "it is now" mean "changed by a release of Tb"? If so, between which Tb releases?

If News account, there is no "Avanced" button in "Server Settings", so no problem.
If POP3 account, "Advanced" is for Global Inbox, so "under Message Storage section" is pretty proper.
If IMAP account, as you say, "Advanced" may be better placed under "Server Settings" section.

"Why Advanced button is placed under Message Storage section" is perhaps;
(1) The "Advanced" button was initially implemented for "Global Inbox" of POP3 account, so placed under Message Storage section.
(2) Same mechanism of "Advanced" button was ported to IMAP account.
(3) "Server Settings" panel design/implementation is common among server types.

Comment 2

[:aceman]

Good job WADA.

There are facilities in the server settings to show/hide buttons/elements depending on the server type. So I see at least 3 options here:
1. split the contents of the advanced dialog into a POP3 dialog and a IMAP4 dialog and make two buttons each pointing to the proper dialog and being positioned in the proper group (and hide the buttons as needed). This would be much work in refactoring the dialog.
2. Have one Advanced dialog and one button but move it dynamically (via JS) into the proper group in the pane, depending on the server type.
3. Have one Advanced dialog (as today) but have two buttons pointing to it, each in the proper group in the pane and hide them as needed. But this is a bit of a XUL code duplication and prone to divergence in the future.

Bwinton, mconley, any ideas which implementation you would like?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

I think either of 2 or 3 should be fine.  I don't really care which one, though.  :)

Thanks,
Blake.

Comment 4

[:aceman]

WADA, for reference, the split was done in bug 340324, for TB14. And the Advanced button got into the Storage section because it is incidentally defined on the line as the "Empty Trash on Exit".

I try to implement option 2 as I am worried what happens if we had 2 buttons with the same ID (what is probably forbidden) and same prefstring value.

Comment 5

[:aceman]

Attachment: patch

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 662264 [details] [diff] [review]
patch

Yeah, this makes more sense…  ui-r=me.

Thanks,
Blake.

Comment 7

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

Comment on attachment 662264 [details] [diff] [review]
patch

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

I'm cool with this if Ian is.

Thanks aceman!

Comment 8

[Ian Neal]

Comment on attachment 662264 [details] [diff] [review]
patch

My preference is not use any JS instead have two advanced buttons, with different IDs (e.g. server.imapadvancedbutton and server.advancedbutton), but all other attributes the same except for hidefor which would be "pop3,nntp,movemail" for the imap one and "imap,nttp,movemail" for the other.

>-      <hbox align="end">
>+      <hbox align="end" id="messageStorageBox">
This change wouldn't be needed either.

Comment 9

[:aceman]

Attachment: patch v2

Comment 10

[Ian Neal]

Comment on attachment 678151 [details] [diff] [review]
patch v2

>+    <hbox align="end">
>+      <hbox align="center" id="imap.deleteModel.box" hidefor="pop3,nntp,movemail"
Nit: one attribute per line
>+            flex="1">
>+        <radiogroup wsm_persist="true" id="imap.deleteModel"
Nit: one attribute per line
>+                    oncommand="selectImapDeleteModel(this.value)"
Nit: missing ;
>+                    prefstring="mail.server.%serverkey%.delete_model">
>+          <grid class="specialFolderPickerGrid">
>+            <columns>
>+              <column/>
>+              <column flex="1"/>
>+            </columns>
>+            <rows>
>+              <row align="center">
>+                <radio id="modelMoveToTrash"
>+                       value="1" label="&modelMoveToTrash.label;"
Nit: one attribute per line
>+                       accesskey="&modelMoveToTrash.accesskey;"/>
>+                <menulist id="msgTrashFolderPicker"
>+                          aria-labelledby="modelMoveToTrash"
>+                          oncommand="folderPickerChange(event)">
Nit: missing ;
>+                  <menupopup id="msgTrashFolderPopup" type="folder" mode="filing"
Nit: one attribute per line
>+                             showFileHereLabel="true"
>+                             fileHereLabel="&filemessageschoosethis.label;"/>
>+                </menulist>
>+              </row>
>+              <row align="center">
>+                <radio id="modelMarkDeleted"
>+                       value="0" label="&modelMarkDeleted.label;"
Nit: one attribute per line
>+                       accesskey="&modelMarkDeleted.accesskey;"/>
>+              </row>
>+              <row align="center">
>+                <radio id="modelDeleteImmediately"
>+                       value="2" label="&modelDeleteImmediately.label;"
Nit: one attribute per line
>+                       accesskey="&modelDeleteImmediately.accesskey;"/>
r=me with those nits fixed.

Comment 11

[:aceman]

Attachment: patch v3 [Checked in: Comment 12]

Comment 12

[Ian Neal]

Comment on attachment 678154 [details] [diff] [review]
patch v3 [Checked in: Comment 12]

https://hg.mozilla.org/comm-central/rev/1189f87a89df