unify advanced server settings into a tabbed dialog

RESOLVED FIXED in mozilla1.5beta

Status

SeaMonkey
MailNews: Account Configuration
--
minor
RESOLVED FIXED
17 years ago
13 years ago

People

(Reporter: Phil Miller, Assigned: Ian Neal)

Tracking

Trunk
mozilla1.5beta
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ue], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

17 years ago
In the account manager, when a news account is selected, clicking the
"Advanced..." button opens a single-option window for setting "When sending
messages from this identity, always use the following server (SMTP)". 
For news accounts, no SMTP server is used, and so this dialog should be
eliminated. For mail accounts, this should be placed under the "Server"
sub-item, along with settings for the POP3/IMAP inbound server.

Comment 1

17 years ago
I agree that the news panels shouldn't have a reference to an SMTP Server.

Add jglick to the Cc: list for her input for the mail accounts. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

17 years ago
Not only shouldn't there be reference to SMTP server in a news account, but the "Advanced Settings" dialog is unnecessary, because it only contains a single option which can be placed directly in the server page for the mail accounts.

Also, is this Windows or PC specific? I doubt it, since the interface is supposed to contain the same things on all platforms, and this ispurely an interface issue.

Comment 3

17 years ago
>For news accounts, no SMTP server is used, and so this dialog should be
>eliminated.

If that is true, I agree it shouldn't be available for news accounts.

Comment 4

17 years ago
*** Bug 110871 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
(Reporter)

Comment 5

17 years ago
Changed Platform/OS to All/All because the interface is supposed to be uniform on all systems, so 
this problem would exist on all systems.
OS: Windows 98 → All
Hardware: PC → All

Comment 6

17 years ago
Bhuvan, is it ok that both of these requests are in this bug?
1. Move the contents of the Advanced button from the account level to the Server
panel.
2. Remove the Advanced button from the News account level since news doesn't use
SMTP.

Comment 7

17 years ago
Yeah. It's fine. The code to fix both these bugs is in the same place.

Comment 8

17 years ago
*** Bug 114224 has been marked as a duplicate of this bug. ***
Summary: Remove "Advanced Account Settings" In Account Manager → Remove "Advanced Account Settings" In Account Manager for news

Comment 9

16 years ago
Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?

Comment 10

16 years ago
1. Remove "Advanced" button from top level Account Settings pane for all accounts.

2. Add "Advanced" button to Server Settings pane for POP
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerPop1P.gif
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerPopAdv1.gif

3. IMAP Server Settings pane already has an Advanced button. Change the dialog
that opens to include both IMAP and SMTP advanced settings (tab dialog).
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImap1P.gif
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImapAdv1.gif

4. If smtp not needed for newsgroup accounts, remove advanced button from
Account Settings pane and don't add to Server Settings pane.
Keywords: nsbeta1
Whiteboard: [ue]

Comment 11

16 years ago
Mail triage team: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [ue] → [adt2][ue]
re-assigning nsbeta1+ bugs
Assignee: racham → sspitzer
Status: ASSIGNED → NEW
> Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?

yes, mrmazda@ij.net is right, that's a news identity has a smtp key for that reason.

that means we need the advanced button for news.

I'll keep this bug open for the tab dialog approach (in
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImapAdv1.gif)

clearing nsbeta1+, as this isn't about removing the button for news accounts.

futuring, as I don't have the cycles to move around the advanced settings UI and
I don't think it's that high of a priority.

jen / ninoschka, please correct me if I'm wrong.
Keywords: nsbeta1+ → nsbeta1
Summary: Remove "Advanced Account Settings" In Account Manager for news → unify advanced server settings into a tabbed dialog
Target Milestone: mozilla1.0.1 → Future
removing adt and nsbeta1+, since this bug has morphed and I don't think it is as
important anymore
Keywords: nsbeta1
Whiteboard: [adt2][ue] → [ue]
(Assignee)

Updated

15 years ago
Blocks: 62429
(Assignee)

Comment 15

15 years ago
Taking
Assignee: sspitzer → bugzilla
(Assignee)

Comment 16

15 years ago
Accepting
Status: NEW → ASSIGNED
(Assignee)

Comment 17

15 years ago
Created attachment 129196 [details] [diff] [review]
Initial Patch v0.1

Moves advanced button from main/identity pane to server settings pane and
combines it with the IMAP advanced button for IMAP servers as a tabbed dialog
window.
am-identity-advanced.xul/js/dtd and am-imap-advanced.xul/js/dtd are replaced by
the new am-server-advanced.xul/js/dtd
Not 100% sure how to generate a cvs diff to add new files so this patch may
have to be applied manually in parts.
(Assignee)

Comment 18

15 years ago
Created attachment 129197 [details] [diff] [review]
Patch v0.1a taken against cleaner tree

As v0.1 except without fixes for another bug in
Attachment #129196 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #129197 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 19

15 years ago
Targetting
Target Milestone: Future → mozilla1.5beta

Comment 20

15 years ago
Comment on attachment 129197 [details] [diff] [review]
Patch v0.1a taken against cleaner tree

>+    <button hidable="true" hidefor="movemail" label="&advancedButton.label;"
Why hide for movemail? You still need SMTP to send, right?
The button is in the wrong place anyway.

>+  var serverSettings = new Array;
The old code is wrong, use = {}; instead.

>Index: content/am-server-advanced.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.xul,v
This should say
RCS file: content/am-server-advanced.xul

>+<?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>
>+
>+<!DOCTYPE page SYSTEM "chrome://messenger/locale/am-server-advanced.dtd">
>+
>+<page
Please convert this to a <dialog> (plus other neccessary changes). Obviously at
one time it was a page of the wizard, but nobody bothered to convert it before
:-(

>+        <menulist datasources="rdf:smtp"
>+                  containment="http://home.netscape.com/NC-rdf#child"
>+                  ref="NC:smtpservers"
>+                  oncommand="onSelected(event)" id="smtpServerList">
>+          <template>
>+            <rule>
>+              <menupopup>
>+                <menuitem uri="..." key="rdf:http://home.netscape.com/NC-rdf#Key"
Sigh. Please change this to value, menulists know about that attribute (see
below for an example).

>Index: content/am-server-advanced.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.js,v
This should say
RCS file: content/am-server-advanced.js

>+  var selectedServerKey = server.smtpServerKey;
>+  if (selectedServerKey)
>+  {
>+    var preselectedItems = serverList.getElementsByAttribute("key", selectedServerKey);
>+
>+    if (preselectedItems && preselectedItems.length > 0)
>+      serverList.selectedItem = preselectedItems[0];
>+  }
>+  else
>+    // no key, so preselect the "useDefault" item.
>+    serverList.selectedItem = document.getElementById("useDefaultItem");
This should simplify to serverList.value = server.smtpServerKey; etc. In fact,
it should be possible to tweak getControls to return either just the smtp or
both sets of controls as appropriate.

>Index: locale/en-US/am-server-advanced.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/locale/en-US/am-server-advanced.dtd,v
This should say:
RCS file: locale/en-US/am-server-advanced.dtd

You don't seem to have edited jar.mn, which actually lists the files to go in
messenger.jar, or shown which files are no longer needed.
Attachment #129197 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 21

15 years ago
OK, I take it back about the button.
BTW diffs of removed files look like added files except with --- for the file
and +++ for /dev/null.
(Assignee)

Comment 22

15 years ago
Created attachment 129234 [details] [diff] [review]
Patch v0.2

Updated patch taking onboard Neil's comments.
Attachment #129197 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #129234 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 23

15 years ago
Created attachment 129251 [details] [diff] [review]
Patch v0.4

Taking on board Neil's comments via IRC - prefered his "if (slot in server)" to
my method in patch v0.2 of creating IMAP entries in array no matter what the
server type was.
(Assignee)

Updated

15 years ago
Attachment #129234 - Attachment is obsolete: true
(Assignee)

Comment 24

15 years ago
Comment on attachment 129234 [details] [diff] [review]
Patch v0.2

Cancelling old review request
Attachment #129234 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

15 years ago
Attachment #129251 - Flags: review?(neil.parkwaycc.co.uk)

Comment 25

15 years ago
Comment on attachment 129251 [details] [diff] [review]
Patch v0.4

I can't get this patch to apply to am-main.xul or am-main.dtd; can you try
again without -w please?
(Assignee)

Updated

15 years ago
Attachment #129251 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 26

15 years ago
Created attachment 129289 [details] [diff] [review]
Patch v0.4a

Still with -w but should apply better.
Attachment #129251 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #129289 - Flags: review?(neil.parkwaycc.co.uk)

Comment 27

15 years ago
Comment on attachment 129251 [details] [diff] [review]
Patch v0.4

Turns out this conflicts with attachment 129132 [details] [diff] [review] :-)

Comment 28

15 years ago
Comment on attachment 129289 [details] [diff] [review]
Patch v0.4a

>+    if (slot in server) {
Nit: { on its own line in this file
>+      var val = server[slot];
Not sure this is worth it as it's only used once.
Attachment #129289 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 29

15 years ago
Created attachment 129387 [details] [diff] [review]
Patch v0.4b

Nits addressed
Attachment #129289 - Attachment is obsolete: true
(Assignee)

Comment 30

15 years ago
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Carrying forward r=
Attachment #129387 - Flags: superreview?(bienvenu)
Attachment #129387 - Flags: review+

Comment 31

15 years ago
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

sr=bienvenu
Attachment #129387 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 32

15 years ago
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Requesting driver approval for checkin to 1.5b
Attachment #129387 - Flags: approval1.5b?
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Approved for 1.5b
Attachment #129387 - Flags: approval1.5b? → approval1.5b+

Comment 34

15 years ago
I can handle checking this in for Ian.

Comment 35

15 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Blocks: 216606
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.