The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bintoro, Assigned: aceman)

Tracking

({polish})

Trunk
Thunderbird 19.0
polish

SeaMonkey Tracking Flags

(seamonkey2.16 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.99 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 2

5 years ago
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
Component: Account Manager → Account Manager
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.  :)

Thanks,
Blake.
(Assignee)

Comment 4

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: polish
(Assignee)

Comment 5

5 years ago
Created attachment 662264 [details] [diff] [review]
patch
Attachment #662264 - Flags: ui-review?(bwinton)
Attachment #662264 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 662264 [details] [diff] [review]
patch

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

Thanks,
Blake.
Attachment #662264 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Updated

5 years ago
Attachment #662264 - Flags: review?(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!
Attachment #662264 - Flags: review?(mconley) → review+

Comment 8

5 years ago
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.
Attachment #662264 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 678151 [details] [diff] [review]
patch v2
Attachment #662264 - Attachment is obsolete: true
Attachment #678151 - Flags: review?(iann_bugzilla)

Comment 10

5 years ago
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.
Attachment #678151 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 678154 [details] [diff] [review]
patch v3 [Checked in: Comment 12]
Attachment #678151 - Attachment is obsolete: true
Attachment #678154 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 12

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

https://hg.mozilla.org/comm-central/rev/1189f87a89df
Attachment #678154 - Attachment description: patch v3 → patch v3 [Checked in: Comment 12]

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-seamonkey2.16: --- → fixed
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.