Last Comment Bug 779990 - "Advanced..." button in Server Settings is in the wrong place
: "Advanced..." button in Server Settings is in the wrong place
Status: RESOLVED FIXED
: polish
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-02 14:22 PDT by bintoro
Modified: 2012-11-04 15:34 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch (6.41 KB, patch)
2012-09-18 12:12 PDT, :aceman
iann_bugzilla: review-
mconley: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (5.82 KB, patch)
2012-11-04 11:53 PST, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 [Checked in: Comment 12] (5.99 KB, patch)
2012-11-04 12:26 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description bintoro 2012-08-02 14:22:45 PDT
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 2012-08-02 17:42:24 PDT
(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 2012-08-03 00:34:00 PDT
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) (:☕️) 2012-09-17 11:12:10 PDT
I think either of 2 or 3 should be fine.  I don't really care which one, though.  :)

Thanks,
Blake.
Comment 4 :aceman 2012-09-18 02:09:10 PDT
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 2012-09-18 12:12:19 PDT
Created attachment 662264 [details] [diff] [review]
patch
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-09-24 08:06:15 PDT
Comment on attachment 662264 [details] [diff] [review]
patch

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

Thanks,
Blake.
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-09-26 13:43:02 PDT
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 2012-11-04 10:12:06 PST
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 2012-11-04 11:53:53 PST
Created attachment 678151 [details] [diff] [review]
patch v2
Comment 10 Ian Neal 2012-11-04 12:12:42 PST
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 2012-11-04 12:26:45 PST
Created attachment 678154 [details] [diff] [review]
patch v3 [Checked in: Comment 12]

Note You need to log in before you can comment on or make changes to this bug.