Closed Bug 779990 Opened 8 years ago Closed 7 years ago

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


(MailNews Core :: Account Manager, defect, minor)

Not set


(seamonkey2.16 fixed)

Thunderbird 19.0
Tracking Status
seamonkey2.16 --- fixed


(Reporter: kalle, Assigned: aceman)


(Keywords: polish)


(1 file, 2 obsolete files)

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.
(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.
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?
Assignee: nobody → acelists
Product: Thunderbird → MailNews Core
Version: unspecified → Trunk
I think either of 2 or 3 should be fine.  I don't really care which one, though.  :)

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.
Ever confirmed: true
Keywords: polish
Attached patch patch (obsolete) — Splinter Review
Attachment #662264 - Flags: ui-review?(bwinton)
Attachment #662264 - Flags: review?(iann_bugzilla)
Comment on attachment 662264 [details] [diff] [review]

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

Attachment #662264 - Flags: ui-review?(bwinton) → ui-review+
Attachment #662264 - Flags: review?(mconley)
Comment on attachment 662264 [details] [diff] [review]

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

I'm cool with this if Ian is.

Thanks aceman!
Attachment #662264 - Flags: review?(mconley) → review+
Comment on attachment 662264 [details] [diff] [review]

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.
Attachment #662264 - Flags: review?(iann_bugzilla) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #662264 - Attachment is obsolete: true
Attachment #678151 - Flags: review?(iann_bugzilla)
Comment on attachment 678151 [details] [diff] [review]
patch v2

>+    <hbox align="end">
>+      <hbox align="center" id="" 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.
Attachment #678151 - Flags: review?(iann_bugzilla) → review+
Attachment #678151 - Attachment is obsolete: true
Attachment #678154 - Flags: review+
Keywords: checkin-needed
Comment on attachment 678154 [details] [diff] [review]
patch v3 [Checked in: Comment 12]
Attachment #678154 - Attachment description: patch v3 → patch v3 [Checked in: Comment 12]
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.