The current Outgoing Server Settings are awful to me. Only one SMTP Server is direct available, for more you've to click on Advanced (this button alone is badly named, should be More or Further ones). And then another click to add/edit the server.

This is an raw example of what I'd think the UI should look. Comments are welcome.

mass re-assign.

New example up to date with the current UI elements and example of the list content.

Christian, I agree with you. However, workload for UI change is not so low and it will take time. I think changing label of "Advanced" button to "Additional SMTP Server" will easily reduce user's confusion and it is very easy. What do you think about this short term workaround? In addition to SMTP server settings, I think that changing label of "Advanced" button in "Server Settings" of each account will also reduce user's confusion. Changing to "Change SMTP server" can be accepted as short term workaround because current "Advanced" setting panel has "SMTP" setting only.

Changing the SMTP buttons label is a good workaround, yes. Although I'm not that sure, changing the whole pane is so difficult. But I wanted to wait for some response on the redesign before I start to look at the actual code. I don't think "Advanced" -> "Server Settings" is good or helpful. The current button resides in the Server Settings and is named "Advanced". So it's for the Advanced Server Settings - quite clear or not? What confuses users is that the SMTP server for a account can be chosen in the Advanced Server settings of the POP/IMAP/NNTP server. And beware of changing it to "Change SMTP server" - at least in the IMAP case this would be wrong/insufficient.

Ok, here are some suggestions/ideas on the proposed UI: 1. The "Delete" button should be renamed to "Remove" to match similar dialogs in Mozilla. The "Set Default" button should be renamed to "Set as Default" to match the assimilable button of the Account Manager. 2. IMHO the user should be presented with just a list of the available SMTP servers at first. This list should be accompanied with the "Add", "Edit", "Remove" and "Set as Default" buttons. 3. The default SMTP server should be bold in the list so the user can identify it. 4. When "Edit" or "Add" is pressed the user should be presented with a dialog that contains the controls you have in the upper half of the pane. 5. I don't think "Config name" is needed. Just show the username and servername in the list. I would suggest using just one column and showing something like username@password:port. When no username is set, just show the servername. When the port is the default port, don't show it either. 6. "Send as hostname in HELO/EHLO". I don't see why this is needed. Why did you add this setting? 7. What is "Try secure authentication" all about? Is there already backend support for it? 8. I don't think two tabs are needed for the settings, at least when you remove the things I suggested. 9. Accesskeys would be nice. ;-)

Bug 151774 can be a DUPE of this bug.

Why not putting the SMTP Server settings where they should be ? It is just a hacked-together screenshot, but that way we could get around the problems when using miltiple accounts on the _same_ server. Quite some other Mozilla bugs "depend" on that problem not being clearly able to specify SMTP server, username and password on an per-account base.

Forgot to mention: Especially use the screenshot as "default" setting, having an outgoing server. but no auth. But if auth is required use the same as incoming. One thing I forgot: A checkbox for "Use SMTP after POP".

Where is the list overview for all SMTP servers specified? We need at least a list in a drop down in each identity panel to select the smtp server. SMTP servers can be shared by multiple identities. So why should each account get a own SMTP panel? And there is no need for an "SMTP after POP" switch. This "mode" simply is SMTP without authentication, so no need for an extra switch.

Since SMTP server is independent from POP/IMAP account and can be shared among POP/IMAP accounts as Christian says, current independent "Outgoing Server(SMTP)" definition and SMTP server choice in Account Settings are proper and sufficient mechanism for Mozilla itself. But, SMTP account and POP/IMAP account are usually provided at same time by ISP. Then SMTP is tied together with POP/IMAP account in user's mind(proper in contract view). So I think path to SMTP server addition/modification from Account Setting is preferable. In addition, current "Advanced" buttons are too difficult to understand for users including me. Who can imagine "Advanced" is the "SMTP server choice" in Account Settings? Who can guess easily "Advanced" is the "SMTP server addition/modification" in "Outgoing Server(SMTP)"? My proposal based on attachment 139820 [details] in Comment #11 From Joachim Otahal. (A) Outgoing SMTP Server Settings (in account settings of an account) (This panel can be under current "Advanced" button) ( if "Advanced" is renamed to "SMTP server choice") ( or appropriate one. ) <SMTP Server Choice> (*1) _ Use Default SMTP Server (Recommended) X Use specific SMTP Server (pull down menu is possible) +--------------------------------+ | _ SMTP.Server-1.net | | _ SMTP.Server-2.net (Defaulted)| [Edit] [Add new SMTP](*2) | X SMTP.Server-3.net | [Delete or Remove] +--------------------------------+ <SMTP Server settings information currently choosed> "Default SMTP server" or "Specific SMTP Server" indicator (*3) Server Name : SMTP.Server-3.net User Name : login-name@abcd.smtp.server-3.net ... Other SMTP related information follows (*1) Moved from current "Advanced" button/Pannel (*2) [Edit][Add new SMTP] is same as current "Advanced" in "Outgoing Server(SMTP)" (*3) This type of information should be fed back to user because "Use default SMTP server" and "Use SMTP Server defaulted" have different meaning in SMTP choice mechanism. (B) "Outgoing Server(SMTP)" <SMTP Server setting information of currently defaulted> (*4) "Default SMTP server" or "Specific SMTP Server" indicator (*3) Server Name : SMTP.Server-3.net User Name : login-name@abcd.smtp.server-3.net ... Other SMTP related information follows <SMTP Server list currently defined> (*5) +--------------------------------+ | _ SMTP.Server-1.net | | _ SMTP.Server-2.net (Defaulted)| [Edit] [Add new SMTP] | X SMTP.Server-3.net | [Delete or Remove] +--------------------------------+ <SMTP use by accounts> _ Reset all account's setting to "Use Deafult SMTP Server" (*6) (*4) Same as current panel (*5) Moved from current "Advanced" button/Pannel (*6) New - easy if Bug 222388 will be fixed

SMTP after POP is needed, about 90% of freemailers offer that as compatibility / alternative for older email programs who don't know SMTP-auth, allowing SMTP connections for that sender address for usally ten minutes from the same IP address if they authenticated themself before using POP3. There are quite some mail servers which only know that way and no other. About 10% of our customers need that. I don't know if this is the right place: The wizard for creating new email accounts should reflect that, offering the possibility to input the SMTP server with a checkbox for using SMTP-auth with the same account data by default. In the German Mozilla 1.6 release I had to scroll in the wizard, a clear sign that the window is too small.

Joachim, what do you think should the "SMTP after POP" switch cause? As I said, SMTP after POP is nothing else than SMTP without auth - so on can say that not checking "use name and password" (whereas I think this term should be changed slightly) is automatically "SMTP after POP". I agree that the wizard should offer the possibility to define an own SMTP server for the account to be created. The one SMTP server for all accounts mentality is IMHO wrong. But that's another bug. WADA, just renaming the "Advanced" button in something else is not that good since this button offers advanced IMAP options in the IMAP panel too. But I think instead using a somehow named button to open another window with some options, just a drop-down, listing the defined labels (or the address if no label defined) would be the best. Moving the SMTP server handling directly into the account seetings is what I really dislike.

Comment #13 From Joachim Otahal : Joachim Otahal, bug for "POP before SMTP" is Bug 81387. For window size of account manager, you can see bugs by next Bugzilla search(22 bugs, 3 open, were displayed when I searched) Summary <contains all of the word/string> : size Product : Mailnews ( plus Thunderbird if you want) Component : Account Manager Status : all status Resolution : FIXED and DUPLICATE and ---

To Comment #14 : > since this button offers advanced IMAP options in the IMAP panel too Sorry, I always forget to support IMAP - "IMAP is a word to be found only in the dictionary of human beings except me." > Moving the SMTP server handling directly into the account seetings is what I really dislike. I agree with you. However, SMPT and POP/IMAP are tied together in user's mind and users tend to think SMTP server and username/password for SMTP are attributes of an account. I think this is one of the biggest reason of comfusion by users in addition to confusing UI. So my main requests are (1) Do not hide SMTP in "Advanced", (2) "One click path to SMTP definition" from Account Settings. SMTP in account definition is still "SMTP choice" only. Following is my concept (focused on SMTP only). (A) <Server Setting> panel in Account Settings Same as current panel [SMTP] [Advanced] (*1 separated SMTP setting choice) (B) <SMTP Choice> panel (*2 separated from current "Advanced Accout Settings" panel) Same as current SMTP tab in "Advanced Accout Settings" panel [SMTP definition] (*3 New one click path to current ) ( "Advanced Outgoing Server(SMTP) Settings" panel) Needless to say, "Advanced Outgoing Server(SMTP) Settings" panel should be renamed and changed because user can reach this panel not only from "Outgoing Server(SMTP)"/"Advanced button" but also from "Account Settings", many chages will be required though. Please note that I do not care on interface for user's choice, button type or list type. I'm focusing on only panel transittion in user's point of view.

Following scenario is typical for the people I know (private use, office use is often worse if I cannot sell them their in-house Linux-mail server): They have 4 to 8 accounts (some more of course), on gmx.de, web.de, yahoo.de, netscape.com, hotmail.com (etc), everyone with their own auth-info, and don't have the luck to use a SMTP-Relay like I can do with my ISP (although it is not free). With that in mind it is really clear why I vote to have the smtp setup to be bound to the email account as close as possible. If it is done different it would make the setup complex than nessecary, I cannot imagine an UI that can handle that very typical situation with more grace than having it bound to the account. If you (then) can choose from a drop-down list within the mailsetup to use one SMTP for 3 email Adresses and the second SMTP for the other adresses (with advanced button?) is IMHO better, that suggestion is a good idea.

Mail/News has a poorly designed SMTP user interface. This type of problem is normally an enhancement, and would thus be of low priority to developers. However, the bad design of the SMTP user interface is causing a number of users to file spurious bugs. It is also resulting in a lot of grief (the magnitude of which is not recognized, because it is dispersed between a number of different bugs). This is a problem with both Mozilla Mail/News and Thunderbird. It remains a problem even in these, more recent versions: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113 Mozilla Thunderbird 0.5 (20040207) Although no single SMTP bug has that many votes, the sum of votes for those bugs partially or wholly dependent upon UI problems is rather large. These bugs aren't really bugs, but are users who are mislead by the UI: bug 228643 - Outgoing Server settings ignore the default bug 170089 - default server value not being used to send mail. Mail cannot be sent. bug 222064 - don't remember the updated default SMTP server. Can't use more then one SMTP server These bugs all mention or are in part due to bad UI design: bug 202468 - 3 votes - Simpler, more consolidated UI for SMTP server settings bug 52384 - 27 votes - difficult to define multiple smtp servers and quickly/automatically switch between them bug 154453 - 2 votes - SMTP setting dialog totally corrupted bug 218518 comment 4 and comment 5 - unable to select alternate SMTP server in Account Settings bug 226017 - Should try other SMTP servers when default doesn't work Note that all of these bugs can't simply be marked closed and consolidated into one bug (most contain more than one issue with the way SMTP servers are entered & organized). However, I suggest that the UI aspects of these bugs be consolidated into bug 202468 . Let's all vote for it, and hopefully get some work done on this UI! Andy

*** Bug 238724 has been marked as a duplicate of this bug. ***

Is there a way how I can hack in on my own ? I have no compiler here, but what ( .xml ?) files are responsible for the mail UI ? The current SMTP Server setup is worse than with Outlook(-Express).

For UI you don't need any compiler. Responsible for the SMTP-UI are am-smtp.xul, smtpEditOverlay.xul, smtpServerEdit.xul and SmtpServerList.xul - all with their .js-files - in mailnews/base/prefs/resources/content/ and smtpEditOverlay.dtd in mailnews/base/prefs/resources/locale/en-US/

.. not only that files, quite some more linked with. This is really tough for someone whose last time coding was at DOS times. Renaming buttons, creating buttons OK, connect the right function to the button is beyond my limit, simply move "smtp server" to every account instead of the global setup: No idea where the tree function does decide what to display in what file.

This is a new approach to improve SMTP server handling. It provides a new mechanism to change the SMTP server for the current account and a new panel for "Outgoing Server (SMTP) Settings". Firstly, the change containes not only UI changes but two new fields, Configname (bug 86370) and machine-identity for EHLO string (bug 68877). This is to see if there's enough room for all the stuff and to get it work that way. Ok, the drop down in the server panel uses the hostname plus the username (if available) or the configname instead if you provided one. More changes have been done in SMTP panel. It puts the server list and the edit fields in the same panel so you don't have to click through three dialogues. To save space general and security options have been separated ("Try secure authentication not working yet"). Note: the "configure" button in the server panel doesn't work yet. It's supposed to switch to the SMTP panel with the right server preselected in future. I'd appreciate help on how to switch to another panel. If that's impossible or unwanted it could also be changed to call a new window with SmtpServerEdit.xul which uses smtpEditOverlay.xul as does am-smtp.xul.

This one shows the right text, spacing and groupbox of the patch.

The screenshots look good! However, I'm not sure they enable one that (for me at least) is very important. Here's my situation: I have one IMAP account. I use this at every location. However, I have two SMTP accounts: work and home. Unfortunately, the office network is configured so that I have to use the work SMTP server while I am there, but I cannot use it when I am at home. So, I need to have these two SMTP servers for just one IMAP account. Therefore, I need to have the client try to use one SMTP server; if it fails, it should then transparently try to use the other server. This functionality is the topic of bug 52384, for which 27 people have so far voted. So it seems pretty important. Sorry I can't provide a patch myself - I really appreciate the work people have done so far with this bug. The new UI is a serious improvement!

I see your problem, Andy. Though I don't like it, I wouldn't do something against it if implemented. But as you wrote, there's already another bug about this issue and it should get fixed there - besides from the fact that that backend issue would complicate this (mainly) frontend bug.

Fair enough, Christian! You're right; it is a separate bug and should clearly remain that way. Your patches are great and a significant improvement over what currently exists - I would vote to include them right now. I do want to suggest one thing, though. If you make further changes to the patch, just keep in mind the additional functionality I suggested (discussed in bug 52384 or in this bug comment 27). Try to make the UI compatable with that functionality (not implementing the functionality, just leaving a way to integrate it with your UI). That way, when the functionality is added, it won't mangle your UI or leave us back in the situation of having a **** UI. Anyhow, that's just if you're going to make any further changes to your patch. No worries otherwise. It's really easy for me to make suggestions, when you're the one doing the work! I do appreciate the time and effort you've put into this: thank you!

Andy, although I don't like to have multiple SMTP servers per account (resp. identity what it really is), I already thought about it. But it's quite hard. We'd need a UI that allows to show all available servers and multiple selections at one time together with the possibility to reorder the servers. What UI element can do this? Maybe a listbox like I proposed for bug 44863 (see http://bugzilla.mozilla.org/attachment.cgi?id=144403&action=view), and additional "up" and "down" buttons to change the order (priority) of the servers (like it the Navigator/Languages panel)? It would occupy much space - a persistent problem for all panels. At least when getting a UI for multiple identities (bug 44863), the SMTP server chooser has to move, but I can't say how much space we'll have there.

When will this enter a nightly (windows-)build ? I would love to test.. Apart from that: What "bug" is it that there are only thos stupid win32-installer nightly builds, why no more nightly .zip ??

> When will this enter a nightly (windows-)build ? When it has a review. As I wrote I still need to get the "configure" button in the server panel working. Then I'll ask for review. I've an TB build with this UI on my server, but since I can't build for Win, it's Linux only. If anyone wants to test: http://www.eyrich-net.org/files/thunderbird-i686-pc-linux-gnu_smtppanel.tar.gz > why no more nightly .zip ? I guess you mean mozilla-i586-pc-msvc.zip. The ZIP's for Win have been renamed some time ago.

I think top.selectServer(NC_NS + "PageTitleSMTP") should select the SMTP panel. Also note that bug 235872 could become useful here.

(In reply to comment #32) Bugs like Bug 240338 are still beeing opened repeatedly. Christian, can you release your change without "configure" button? I think your new SMTP server related UI is excellent, and this is effective enough to reduce repeated bugs caused by user confusion, even if "configure" button is not implemented. I believe that, to make up for lack of "configure" button, message text in UI, for example "See Outgoing Server for SMTP definition" or "See Server Settings of Account Settings for SMTP choice", is acceptable, as the first release of fix for this bug. I think "configure" button can be delayed until second release of the fix. Christian, could you compromise?

Oh, the patch made progress in the last weeks and it works well, also the configure button. There are nevertheless still questions to be answered. But I could attach the current interims version.

Up-to-date patch with some bigger changes. First it uses <tree> instead of <listbox> for the SMTP list. Though a little more complicated (e.g. tree lacks some useful high-level methods) and costly it has a better column-handling and especially provides a column-picker. In discussion on the german mozilla newsgroup people voted for having the choice which columns displayed, so configname, servername and username are provided. Another change is the support of the new multiple identity window. And the tabs in the SMTP panel have been removed again. Smaller changes include wording and rearranging of elements and groupboxes. The currently so called "Configname" will also fix bug 158099, *please* make suggestions how it should get called ("Configname", "Description", a.s.o.). Other questions include 1. Should the Apply button get moved into the upper groupbox? That would make it a little more clear but will cost vertical space. 2. The default smtp server is currently marked by "(Default)" behind the server name. It would be nice (and consistent with the default account in the left pane) to mark it as bold. Additionally it would solve the problem that "(Default)" isn't visible if the Server column is disabled. The problem with this idea is that the themes have to be changed. Especially users of alternative themes won't have an idea what the default smtp server is until their theme is changed. For a Linux build (that hopefully works not only on my system) see the url in comment #32.

*** Bug 240338 has been marked as a duplicate of this bug. ***

Well, taking a side-look to other email programs, the only possible "Cuatomname" seems to be either "Account" or "Account name", in German "Konto" "Kontoname". Has anyone something better to offer ? (praying to get this UI... at least with 1.8)

Right, it's an account, so this would be right. But I hesitate taking this cause one could mix it up with the account where POP server and identity is merged. So the very right name would be "SMTP account name" - that's a monster.

(In reply to comment #36) > The currently so called "Configname" will also fix bug 158099, *please* make > suggestions how it should get called ("Configname", "Description", a.s.o.). I like "Description". > Other questions include > 1. Should the Apply button get moved into the upper groupbox? That would make > it a little more clear but will cost vertical space. The "Apply" button is there, because you want to avoid a separate dialog for the Add and Edit operations. It mimics an OK-Button of an independent dialog. I'm not sure all this is really such a good idea and whether it would be better to just use a separate dialog like in all other places in Mozilla. After thinking a little about this, I believe this construction is not simpler, but a little confusing. :-( With a separate dialog the "workflow" of this UI would be clearer plus you get a "Cancel" button which is useful, too. If you want to keep the "Apply" button I vote for moving it to the "Configuration of the selected Server" groupbox, because the button applies the changes made there. > 2. The default smtp server is currently marked by "(Default)" behind the server > name. It would be nice (and consistent with the default account in the left > pane) to mark it as bold. Additionally it would solve the problem that > "(Default)" isn't visible if the Server column is disabled. > The problem with this idea is that the themes have to be changed. Especially > users of alternative themes won't have an idea what the default smtp server is > until their theme is changed. I think this should be bold like the default account and the "(Default)" should be kept for a transition period at least. CCing Scott and Neil, because I think their opinion is more important than mine when it comes to MailNews/TB-UI questions. ;-)

> The "Apply" button is there, because you want to avoid a separate > dialog for the Add and Edit operations. Yes. And don't forget the "view" operation. I.e. if one wants to have a look at the one or another SMTP config. Clicking Edit and opening a new window for that is cumbersome. Or do you want the current panel for viewing and a separate window for Add and Edit? I'll implement what the majority or the reviewer wants. > I think this should be bold like the default account and the "(Default)" > should Of course, that's the other possibility. be kept for a transition period at least.

I go with Description too. Note that the Manage Identities button automatically applies the changes made to the identity panel before opening the identity list. The Set as Default button would also do this. You only need to enable the Edit button for other servers. I assume the default server would get selected in the list by default, which would make it easy to spot at least when the panel is first selected. And which useful high-level methods does tree lack?

> I assume the default server would get selected in the list by > default The default is selected when the SMTP panel is opened. Using the Configure... button the server/configuration choosen in the menulist is selected. > The Set as Default button would also do this. You only need to > enable the Edit button for other servers. Pardon? I don't understand. You mean the Edit button Stefan proposed? > And which useful high-level methods does tree lack? Methods and attributes - e.g. removeItemAt(index) or view.selection.select(item) or selectedItem/selectedItems.

Ok, I now added code to mark the entries of the default configuration in the drop-down list and the list in the SMTP panel bold. I added the style rules to prefPanels.css - if that's not good, please let me know where's a better place. Furthermore "Configname" has been renamed to "Description" and the "Apply" button moved into the "Configuration of the selected Server" groupbox. The current patch, screenshots and binary are available on http://www.eyrich-net.org/mozilla/bug202468.html (page in german but the links should be understandable by anyone).

up-to-date patch

Comment on attachment 151360 [details] [diff] [review] patch v6 Trying to get a comment from a reviewer.

I've looked through all the comments on this bug and haven't found anyone mentioning the following. Perhaps it doesn't belong here, and if someone more familiar with this family of bugs and issues knows a better place for it, let me know. If one has 1 or more smtp servers defined, and then creates a new mail account (pop3/imap), this new account is set, by default, not to have it's smtp server set to "Always use default server" but instead to whatever the default server happens to be. This is a very bad design, since if you don't know that this setting exists (which is easy to miss since it's under a tab labeled "Advanced..." under the accounts "Server Settings" section), and you go and change the default smtp server, in the "Outgoing Server (SMTP)" ... "Advanced ..." window, it doesn't affect anything. Obviously the correct behaviour is when you create a new email account (pop3/imap) that it is by default set to "Always use default server". That is after all why they call it DEFAULT. Any thoughts? Am I wrong? Should this be a seperate bug? It's not so much UI as it is behaviour.

(In reply to comment #50) > Obviously the correct behaviour is when you create a new email account > (pop3/imap) that it is by default set to "Always use default server". Bug 222388 is already opened for the problem. > Bug 222388 : Set initial SMTP server setting to "Always Use Deafult SMTP Server" instead of specific SMTP server

Comment on attachment 151360 [details] [diff] [review] patch v6 Coming along nicely, really, although there are a number of issues... >Index: mailnews/base/prefs/resources/content/am-server-advanced.xul It occurs to me that once you've removed the SMTP functions from this dialog it could be split into separate IMAP and POP dialogs. > <groupbox> >- <caption label="&serverSettings.label;"/> I once let Stefan remove a caption from a groupbox. I now think it's a bad idea... >- <button label="&advancedButton.label;" >- accesskey="&advancedButton.accesskey;" >- oncommand="onAdvanced();" >- wsm_persist="true" id="server.advancedbutton" >- prefstring="mail.server.%serverkey%.advanced.disable"/> >- <label hidden="true" wsm_persist="true" id="identity.smtpServerKey"/> >+ <vbox hidefor="nntp,movemail"> >+ <button label="&advancedButton.label;" >+ accesskey="&advancedButton.accesskey;" >+ oncommand="onAdvanced();" >+ wsm_persist="true" id="server.advancedbutton" >+ prefstring="mail.server.%serverkey%.advanced.disable"/> >+ </vbox> Why the extra vbox? hidefor= should work on the button itself, no? >-<!DOCTYPE page SYSTEM "chrome://messenger/locale/am-advanced.dtd" > >+<!DOCTYPE dialog SYSTEM "chrome://messenger/locale/am-advanced.dtd"> No, this one's still a page :-P > > <page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >- onload="onLoad();"> >+ onload="parent.onPanelLoaded('am-smtp.xul'); onLoad();"> > <script type="application/x-javascript" src="amUtils.js"/> > <script type="application/x-javascript" src="am-smtp.js"/> >+ <script type="application/x-javascript" src="SmtpServerList.js"/> You could move these to the overlay... >+var serverList; // the root <listbox> node It looks like it's a tree to me :-P >+ initializeDialog(smtpServer); >+} > >+function initializeDialog(smtpServer) Hardly seems worth it ;-) >+ // surprisingly that's faster than serverList.getElementsByAttribute("key", smtpServer.key); >+ var entryForServer = document.getElementById("smtpServer." + smtpServer.key); Not very surprising, actually, as ids (and refs, for some reason) are hashed. >+ var i = serverList.contentView.getIndexOfItem(entryForServer); >+ serverList.treeBoxObject.ensureRowIsVisible(i); Now I see a problem here, the tree is always scrolled up as far as it can go. I think this is because unless you force it to the dialog's size doesn't get computed until after onload. >+ <vbox id="smtpServerEditor" flex="100%"> flex is actually an integer, what happens here is that the % is ignored and 100 used. As it happens, only the vbox in the smtp page needs to flex, the one in the dialog does not. >+ <textbox id="smtp.description" flex="1" >+ preftype="string" >+ class="uri-element" This is a BIDI flag, to indicate fields that are always ltr, which this one is not. >+ prefstring="mail.smtpserver.%serverkey%.description"/> >+ <hbox align = "center"> While you're reindenting the code it would be nice if you got rid of those spaces around =s too ;-) >- <hbox align="center"> > <checkbox id="smtp.useUsername" label="&alwaysUseUsername.label;" > accesskey="&alwaysUseUsername.accesskey;" > oncommand="onUseUsername(event.target,true);" > prefattribute="value" > prefstring="mail.smtpserver.%serverkey%.use_username"/> >- </hbox> I think this hbox was added to prevent the checkbox from being stretched? >+ <hbox> >+ <label value="&isSecure.label;"/> >+ </hbox> On the other hand, I don't think we have to worry about stretched labels. >+ <radiogroup id="smtp.trySSL" >+ prefstring="mail.smtpserver.%serverkey%.try_ssl" >+ oncommand="selectProtocol(0);"> >+ <hbox class="indent"> With orient="horizontal" you can put the class="indent" on the radiogroup. >+ <hbox flex="100%"> 100% => 1 again. >+ <tree id="smtpList" onselect="onSelectionChange(event);" flex="1" seltype="single" style="height: 0px;"> >+ <treecols> >+ <treecol id="name" label="&descriptionCol.label;" flex="2" persist="hidden"/> >+ <treecol id="server" label="&serverCol.label;" flex="2" persist="hidden"/> >+ <treecol id="user" label="&userCol.label;" flex="1" hidden="true" persist="hidden"/> >+ </treecols> Hmm... I was expecting splitters. >- <separator class="thin"/> >+ <separator class="thin" /> Watch out that you don't add spaces by mistake ;-) >+ var elements = gSmtpTrySSL.getElementsByAttribute("value", server.trySSL); >+ if (!elements.item(0)) >+ elements = gSmtpTrySSL.getElementsByAttribute("value", "1"); >+ gSmtpTrySSL.selectedItem = elements[0]; A pity gSmtpTrySSL.value = server.trySSL; doesn't work; I think there's a bug filed on it. >+ gSmtpHostname.focus(); This isn't nice. I was trying to arrow through the server list :-( >+ if (gSmtpUseUsername.checked) >+ gSmtpAuthMethod.setAttribute("value", "1"); >+ else >+ gSmtpAuthMethod.setAttribute("value", "0"); You don't seem to use this value, any reason? >+ server.trySSL = gSmtpTrySSL.selectedItem.value; You should be able to shorten this to gSmtpTrySSL.value >+ // not only do we enable the elements when the check box is checked, >+ // but we also make sure that it's not disabled (ie locked) as well. >+ if (!checkbox.disabled) { >+ gSmtpUsername.removeAttribute("disabled"); >+ gSmtpUsernameLabel.removeAttribute("disabled"); >+ } >+ if (dofocus) >+ gSmtpUsername.focus(); Strange that the code tries to focus the username even though it might still be disabled... > if (gSmtpTrySSL.disabled) // see bug 70033 on why this is necessary for radiobuttons >- gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; >+ gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; Ooh, very nasty work here :-( 1) bug 70033 is not relevant here, although in fact it is fixed. 2) gSmtpTrySSL is not disabled until later 3) the function that disables it gets it wrong May well be worth a separate bug. >+<!ENTITY trySecAuth.label "Try secure authentication"> I don't see this used, another patch perhaps? > function onSelectionChange(event) > { >+ try { >+ var server = getSelectedServer(); >+ initSmtpSettings(server); >+ } catch(ex){} > updateButtons(); > } If you made getSelectedServer return null when the selection was empty then you a) wouldn't need to try/catch here b) wouldn't need to call initSmtpSettings(null) after clearing the selection >-function onDelete(event) >+function onRemove(event) Why the Delete -> Remove change? >+ // disable firing event on select >+// serverList.view.selection.selectEventsSuppressed = true; >+ if ("selection" in serverList) // only available when content in the tree >+ serverList.view.selection.clearSelection(); You could put a <treechildren/> in the XUL, that way you could be sure. >+ // reenable firing event on select >+// serverList.view.selection.selectEventsSuppressed = false; These don't help because they still send at least one event... doesn't the selection get updated anyway? >+ // remove the treechildren >+ if (serverList.lastChild.nodeName == "treechildren") >+ serverList.removeChild(serverList.lastChild); Same again. >+ treecell2.setAttribute("defaultSMTPServer", "true"); Surely this is unnecessary? >+function updateCurrentSmtpListItem() I was wondering whether there was some way of modifying this to work with setDefault to avoid rebuilding the whole tree. >+ <menulist wsm_persist="true" id="smtpServerList" flex="1"> >+ <menupopup id="smtpPopup" oncommand="onSMTPcmd();"> >+ <menuitem value="" label="&smtpDefaultServer.label;" id="useDefaultItem"/> >+ <menuseparator/> >+ <!-- list will be inserted here --> >+ </menupopup> >+ </menulist> >+ <button label="&smtpEditButton.label;" oncommand="onSMTPconfig();" >+ wsm_persist="true" id="smtp.configbutton" >+ accesskey="&smtpEditButton.accesskey;"/> >+ <label hidden="true" wsm_persist="true" id="identity.smtpServerKey" >+ pref="true" preftype="string" prefattribute="value" >+ prefstring="mail.identity.%identitykey%.smtpServer"/> I think that if you give the menulist these pref settings then things should just work, instead of having to copy values around. Although, you may have to clear the value before rebuilding the list. >- var account = parent.getCurrentAccount(); >+ account = parent.getCurrentAccount(); This duplicated line could probably just be deleted. >+<!ENTITY smtpDesc.label "Outgoing Server (SMTP) to use when sending messages for this identity:"> I'm not sure that this amout of description is necessary. >+ menuitem.setAttribute("defaultSMTPServer", "true"); We already have global styles for default menuitems - <menuitem default="true"/> >+ // give it some unique id >+ menuitem.id = "smtpServer." + server.key; Are you going to refer to it by id again? >+function onSMTPconfig() >+{ >+ var serverarg = null; >+ if (SmtpServerKey) { >+ var smtpService = Components.classes["@mozilla.org/messengercompose/smtp;1"].getService(Components.interfaces.nsISmtpService); >+ serverarg = smtpService.getServerByKey(SmtpServerKey); >+ } >+ var args = {server: serverarg, >+ result: false}; >+ window.openDialog('chrome://messenger/content/SmtpServerEdit.xul', 'smtp', 'modal,titlebar,chrome', args); >+ >+ if (args.result) >+ onSMTP(); There are two possible problems here, one that I noticed, and one that I thought of: 1. If you change the description, apply, then close, the menulist doesn't update 2. What if you delete the current server? > interface nsISmtpServer : nsISupports { > > attribute string key; // unique identifier >- >+ >+ attribute string description; Notes: 1. In IDL, strings are limited to ASCII. To save you doing too much conversion, try an AUTF8String instead. 2. Someone might want you to change the iid. 3. You've got trailing spaces :-P >+ nsCAutoString pref; Note: try to declare variables as near to their use as possible, to avoid unnecessary construction. >+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); Hmm... this is so obsolete, I think someone else is working on fixing it though... >+ if ("onDiscard" in top.frames["contentFrame"]) >+ top.frames["contentFrame"].onDiscard(); You should be able to use window.contentFrame here. >+treechildren::-moz-tree-cell-text(defaultSMTPServer) { >+ font-weight: bold; >+} Ask Jan if he'd like a global "default" style for trees.

(In reply to comment #52) Thanks for review this now, Neil. Items I don't reply to have been changed according to your comment. A patch with these silent changes and most of changes mentioned below can be found at http://www.eyrich-net.org/files/202468_7_u.diff [am-server-advanced.xul] > It occurs to me that once you've removed the SMTP functions from this > dialog it could be split into separate IMAP and POP dialogs. While the patch is quite big I tried not to change something not directly connected to SMTP. So splitting the dialogue I see as a separate bug. [am-server.xul] > I once let Stefan remove a caption from a groupbox. I now think it's a > bad idea... Maybe we should remove the groupbox at all. Adding a caption saying "Server Settings" looks quite useless to me. The whole panel consists of server settings and it's also the panels dialogheader. > Why the extra vbox? hidefor= should work on the button itself, no? I also thought it would but I learned it doesn't. [am-smtp.js] >>+ var i = serverList.contentView.getIndexOfItem(entryForServer); [smtpEditOverlay.xul] > Hmm... I was expecting splitters. would +<splitter resizebefore="closest" resizeafter="closest" class="tree-splitter"/> with persist="hidden width" for treecols be ok? [smtpEditOverlay.js] >>+ gSmtpHostname.focus(); > This isn't nice. I was trying to arrow through the server list :-( Uhm yes, I also ran into this. It was intended for the case when adding an entry but would make it necessary to move it into onAdd() - along with the document.getElementById("smtp.hostname") stuff. :-( >>+ if (gSmtpUseUsername.checked) >>+ gSmtpAuthMethod.setAttribute("value", "1"); >>+ else >>+ gSmtpAuthMethod.setAttribute("value", "0"); > You don't seem to use this value, any reason? This code is used to keep the value in sync with gSmtpUseUsername.checked. The latter is modified by the user, but is set by gSmtpAuthMethod in initSmtpSettings() above every time viewing an entry. >>+ if (dofocus) >>+ gSmtpUsername.focus(); > Strange that the code tries to focus the username even though it might > still be disabled... You mean the case where checkbox is newly checked but disabled? It's unlikely that this will happen. If it's disabled, no oncommand will call the function. And the initail call in initSmtpSettings() doesn't focus. But I could of course move these lines into the if (!checkbox.disabled) case. The question for me is, is the current behaviour wanted at all? Currently all the prefs in UI are locked if the prefIsLocked() return ok for the *defaultSmtpServerKey*, not for the displayed SMTP server. >>- gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; >>+ gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; > Ooh, very nasty work here :-( I must admit I don't understand the code at all. [SmtpServerList.js] > If you made getSelectedServer return null when the selection was empty > then you > a) wouldn't need to try/catch here > b) wouldn't need to call initSmtpSettings(null) after clearing the > selection If I'd do this, in case of rebuilding the list, initSmtpSettings(server) with server=null would be called. Unnecessary and causes blank fields flickering up until the new (old) item is selected. >>-function onDelete(event) >>+function onRemove(event) > Why the Delete -> Remove change? In comment #6, Stefan suggested changing "Delete" to "Remove". To be consistent I also changed all delete in vars or functions to remove. >>+ if ("selection" in serverList) // only available when content in the tree >>+ serverList.view.selection.clearSelection(); > You could put a <treechildren/> in the XUL, that way you could be sure. No, the treechildren is removed in the next line. As I wrote in the comment, this prevends us from looping through all treeitems and deleting them one by one. >>+ // reenable firing event on select >>+// serverList.view.selection.selectEventsSuppressed = false; > These don't help because they still send at least one event... doesn't > theselection get updated anyway? They don't help, yes, I noted this. That's the reason they're disabled. I guess I should remove them. >>+function updateCurrentSmtpListItem() > I was wondering whether there was some way of modifying this to work > with setDefault to avoid rebuilding the whole tree. That would be great, I know. I tried adding this: // if default server if (currentServerKey == gSmtpService.defaultServer.key) { var elements = serverList.getElementsByAttribute("properties", "defaultSMTPServer"); for (var i = 0; i < elements.length; i++) { elements[i].removeAttribute("properties"); } item.childNodes[0].childNodes[0].setAttribute("properties", "defaultSMTPServer"); item.childNodes[0].childNodes[1].setAttribute("properties", "defaultSMTPServer"); item.childNodes[0].childNodes[2].setAttribute("properties", "defaultSMTPServer"); } But it doesn't work - one of the three columns always stays bold. In Venkman I see strange things going on (i doesn't always get incremented) but don't see where the error is. [am-main.xul] > I think that if you give the menulist these pref settings then things > should just work, instead of having to copy values around. Although, > you may have to clear the value before rebuilding the list. I already tried this. Even if I put serverKeyElement = document.getElementById("smtpServerList").value; as first line in onSMTP(), serverKeyElement is always "". [am-main.dtd] >>+<!ENTITY smtpDesc.label "Outgoing Server (SMTP) to use when sending messages for this identity:"> > I'm not sure that this amout of description is necessary. I think all these infos are necessary. But suggestions are appreciated. [am-identity-edit.js] >>+ menuitem.setAttribute("defaultSMTPServer", "true"); > We already have global styles for default menuitems - <menuitem default="true"/> Would be nice to use it. But somehow I don't get it work. menuitem.setAttribute("default", "true"); doesn't make it bold. DOM Inspector shows that no attribute "default" is added. > 1. If you change the description, apply, then close, the menulist > doesn't update Via Account Settings, Configure? I see the menulist getting updated - at least if left via OK. > 2. What if you delete the current server? I just noticed that deleting in the extra Server Configure dialog is broken. In updateServers() replaceWithDefaultSmtpServer() is accessed through window.parent which isn't available in the Configure dialog. Any ideas? But even without this problem, deleting would work but the SmtpServerKey isn't refreshed. I basically could do what is done after coming back from manage identities: add var identity; if (gIdentity) identity = gIdentity; else identity = parent.getCurrentAccount().defaultIdentity; document.getElementById('identity.smtpServerKey').value = identity.smtpServerKey; before calling onSMTP() in onSMTPconfig(). Looks this ok? [nsISmtpServer.idl] > 1. In IDL, strings are limited to ASCII. To save you doing too much > conversion, try an AUTF8String instead. I changed it to attribute AUTF8String description; and use nsSmtpServer::GetDescription(nsACString &aDescription) { nsresult rv; nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); if (NS_FAILED(rv)) return rv; nsCAutoString pref; nsXPIDLCString temp; getPrefString("description", pref); rv = prefs->CopyCharPref(pref.get(), getter_Copies(temp)); if (NS_SUCCEEDED(rv)) aDescription.Assign(temp); return rv; } That doesn't work, rv is always "8000FFFF" although CopyCharPref() returns the right string. If I just return NS_OK everything looks just fine, but there must be a reason for this unexpected error. >>+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); > Hmm... this is so obsolete That's true. I had a patch as part of bug 68877 but this should be a bug on its own. > Ask Jan if he'd like a global Er sorry, Jan who?

(In reply to comment #53) >(In reply to comment #52) >Thanks for review this now, Neil. Yeah... sorry for the delay. >Items I don't reply to have been changed according to your comment. >A patch with these silent changes and most of changes mentioned below can be >found at http://www.eyrich-net.org/files/202468_7_u.diff >So splitting the dialogue I see as a separate bug. Agreed. >Maybe we should remove the groupbox at all. I'll have to see how that looks. >>Why the extra vbox? hidefor= should work on the button itself, no? >I also thought it would but I learned it doesn't. >>Hmm... I was expecting splitters. >would +<splitter resizebefore="closest" resizeafter="closest" >class="tree-splitter"/> <splitter class="tree-splitter"> suffices. >with persist="hidden width" for treecols be ok? Up to you if you want to persist the width. >>>+ if (gSmtpUseUsername.checked) >>>+ gSmtpAuthMethod.setAttribute("value", "1"); >>>+ else >>+ gSmtpAuthMethod.setAttribute("value", "0"); >> You don't seem to use this value, any reason? >This code is used to keep the value in sync with gSmtpUseUsername.checked. >The latter is modified by the user, but is set by gSmtpAuthMethod in >initSmtpSettings() above every time viewing an entry. To me it looks as if initSmtpSettings overwrites it with server.authMethod >>>+ if (dofocus) >>>+ gSmtpUsername.focus(); >> Strange that the code tries to focus the username even though it might >> still be disabled... >You mean the case where checkbox is newly checked but disabled? Not the checkbox, the username! >The question for me is, is the current behaviour wanted at all? Currently all >the prefs in UI are locked if the prefIsLocked() return ok for the >*defaultSmtpServerKey*, not for the displayed SMTP server. Yes, I was wondering about that... I'd forgotton to look that up and nag you. > >>- gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; > >>+ gSmtpTrySSL.disabled = gSmtpTrySSL.disabled; > > Ooh, very nasty work here :-( >I must admit I don't understand the code at all. You'd better not touch it then ;-) >>If you made getSelectedServer return null when the selection was empty >>then you >>a) wouldn't need to try/catch here >>b) wouldn't need to call initSmtpSettings(null) after clearing the >>selection >If I'd do this, in case of rebuilding the list, initSmtpSettings(server) with >server=null would be called. Unnecessary and causes blank fields flickering >up until the new (old) item is selected. That's for selectEventsSuppressed is for :-P >>>-function onDelete(event) >>>+function onRemove(event) >>Why the Delete -> Remove change? >In comment #6, Stefan suggested changing "Delete" to "Remove". Ah, sure. >>You could put a <treechildren/> in the XUL, that way you could be sure. >No, the treechildren is removed in the next line. The idea is that it's easier to remove if you know it's going to be there. > var elements = serverList.getElementsByAttribute("properties", > "defaultSMTPServer"); > for (var i = 0; i < elements.length; i++) { > elements[i].removeAttribute("properties"); > } This is no good, getElementsByAttribute returns a live list. Every time you remove the attribute the list gets rebuilt! >>I think that if you give the menulist these pref settings then things >>should just work, instead of having to copy values around. Although, >>you may have to clear the value before rebuilding the list. >I already tried this. >Even if I put >serverKeyElement = document.getElementById("smtpServerList").value; >as first line in onSMTP(), serverKeyElement is always "". Oh, I think I know what's going on here - the list isn't filled early enough. Never mind. >menuitem.setAttribute("default", "true"); >doesn't make it bold. Well it worked on a random menuitem that I tested it on. >>1. If you change the description, apply, then close, the menulist >>doesn't update >Via Account Settings, Configure? I see the menulist getting updated - at >least if left via OK. But I didn't leave via OK, and the menulist still needed updating. >I just noticed that deleting in the extra Server Configure dialog is broken. >In updateServers() replaceWithDefaultSmtpServer() is accessed through >window.parent which isn't available in the Configure dialog. >Any ideas? Move replaceWithDefaultSmtpServer into the am-smtp.js file? Or does that rely on some other global variables? >But even without this problem, deleting would work but the SmtpServerKey isn't >refreshed. I'd have to see a patch with your suggestion, I can't figure it out standalone. >rv is always "8000FFFF" although CopyCharPref() returns the right string. Strange, as you didn't even change that line... >>>+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); >> Hmm... this is so obsolete >That's true. I had a patch as part of bug 68877 but this should be a bug on >its own. OK. >>Ask Jan if he'd like a global >Er sorry, Jan who? Jan Varga <varga@mozilla-europe.org>

(In reply to comment #54) > <splitter class="tree-splitter"> suffices. Ah ok, I didn't try without the resizes. >> with persist="hidden width" for treecols be ok? > Up to you if you want to persist the width. Yes, as user I'd expect the width to persist I've chosen. It's like this anywhere else, no? > To me it looks as if initSmtpSettings overwrites it with > server.authMethod Yes, and indeed it works without this code too. Strange, I thought I tried removing this in the past. >>>Strange that the code tries to focus the username even though it >>>might still be disabled... >> You mean the case where checkbox is newly checked but disabled? > Not the checkbox, the username! Yes of course the problem would be when the username is disabled but focused. But IMO this problem can only happen if the checkbox is newley checked but disabled. In any other case the username would be enabled before it's gonna get focused. > Yes, I was wondering about that... I'd forgotton to look that up and > nag you. And what's your oppinion now? What's the reason for this at all? I searched for how to lock the prefs but didn't found a situation when this will happen. > You'd better not touch it then ;-) I will not - except removing those two spaces. >>>If you made getSelectedServer return null when the selection was empty >>>then you >>>a) wouldn't need to try/catch here >>>b) wouldn't need to call initSmtpSettings(null) after clearing the >>>selection >> If I'd do this, in case of rebuilding the list, initSmtpSettings(server) with >> server=null would be called. Unnecessary and causes blank fields flickering >> up until the new (old) item is selected. > That's for selectEventsSuppressed is for :-P I guess I don't get it. I should add if (serverList.currentIndex <= 0) return null; to getSelectedServer() and remove initSmtpSettings(null); from onAdd(event), yes? But all that happens is that in response to the clearSelection(), onSelectionChange() gets called while currentIndex is still the old (not -1) and so initSmtpSettings() is called with an server != null. So that's no replacement to the initSmtpSettings(null). > This is no good, getElementsByAttribute returns a live list. > Every time you remove the attribute the list gets rebuilt! Aha, I feared it's something like that. Is this true for all getElementsBy methods? But ok, what about not extending updateCurrentSmtpListItem() but change onSetDefault() to this: { if (!serverList.view.selection.count) return; var currentServerKey = serverKeyElement.value; if (currentServerKey == "") return; var element = document.getElementById("smtpServer." + gSmtpService.defaultServer.key); element.childNodes[0].childNodes[0].removeAttribute("properties"); element.childNodes[0].childNodes[1].removeAttribute("properties"); element.childNodes[0].childNodes[2].removeAttribute("properties"); var item = serverList.view.getItemAtIndex(serverList.currentIndex); item.childNodes[0].childNodes[0].setAttribute("properties", "defaultSMTPServer"); item.childNodes[0].childNodes[1].setAttribute("properties", "defaultSMTPServer"); item.childNodes[0].childNodes[2].setAttribute("properties", "defaultSMTPServer"); gSmtpService.defaultServer = getSelectedServer(); updateButtons(); } The " (default)" suffix isn't updated as it wasn't in the previous try but I'd rather remove this at all than put code in to update this. Or do you know of a simpler solution? >> menuitem.setAttribute("default", "true"); >> doesn't make it bold. > Well it worked on a random menuitem that I tested it on. I can't say what's going on but I tried it again and now it works. :-/ > But I didn't leave via OK, and the menulist still needed updating. Well yes. Since changes are applied with Apply and not through leaving with OK, it's possible the items need to get updated though args.result is false. Removing if (args.result) in onSMTPconfig() will do the job. Drawback is that now the menu is rebuilt even if left with Cancel. I could add a flag that's set to true only when something is applied (though that's no assurance something has really changed) and return this flag. Do you think that's worth it? > Move replaceWithDefaultSmtpServer into the am-smtp.js file? > Or does that rely on some other global variables? The second half of the function uses accountArray and getAccountFromServerId() not available in am-smtp.js. But it looks to me this second for isn't necessary (anymore). > I'd have to see a patch with your suggestion, I can't figure it out > standalone. As I wrote, it's available on the named URL (again updated now). > Strange, as you didn't even change that line... No? It now is + rv = prefs->CopyCharPref(pref.get(), getter_Copies(temp)); and was + rv = prefs->CopyCharPref(pref.get(), aDescription); But I can't see what should be wrong with that. And as I wrote, except the rv, it works.

(In reply to comment #55) >(In reply to comment #54) >>>>Strange that the code tries to focus the username even though it >>>>might still be disabled... >>>You mean the case where checkbox is newly checked but disabled? >>Not the checkbox, the username! >Yes of course the problem would be when the username is disabled but focused. >But IMO this problem can only happen if the checkbox is newly checked but >disabled. In any other case the username would be enabled before it's gonna >get focused. >>Yes, I was wondering about that... I'd forgotton to look that up and >>nag you. >And what's your opinion now? >What's the reason for this at all? I searched for how to lock the prefs but >didn't found a situation when this will happen. That pref disabling code is really screwy. Not only is all the disabling is computed based on the default server, but the username can't be locked. FYI, to lock the prefs you'll need an autoconfig.jsc file, you can set config.obscure_value to 0 in all.js to make it easier to create. But feel free to address this in a separate bug. >>You'd better not touch it then ;-) >I will not - except removing those two spaces. I'm against reindenting code that's not related to code you're modifying. >But all that happens is that in response to the clearSelection(), >onSelectionChange() gets called while currentIndex is still the old (not -1) OK, could you try .select(-1) instead of clearSelection()? >>This is no good, getElementsByAttribute returns a live list. >>Every time you remove the attribute the list gets rebuilt! >Aha, I feared it's something like that. >Is this true for all getElementsBy methods? Yes, but actually you can usually work around it by counting backwards. >But ok, what about not extending updateCurrentSmtpListItem() >but change onSetDefault() to this: [snipped] If you're not going to use " (default)" at all, then that code is fine. >I could add a flag that's set to true only when something is applied (though >that's no assurance something has really changed) and return this flag. Do >you think that's worth it? No, I don't see a problem with rebuilding the list after a cancel. >>Move replaceWithDefaultSmtpServer into the am-smtp.js file? >>Or does that rely on some other global variables? >The second half of the function uses accountArray and getAccountFromServerId() >not available in am-smtp.js. But it looks to me this second for isn't >necessary (anymore). Now I look more closely, I think it is. I think you'll have to use a similar trick that the editor dialogs use for GetCurrentEditorElement, start with tmpWindow = window.top and step through tmpWindow = tmpWindow.opener until you find replaceWithDefaultSmtpServer. >>Strange, as you didn't even change that line... >No? It now is >+ rv = prefs->CopyCharPref(pref.get(), getter_Copies(temp)); >and was >+ rv = prefs->CopyCharPref(pref.get(), aDescription); > >But I can't see what should be wrong with that. And as I wrote, except the rv, >it works. Now that I look at it, in the old version, you returned NS_OK anyway. That's not a bad thing, you'll just return an empty missing description. Now here's an idea that might (but probably won't) work. Currently, a SMTP server already has a display name, which is used to generate the menulist in the advanced server dialog. If you make the display name writable (via preferences), but fall back onto the existing code if the preference is blank (or does not exist, which will also leave an XPIDLCString empty), then in theory the rdf:smtp data source should keep your smtp server list (and menulist) up to date. For instance, the Set As Default button in account manager sets the default account; then rdf pokes properties into the tree to update the bold (or whatever) style. No doubt the rdf:smtp data source has no magic :-( In fact it too assumes an ascii description. More bugs, sigh...

I don't think we need a default global style for trees ATM. I'd move it into the global style if it were used in other modules too.

(In reply to comment #56) >>But all that happens is that in response to the clearSelection(), >>onSelectionChange() gets called while currentIndex is still the old (not -1) > OK, could you try .select(-1) instead of clearSelection()? That works in the way that currentIndex really is -1. But in this case serverList.view.selection.count count == 1 with currentIndex == -1. I guess I saw this things wrong. clearSelection() clears the selection but doesn't clear the focus (currentIndex). .select(-1) clears the focus but leaves the selection. If I test for .selection.count == 0 in getSelectedServer() it works with clearSelection(). But nevertheless I'm not sure if this is intended. Is it really possible to have a row focused without having one selected? And is it ok to have a non 0 count if an illegal index is selected? >>But ok, what about not extending updateCurrentSmtpListItem() >>but change onSetDefault() to this: [snipped] > If you're not going to use " (default)" at all, then that code is fine. I'd like to get rid of it. As mentioned earlier, it's only there for users with alternative themes to see which one is default. > I think you'll have to use a similar trick that the editor dialogs use > for GetCurrentEditorElement, start with tmpWindow = window.top and step > through tmpWindow = tmpWindow.opener until you find > replaceWithDefaultSmtpServer. Yes, that works. Isn't nice but works. > Now here's an idea that might (but probably won't) work. > [...] This is meant as an alternative to my patch? So the tree/menulist isn't built by JS code but by a datasource? Would this way of creation usable with modifying the tree by onApply() or onRemove()? Or is a datasource created list only changeable through the datasource?

*** Bug 262911 has been marked as a duplicate of this bug. ***

(From update of http://www.eyrich-net.org/files/202468_7_u.diff) >+ var smtpServer = null; >+ if ("arguments" in window) // only available when called as dialog >+ smtpServer = window.arguments[0].server; Maybe make smtpServer a parameter to onLoad? >+ var elements = gSmtpTrySSL.getElementsByAttribute("value", server.trySSL); >+ if (!elements.item(0)) >+ elements = gSmtpTrySSL.getElementsByAttribute("value", "1"); >+ gSmtpTrySSL.selectedItem = elements[0]; This has bitrotted but I think I managed to fix up my copy.

(In reply to comment #60) > (From update of http://www.eyrich-net.org/files/202468_7_u.diff) > >+ var smtpServer = null; > >+ if ("arguments" in window) // only available when called as dialog > >+ smtpServer = window.arguments[0].server; > Maybe make smtpServer a parameter to onLoad? That would make it a bit simpler, yes. But how ca onload="onLoad();" in SmtpServerEdit.xul know what the argument should be? > This has bitrotted but I think I managed to fix up my copy. Right, I just uploaded the updated patch 8 on my site.

SmtpServerEdit.xul onload="onLoad(window.arguments[0]);" am-smtp.xul onload="onLoad(null);" Also, two other things I noticed: You shouldn't need both (default) and default="true" on a menuitem You don't seem to use the result of the server edit dialog, I suggest just removing all the code that tracks that.

(In reply to comment #62) > SmtpServerEdit.xul onload="onLoad(window.arguments[0]);" > am-smtp.xul onload="onLoad(null);" Huh, didn't realize I can access window in the XUL. Changed. > You shouldn't need both (default) and default="true" on a menuitem Having "(default)" is good to see if the current server chosen is the default in the menulist element. The bold marking isn't shown there. But I removed it now. > You don't seem to use the result of the server edit dialog, > I suggest just removing all the code that tracks that. Right, it's not used anymore. I updated the patch on my site (still v8).

I'm not sure if patches here deal with allowing users to specify an SMTP server when creating an account. If it does (it seems not), bug 170520 should be a dupe of this. If not, I just mention it for an easy reference.

nominating for TB 1.0 (perhaps, some TB specific changes in mail/ need to be added) Christian, is your patch ready for review?

jshin we are long past our feature freeze and are fixing low risk crash and polish bugs for 1.0 right now.

Was our request too ideal? I probably requested too many things. According to my experience in MozilllaZine forums or forums in Japan, many confused users could easily understand current SMTP setting design by next description ; - Multiple SMTP use consists of 2 steps. (1) Define SMTP servers("Outgoing SMTP Server"/Advanced) (2) Choose SMTP server for a account("Server Settings"/Advanced/SMTP Tab") If this very simple description is placed on "Server Settings" panel, UI change can be postponed, I believe. And fortunately, there is a space under "Local Dirrectry" setting... (Description in help is also appriciated.) Chirstian, how about this for Thunderbird 1.0?

Has any change has been made in how to set the default SMTP server? (This may just be hidden in a welter of detail). I'm really hoping this can just become a simple "default server" pulldown on the SMTP servers pane (rather than an entirely separate dialog as in 1.7), perhaps with the addition of a "No default" choice.

Sorry, but per comment 66 this won't make Thunderbird 1.0; I am trying to find time to complete my review but I doube we'll make Mozilla 1.8a5 either :-/

(In reply to comment #67) > According to my experience in MozilllaZine forums or forums in Japan, many > confused users could easily understand current SMTP setting design by next > description ; > - Multiple SMTP use consists of 2 steps. > (1) Define SMTP servers("Outgoing SMTP Server"/Advanced) > (2) Choose SMTP server for a account("Server Settings"/Advanced/SMTP Tab") > If this very simple description is placed on "Server Settings" panel, > UI change can be postponed, I believe. Er sorry, does on your system TB's Server Settings pane looks different? On my machine I can't see any free room (http://www.eyrich-net.org/files/0.9serversettingspane.png).

(In reply to comment #68) > I'm really hoping this can just become a simple "default server" pulldown on > the SMTP servers pane (rather than an entirely separate dialog as in 1.7), > perhaps with the addition of a "No default" choice. I don't think so. "feature freeze" and "fixing low risk crash only" don't read to me as this is possible. Adding just a menulist wouldn't help, building the contents online and saving/retrieving of old/new data for the form would be necessary too. That's definitly more than a polish bug.

I guess given the feature freeze it would be rather high risk. Sorry to bother you guys.

(In reply to comment #70) > Er sorry, does on your system TB's Server Settings pane looks different? Yes, I use 1024*768 screen size, on Win-2K, and there is not-enough but satisfactory room for the three line explanation. ( http://www.ops.dti.ne.jp/~muttley/pub/bug-202468-screen-shot.jpg ) But it seems to be no room if smaller screen size such as 800*600 and so on. Christian, how about explanation in "Help" only for Thunderbird 1.0, if inculding new UI is difficult? If written in HELP, we can say "See Help" to confused users, then the user can easily say "See Help" to other confused users. "HELP" only is far better than no HELP/no new UI.

Sorry for the delay... (From update of http://www.eyrich-net.org/files/202468_8_u.diff) >- <caption label="&serverSettings.label;"/> Is this change relevant to this bug? > if (id == "smtp.trySSL") > { > document.getElementById("smtp.neverSecure").setAttribute("disabled", "true"); > document.getElementById("smtp.sometimesSecure").setAttribute("disabled", "true"); > document.getElementById("smtp.alwaysSecure").setAttribute("disabled", "true"); >+ document.getElementById("smtp.alwaysSmtpS").setAttribute("disabled", "true"); > } > else > element.setAttribute("disabled", "true"); Ideally this entire block would be replaced with element.disabled = true; >+ if (promptService) >+ promptService.alert(window, alertTitle, alertMsg); >+ else >+ window.alert(alertMsg); An alert isn't going to be much use without a prompt service :-P >+ if (gSavedUsername && gSavedUsername != "") Duplication; if (gSavedUsername) tests for both null and "". >+var gRemovedSmtpServers = new Array; My preference is for [] instead of new Array. >+ element.childNodes[0].childNodes[2].removeAttribute("properties"); element.firstChild would look neater here (six cases). >+ var treeitem = createSmtpListItem(server, defaultServer.key == server.key); and + if (gSmtpService.defaultServer.key == server.key) surprised me, because earlier you correctly wrote server == defaultServer + <separator/> I don't think am-identity-edit needs this separator. Apart from these nits I think I can say r=me on this patch.

(In reply to comment #74) > Sorry for the delay... > > (From update of http://www.eyrich-net.org/files/202468_8_u.diff) > [...] Thanks for looking into it again. Because of a dead CPU I'm currently without my developement machine, but I hope to be able to continue in about a week.

(In reply to comment #74) >>- <caption label="&serverSettings.label;"/> >Is this change relevant to this bug? Not functionally. But it's just a small, harmless change. The text "Server Settings" at this place is simply redundant since the whole panel is about Server Settings. > if (id == "smtp.trySSL") > { > document.getElementById("smtp.neverSecure").setAttribute("disabled", "true"); > document.getElementById("smtp.sometimesSecure").setAttribute("disabled", "true"); > document.getElementById("smtp.alwaysSecure").setAttribute("disabled", "true"); >+ document.getElementById("smtp.alwaysSmtpS").setAttribute("disabled", "true"); > } > else > element.setAttribute("disabled", "true"); > Ideally this entire block would be replaced with element.disabled = true; Sorry? Do you mean blabla.disabled = true; in general (ok for me) or replacing all 9 lines by one element.disabled = true; (what wouldn't make sense for me)? >>+ if (promptService) >>+ promptService.alert(window, alertTitle, alertMsg); >>+ else >>+ window.alert(alertMsg); > An alert isn't going to be much use without a prompt service :-P I only moved that piece of code. If you're absolutely sure about that, I'll remove the else case. I also found this construction in at least another Mozilla js file - so if it's an mistake, it's not unique. >>+var gRemovedSmtpServers = new Array; > My preference is for [] instead of new Array. That's just a replacement for "var gDeletedSmtpServers = new Array;". This and gAddedSmtpServers also use "new Array". Is [] just a personal preference or why is it better. + <separator/> > I don't think am-identity-edit needs this separator. Hm, isn't that a matter of taste?

(In reply to comment #76) >(In reply to comment #74) >>>- <caption label="&serverSettings.label;"/> >>Is this change relevant to this bug? >Not functionally. But it's just a small, harmless change. To start with, I'm not a fan of uncaptioned groupboxes. Additionally, the contents of that groupbox depends on the server type, so perhaps it should actually say POP3|IMAP|NNTP settings (or some such). >Do you mean blabla.disabled = true; in general (ok for me) or replacing all >9 lines by one element.disabled = true; (what wouldn't make sense for me)? If you think about it setAttribute("disabled", "true") doesn't make sense for radiogroups because the radios themselves need disabling. However .disabled = true; takes care of that automatically. >I only moved that piece of code. You'll still get CVS blame for it ;-) >if it's an mistake, it's not unique. We still have too many alerts in the UI - people just ignore the debug warning :-( >Is [] just a personal preference or why is it better. I'd have to ask a JS engine expert that, sorry :-/ >>>+ <separator/> >>I don't think am-identity-edit needs this separator. >Hm, isn't that a matter of taste? [Like the uncaptioned groupboxes or the [] array syntax, you mean?] I actually meant it in am-main.xul, but I see now that the local style is to use <separator class="thin"/> so perhaps we can compromise on that?

(In reply to comment #77) > To start with, I'm not a fan of uncaptioned groupboxes. Additionally, the > contents of that groupbox depends on the server type, so perhaps it should > actually say POP3|IMAP|NNTP settings (or some such). In general I don't like uncaptioned ones too. But here it's unnecessary. And since the server type is named above the Server Name repeating it in the caption is unnecessary too. But ok, either we remove the groupbox, or I don't change it at all - I didn't want making it a political issue, only a BTW change. > If you think about it setAttribute("disabled", "true") doesn't make sense for > radiogroups because the radios themselves need disabling. However .disabled = > true; takes care of that automatically. Ah, ok, I see, thanks. Testing this I discovered two problems: 1. There's no code to disable smtp.description and smtp.username. Since all other elements are listed in allPrefElements, I think these both should be too. But I'm not sure since I don't know what this whole onLockPreference() is for at all. 2. Once disabled these elements stay disabled. In the old UI every server had its own window, so the problem didn't arise. But now we'll need to reenable the elements on each settings init. Am I right here? > [Like the uncaptioned groupboxes or the [] array syntax, you mean?] Exactly :) > I actually meant it in am-main.xul, Whose content is nearly the same am-identity-edit's. > but I see now that the local style is to use <separator class="thin"/> so > perhaps we can compromise on that? Ok, changed both.

(In reply to comment #78) >I didn't want making it a political issue, only a BTW change. BTW changes are almost as dangerous ;-) >I don't know what this whole onLockPreference() is for at all. It's for network admins; they can configure an initial SMTP server without letting you change any of the settings. You can test this using autoconfig. >But now we'll need to reenable the elements on each settings init. Good point. You can probably do element.disabled = getPrefIsLocked();

(In reply to comment #79) > It's for network admins; they can configure an initial SMTP server without > letting you change any of the settings. You can test this using autoconfig. Huh, I didn't even know that something like autoconfig exists - pretty cool. > >But now we'll need to reenable the elements on each settings init. > Good point. You can probably do element.disabled = getPrefIsLocked(); Hm, now after reading the code more careful and testing autoconfig, I'm even more confused. This is how it works in *current* Mozilla: If an default SMTP server exists (virtually always) and a property of the default server (hostname, port a.s.o.) is locked, that property is locked (grayed out) for *every* server. What I thought is an error in the new UI is how it already works. But it doesn't make sense to me. Either locking should work for all servers, than we should test the particular servers pref instead the default servers one. Or it should only work for the default server, than we shouldn't grey out other servers fields at all. The code in question was introduced in bug 144563, but I can't see if the current behaviour was intended. What do you think, what do you know?

My guess was that the code was written in a hurry (in order to make the Netscape 7.0 release) and wasn't properly tested on multiple SMTP servers :-(

Latest version, adapted to fit in the tabbed am-identity-edit.xul and with fixed onLockPreference(). This is a -uw patch, a -u patch is available from my site as well as a collection of most important files changed.

Comment on attachment 169990 [details] [diff] [review] patch v9 r=me, but I thought I'd just mention a couple of nits: > // Disables xul elements that have associated preferences locked. > function onLockPreference() > { >+ var finalPrefString = "mail.smtpserver." + serverKeyElement.value + "."; > var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService); >+ var SmtpPrefBranch = prefService.getBranch(finalPrefString); > >+ var prefstrArray = [ >+ {prefstring:"description", element:gSmtpDescription}, >+ {prefstring:"hostname", element:gSmtpHostname}, >+ {prefstring:"port", element:gSmtpPort}, >+ {prefstring:"use_username", element:gSmtpUseUsername}, >+ {prefstring:"username", element:gSmtpUsername}, >+ {prefstring:"try_ssl", element:gSmtpTrySSL} > ]; > >+ // Does the work of disabling an element given the array which >+ // contains xul id/prefstring pairs. >+ for (var i=0; i<prefstrArray.length; i++) >+ prefstrArray[i].element.disabled = SmtpPrefBranch.prefIsLocked(prefstrArray[i].prefstring); > } It hardly seems worth writing a loop for only six elements. But I do prefer the previous style of spaces after { and before }. >+<!ENTITY serverCol.label "Servername"> >+<!ENTITY userCol.label "Username"> These should either not include the "name" or have it as a separate word.

(In reply to comment #83) > >+<!ENTITY serverCol.label "Servername"> > >+<!ENTITY userCol.label "Username"> > These should either not include the "name" or have it as a separate word. I disagree for 'Username' (ie, it should just be 'Username', as is. But in the case of 'Servername', that should read as you suggest.

(In reply to comment #84) > (In reply to comment #83) > > >+<!ENTITY userCol.label "Username"> > > These should either not include the "name" or have it as a separate word. > > I disagree for 'Username' (ie, it should just be 'Username', as is. We already have 'User Name' (and 'Server Name') in our mail account UI. I'd go for 'User Name'.

(In reply to comment #85) > > I disagree for 'Username' (ie, it should just be 'Username', as is. > > We already have 'User Name' (and 'Server Name') in our mail account UI. I'd go > for 'User Name'. Both (without 'Name' and with 'Name' but separated) is fine for me - but only same style for both strings. So I agree with Jungshik.

Someday (outside the scope of this bug) we should decide which it is, since our very own Password Manager uses plain old 'Username'.

Latest version, with some enhancements in building the tree and fixed bugs. It uses 'User Name' and 'Server Name'. This is a -uw patch, a -u patch is again available on my site.

Comment on attachment 170915 [details] [diff] [review] patch v10 Nits: There's an attempt to change a file in mail/ but that part of the patch is malformed. Also, eight lines have acquired trailing spaces.

(In reply to comment #89) > (From update of attachment 170915 [details] [diff] [review] [edit]) > Nits: There's an attempt to change a file in mail/ but that part of the patch > is malformed. I didn't really had that file locally, tried to fake the diff and ... failed. > Also, eight lines have acquired trailing spaces. I've addressed both issues in my patch. Thanks for your review.

*** Bug 273938 has been marked as a duplicate of this bug. ***

Updated version. It includes the changes Neil suggested in comment #89 but mainly make it work after the modifications the patch to bug 226005 made to nsSmtpServer.cpp.

Our mail server uses SMTP authentication. For security reasons, it doesn't accept mail from another account than the one you have authenticated with. That is, authenticating with the username and password of account address1@example.com won't allow sending messages from address2@example.com. To send messages from address2@example.com I have to authenticate with address2 username and password. Having three accounts witht the same server, I need to set up 3 smtp server to the same smtp server in order to give each one the corresponding username and password. First problem is that under the Advanced Account Settings -> SMTP tab, in Thunderbird, after having set all SMTP servers, the drop down list doesn't distinguish them at all. They all have the same name (smtp.example.com:25). Just mentioning this here instead of opening a new bug as per comment 18. I also hope this bug sorts all the other issues mentioned. Good work.

Comment on attachment 171401 [details] [diff] [review] patch v11 > NS_IMETHODIMP >+nsSmtpServer::GetDescription(nsACString &aDescription) >+{ >+ nsresult rv; >+ nsXPIDLCString temp; >+ rv = mPrefBranch->GetCharPref("description", getter_Copies(temp)); >+ if (NS_SUCCEEDED(rv)) >+ aDescription.Assign(temp); >+ return NS_OK; >+} I'm afraid that I didn't spot this error before, but a successful getter (e.g. one that always returns NS_OK) should always assign to its out parameter. In this case, simply perform the assignment whether or not the pref exists thus making rv unnecessary.

(In reply to comment #94) > a successful getter (e.g. one that always returns NS_OK) should always assign > to its out parameter. But that's only a convention, yes? I can't see a functional difference between leaving aDescription empty or assigning an empty temp. Hm ok, if the function parameter isn't empty, it would make a difference. I don't think that will happen, but I updated the patch.

(In reply to comment #95) > (In reply to comment #94) > > a successful getter (e.g. one that always returns NS_OK) should always assign > > to its out parameter. > > But that's only a convention, yes? I can't see a functional difference between I think Neil meant you should have |return rv| in place of |return NS_OK|

Returning an empty string when the pref does not exist is fine with me although I notice some of the other routines return null (explicitly, of course).

(In reply to comment #97) > some of the other routines return null (explicitly, of course). No problem with char ** but not with nsACString &. So it's now mPrefBranch->GetCharPref("description", getter_Copies(temp)); aDescription.Assign(temp);

Just another update to the current state, only change was to GetDescription() according to comment #94. Carrying forward Neil's rv.

Comment on attachment 172008 [details] [diff] [review] patch v12 Scott does want to review this, since he has some concerns...

*** Bug 279284 has been marked as a duplicate of this bug. ***

*** Bug 202308 has been marked as a duplicate of this bug. ***

*** Bug 147012 has been marked as a duplicate of this bug. ***

*** Bug 151774 has been marked as a duplicate of this bug. ***

Requesting to block Mozilla 1.8b. This bug should definitely go into 1.8 (and thus comming beta) and since only superreview is pending it should be quite easy to get it in soon.

I agree this SHOULD make into v1.8. Quite some email providers implement some kind of SPDIF or else. I already cannot email to some peoble using my ISP's non-free relay server using all of my accounts. Gmx (as example) refuses to accept gmx.net as sender from my ISP's relay server, I have some kind of the same problems with some hotmail contacts, and some verizion people. If this cannot go into 1.8 I am forced to use something else, probably M$ Office Outlook (non-express), but I am very unwilling to leave Mozilla Mail and I am already accepting that I cannot contact some contacts from my adressbook the normal way.

(In reply to comment #106) > If this cannot go into 1.8 I am forced to use something else, probably M$ Office > Outlook (non-express), but I am very unwilling to leave Mozilla Mail and I am You don't have to give up Mozilla mail. Even if this patch doesn't make it in 1.8, you can still specify different SMTP servers for different accounts. This bug is NOT about adding that feature (which has been available for a long long time) BUT about making it more convenient to do that. How to do that? 1. go to Mail & News account settings 2. Choose Outgoing server(SMTP) 3. Click on 'Advanced' and you'll get a pop-up window in which you can add as many SMTP servers as you like 4. In each of your account for which you want to specify an SMTP server other than the default, select 'Server setting' and click on 'Advanced' button. 5. In SMTP tab (well, that's the only tab for mail account), pick an SMTP server to use for that account. Btw, this is not to say I don't want to see this in 1.8. I do want this in.

> 1. go to Mail & News account settings > 2. Choose Outgoing server(SMTP) Thanks, but completely useless. I have three accounts on gmx.net, all of them in use for their different purpose (and some other somewhere else): NORMALLY each of them require their own username and password to send. How can I select between three different mail.gmx.net accounts if there is no way to see which one I am choosing ? They all have the same name. Even with about:config this problem is not easy to solve, hackin' round the id's without knowing if the next mozilla version destroys that config, what already happened a few times. So I am stuck with 2 Euro per month extra pay for my ISP's relay server, a workaround which starts to fail too. So, I am back, I may have to give up Mozilla Mail.

> How can I select between three different mail.gmx.net accounts if there is no > way to see which one I am choosing ? They all have the same name. Show SMTP Username Extension: http://www.chuonthis.com/extensions/ssun.php

(In reply to comment #109) > Show SMTP Username Extension: > http://www.chuonthis.com/extensions/ssun.php Works. Thank you. Lots.

hey, what about Thunderbird ????

I took some screen shots of the latest version of this patch. Some UI comments coming next.

Christian, thanks for getting this SMTP UI improvement patch going. Some comments mostly at the UI level and not at the code level. In general, having the edit controls for the tree on the same page as the tree is cumbersome from a UE point of view. I found it very hard to set up my SMTP servers with this UI. And if you accidentally click the Add button we won't let you leave that panel until you actually finish creating the server. Its very hard to cancel out of that without canceling out of the account manager window itself. I also thought the huge tree box at the bottom of the SMTP servers panel was visually unattractive. Its too tall and it half its width is for a field for description values which for all users up to this point in time is going to be blank anyway. I think we can start with this patch and turn it into something a little more creative that's going to be just as flexible, while improving the look and feel. We should be doing something like the following: SMTP Servers dialog: 1) A listbox at the top with a height of 5 to 6 rows. Hide the column header box and make the list cell value be the server name followed by a hyphen followed by the description. i.e. mail.mozilla.org - My Work SMTP Server. Having a column that is going to be completely empty for most of us because we've never filled in a description field looks weird in the current proposal. 2) Use a styled box below the tree that shows properties of the currently selected SMTP server in a read only mode. Look at the RSS dialog for an example (https://bugzilla.mozilla.org/attachment.cgi?id=173596) as to how to set up this box. It should have the following fields: Description: My Work Server Server Name: mail.mozilla.org User Name: <none specified> Use Secure Connection: Never 3)To the right of the listbox, have the following buttons: Edit, Add, Remove, Make Default 4) double clicking a list item should open the edit dialog 5) Bring back the SMTP Server Edit dialog and use that as the UI for editing an SMTP server. Update the listbox after the dialog is dismissed. Account Settings Top Level Panel: 1) The combo box here looks fine. I don't think we should bold the default server though. Also, if there is a description field we should use it instead of the user name in the combo box. If there is no description field then fall back to the user name (if it is there). 2) Lose the configure button to the right of SMTP server combo box, we already have UI in the very same window already (Outgoing SMTP Server panel) for configuring SMTP servers and shouldn't be duplicating access points. I'd be happy to help work with you on these changes if you are interested.

The screenshots look good. But is the "account settings" over 700 rows high by default? It should be possible to have it at ~580 rows so it fits on 800x600, I would like to ask for 460 rows to fit on 640x480, but AFAIK scrollbars appear when the menu doesn't fit at all - so that "outdated" resolution isn't that important. Scrollbars shouldn't appear on 800x600. (personal work resolution is 1360x1020, I rather think of quite some people I know who prefer 800x600 so everything isn't so small)

(In reply to comment #114) >is the "account settings" over 700 rows high by default? Sadly so. This is due to creeping featurism for POP3 servers. You have: Use secure connection: (o) Never (o) If Available (o) Always (o) SSL [X] Use secure authentication [X} Check for new messages at startup [X] Check for new messages every [___] minutes [X] Automatically download new messages [X] Fetch headers only [X] Leave mail on server [X] For at most [___] days [X] Until I delete or move them from Inbox [X] Empty Trash on Exit It has to be that high to get them all to fit :-(

(In reply to comment #115) > (In reply to comment #114) > >is the "account settings" over 700 rows high by default? > Sadly so. This is due to creeping featurism for POP3 servers. You have: > Use secure connection: (o) Never (o) If Available (o) Always (o) SSL > [X] Use secure authentication This is the POP3/IMAP Window you mention here. That one is 500 to 510 rows (depending on OS and Desktop theme), and fits very well on 800x600, even on 640x480 it isn't too bad. The SMTP Account Settings screenshot doesn't have all the options you mention, there is space to compress. Just make the listbox showing all configured servers smaller so the window matches the size of the POP3/IMAP account settings window. You could move the "apply" button up one notch, next to the SSL choices, but that might look strange and not everyone will agree. Apart from that: Why the "apply" button anyway? I can't see one in the 1.7.5 Mozilla. I agree with comment #113, the listbox showing all servers should start with the server, then username and then SMTP nick (if used).

(In reply to comment #116) >This is the POP3/IMAP Window you mention here. There is only one window. The panel changes depending on the type of account.

'Use name and password' is not clear enough. For a start, it should be 'username', not 'name'. Also, how do you tell each individual account which SMTP server to use? Can we have a screenshot of that?

I've been modifying Christian's patch to start demonstrating some of the points I was trying to make in my UI suggestions.

I've got a patch which implements the design I proposed above based on Christian's work. Here is a screen shot showing: 1) The new SMTP Account Manager panel 2) The new SMTP Properties dialog window 3) The SMTP server combo box drop down for the identity panel 4) The new Advanced server properties dialog (I removed all the tabs). Patch coming up.

(In reply to comment #120) > 2) The new SMTP Properties dialog window Is it intended that in the properties dialog, the user name is hidden in a password field, while (correctly) being displayed in plain text in the SMTP Settings panel?

Comment on attachment 174725 [details] [diff] [review] proposed fix + if (this.mServerList.selectedItems.length <= 0) + return; + this.openServerEditor(this.getSelectedServer()); you can switch the sense of the <= and get rid of the early return... you're sure that hiding the panel instead of just the tab really works and doesn't hide the subsequent panels? I'll take your word for it :-) it seemed like a bug that it didn't work for me earlier.

From the latest screenshots, how does someone go about changing the password? And while we are on this, on the last screenshot, under the SMTP Server dialog box, after Port: 25, the text "Default: 25" is a bit above the baseline of the preceeding text. Might want to move it down a bit.

(In reply to comment #124) > you're sure that hiding the panel instead of just the tab really works and > doesn't hide the subsequent panels? I'll take your word for it :-) it seemed > like a bug that it didn't work for me earlier. I'm sure because the panels have all been removed and it's just a box for IMAP and a box for POP3 so we are showing/hiding one box at a time. That being said, I should be using a deck now instead of just the two boxes, letting the deck do the work. I'll check this patch in with a deck.

I left this out of the original patch by mistake

re-assigning to myself as I drive this into the tree pending Neil's review comments of course. Thanks for getting this discussion and patch started Christian.

Comment on attachment 174916 [details] [diff] [review] mailnews\compose portion of the fix Hmm... when I reviewed the previous patch I should have got the author to fix up the bad code style (else after return; ignoring rv from ClearUserPref) :-[

Comment on attachment 174750 [details] [diff] [review] cvs diff -u (no -uw) version of the patch >- window.openDialog('am-identity-edit.xul', 'identity-edit', 'modal,titlebar,chrome', args); >+ window.openDialog('am-identity-edit.xul', 'identityEdit', 'modal,titlebar,chrome', args); The window name should probably be left blank for modal windows, to stop you from replacing the contents from another window. >+ document.getElementById('identity.smtpServerKey').value = identity.smtpServerKey; > } > > setupSignatureItems(); >+ loadSMTPServerList(); If you fill the smtp servers earlier (the main panel would have to use onPreInit) you could set and get the menulist value directly instead of having to cache it on the key element. This would make onSMTPServerChosen unnecessary. >+ var menuitem = createSmtpMenuItem(server, defaultServer.key == server.key); >+ menupopup.appendChild(menuitem); It would be nicest to get RDF to build the menulist. Failing that, it would still be quite nice to use the menulist.AppendItem method. Using the menulist's built in description support is another consideration. >Index: mailnews/base/prefs/resources/content/am-server-advanced.xul I can't wait for this to be split into separate windows! >- <button label="&advancedButton.label;" >- accesskey="&advancedButton.accesskey;" >- oncommand="onAdvanced();" >- wsm_persist="true" id="server.advancedbutton" >- prefstring="mail.server.%serverkey%.advanced.disable"/> >- <label hidden="true" wsm_persist="true" id="identity.smtpServerKey"/> >+ <vbox hidefor="nntp,movemail"> Can't you put the hidefor on the button? >Index: mailnews/base/prefs/resources/content/am-smtp.js Because you completely rewrote the style of this file I'll have to review it separately, but from a first glance I think you're not handling smtp server deletions correctly. >+ <hbox id="smtpServerInfoBox"> class="inset" makes sense here, as that already sets up standard border and margins. You could probably set the class on the stack (or grid, if you give up the opacity!) avoiding an otherwise useless box. >+<!ENTITY smtpListAdd.label "Add..."> >+<!ENTITY smtpListAdd.accesskey "A"> >+<!ENTITY smtpListDelete.label "Delete"> >+<!ENTITY smtpListDelete.accesskey "D"> >+<!ENTITY smtpListSetDefault.label "Set Default"> >+<!ENTITY smtpListSetDefault.accesskey "f"> These conflict with the main account manager window (tricky for Add, I know!). >+<!ENTITY smtpName.label "Outgoing Server (SMTP):"> >+<!ENTITY smtpName.accesskey "S"> Whoops, just noticed that this conflicts with reply-to address :-[ These access keys are harder than they look! >+#backgroundBox { >+ background-color: #FFFFFF; >+ opacity: 0.5; >+} This is not ideal for Classic, as all colours should be OS colours, or for Modern, where you should stick to its palette. In the latter case you already know the default colour, so you can figure out the fade colour yourself rather than getting Mozilla to composite it. Possibly you should just use a listbox instead? >+#smtpServerInfoBox { >+ border: 1px solid ThreeDShadow; >+ -moz-border-radius: 0px; >+ margin: 4px; >+ padding: 0px; >+} class="inset" on the box should take care of all this in a portable way.

1) removed window name on identity edit modal dialog 2) identity's smtp menu list no longer uses a hidden label field for the serverkey, everything happens directly on the menu list item now (great suggestion) 3) when buildling the smtp menu list, we now use appendItem. I didn't like the placement of the descrition text when I use the description field so I left that blank. 4) moved the hidefor from the box (which I removed completely) to the advanced button. 5) use class="inset" on the server list stack and removed some border CSS rules from accountManage.css 6) fixed broken access keys 7) fixed the bad coding style in nsSmtpServer::SetDescription Additional notes: 1) About deleting SMTP servers. We now prompt for confirmation on delete, and I intentionally removed all of the smtp deleted server caching code (in case of cancel) now that we prompt instead. 2) Just let me know what CSS you want for mozilla suite to replace: +#backgroundBox { + background-color: #FFFFFF; + opacity: 0.5; +} and I'll add it to the patch for classic and modern. I like the look as it is for Thunderbird as it is something we are adding in several places to Firefox and Thunderbird going forward. Thanks for the good comments.

First I'd like to thank you all for working on the SMTP UI improvements - special thanks to Christian. A year ago Christian proposed to integrate the SMTP dropdown into the UI for every separate identity. This was based on the intention to be able to choose different SMTPs for every identity. In the actual screenshots by mscott I can't see the option to choose different SMTPs for the different identities. So why don't you move the SMTP dropdown into the 'server settings' panel (as proposed by my Patch a year ago - Christian knows this patch)? IMHO users are searching for the SMTP dropdown in the server settings - not in the identity panel. Regards Alex Ihrig Mozilla Thunderbird DE

Ooops, maybe I've been sleeping... mscotts Screenshot "of all the new windows" shows the option to choose different SMTPs for the different identities, or not? So the SMTP dropdown is correct in the identities panel. Sorry for this.

Will this patch be applied only to the Mozilla Mail/News component, or ThunderBird as well?

Both. See above, there are Thunderbird-Screenshots.

I wonder how this is gonna 'interact with' bug 222388 and bug 170520.

Comment on attachment 175456 [details] [diff] [review] updated patch based on review comments The mailnews/jar.mn changes went missing this time ;-) > setupSignatureItems(); >+ loadSMTPServerList(identity); This isn't earlier: I meant before the if (identity) so that the line you had in the previous patch that set the value on what was the hidden label would successfully select the correct item in the list. >+ // remove all menuitem children >+ while (smtpPopup.lastChild.nodeName != "menuseparator") >+ smtpPopup.removeChild(smtpPopup.lastChild); I had meant to comment on this last time... I don't think that you ever have any menuitem children to remove, as you're not editing the list from this panel (ch.ey had the option, which is why his code did this). >+ smtpServerList.appendItem(serverName, server.key, ''); You can omit the last parameter, the xbl checks for it. P.S. Did you not like that style for the compose window? Or would you mind reviewing the patch that changes it from the custom widget to the global widget? >+function onPreInit(account, accountValues) >+{ >+ loadSMTPServerList(account.defaultIdentity); >+} Because this is being done in onPreInit I'm hoping that setFormElementValue (whose code works but is outdated) should already select the correct value, so you don't need to repeat yourself. Isn't this value incorrect in the case of switching page to an unsaved change? >+const IPS = Components.interfaces.nsIPromptService; As far as I know most of the code tends to use: const nsIPromptService = Components.interfaces.nsIPromptService; >+var gSmtpServerListWindow = Personally I think this style is stretching a point, making this a singleton object just means lots of extra "this." or "gSmtpServerListWindow." prefexes. >+ mAddButton: null, >+ mEditButton: null, These just exist for completeness? >+ (IPS.BUTTON_TITLE_YES * IPS.BUTTON_POS_0) + (IPS.BUTTON_TITLE_NO * IPS.BUTTON_POS_1), nsIPromptService.STD_YES_NO_BUTTONS although maybe your titles should be "Delete" and "Cancel" (aren't Yes and No considered harmful?) >+ smtpService.deleteSmtpServer(server); Except you don't fix up the accounts that were using the server, you still need to call parent.replaceWithDefaultSmtpServer. >+ document.getElementById('descriptionValue').value = aServer.description ? aServer.description : noneSelected; >+ document.getElementById('portValue').value = aServer.port; >+ document.getElementById('userNameValue').value = aServer.username ? aServer.username : noneSelected; JavaScript has this neat trick borrowed from Perl: instead of writing x ? x : y; like C++ forces you to, just use x || y; >+ var args = {server: aServer, >+ result: false, >+ addSmtpServer: ""}; >+ >+ window.openDialog("chrome://messenger/content/SmtpServerEdit.xul", >+ "smtpEdit", "chrome,titlebar,modal,centerscreen", args); >+ if (args.result) >+ this.refreshServerList(); It would be nice if this could be made to reselect the server just added/edited. Actually it would be nicer still if the list could be constructed in RDF, but that's outside the scope of this bug. >+ <hbox id="backgroundBox" flex="1"/> Since this is empty you can use a spacer. Also, stack children are always stretched to fit so you can drop the flexes too. >+ <grid flex="1"> >+<!ENTITY smtpListDelete.label "Delete"> >+<!ENTITY smtpListDelete.accesskey "l"> I notice for comparison that the account manager use "Remove Account". >+<!ENTITY smtpListSetDefault.label "Set Default"> >+<!ENTITY smtpListSetDefault.accesskey "f"> while I was checking that account manager also uses f for set default. >+ return mPrefBranch->ClearUserPref("description"); A double space crept in here ;-) >+#backgroundBox { >+ background-color: #FFFFFF; >+ opacity: 0.5; >+} >+ >+#smtpServerInfoBox textbox { >+ background-color: transparent; >+} For Classic try background-color: ThreeDLightShadow; for Modern try background-color: #E4EAEF (no opacity in either case).

1) Removed: >+ while (smtpPopup.lastChild.nodeName != "menuseparator") >+ smtpPopup.removeChild(smtpPopup.lastChild); it wasn't needed as you pointed out. I had to juggle a few things in am-identity-edit.js to make it work right with the identity dialog vs. the identity account settings panel. 2) Removed the empty parameter here: >+ smtpServerList.appendItem(serverName, server.key, ''); 3) "Because this is being done in onPreInit I'm hoping that setFormElementValue (whose code works but is outdated) should already select the correct value" Fixed. 4) >+const IPS = Components.interfaces.nsIPromptService; "As far as I know most of the code tends to use: const nsIPromptService = Components.interfaces.nsIPromptService;" Fixed. 5) We've been coming full circle and have started using Yes/No prompts again in the new apps. 6) >+ smtpService.deleteSmtpServer(server); "Except you don't fix up the accounts that were using the server, you still need to call parent.replaceWithDefaultSmtpServer." Great catch! Fixed. 7) "JavaScript has this neat trick borrowed from Perl: instead of writing x ? x :y; like C++ forces you to, just use x || y;" fixed. 8) "It would be nice if this could be made to reselect the server just added/edited. " fixed. 9) >+ <hbox id="backgroundBox" flex="1"/> "Since this is empty you can use a spacer. Also, stack children are always stretched to fit so you can drop the flexes too." fixed 10) fixed the new access key issues 11) fixed the double space in nsSmtpServer.cpp 12) modified classic and modern themes for the suite but I haven't tested that yet as I don't have a mozilla build on this machine tonight. are we ready for landing yet? :)

Comment on attachment 175694 [details] [diff] [review] another updated patch based on the latest review comments am-identity-edit.js: you don't indicate which server is the default server, also you don't check that identity is null (new identity?), although attachment 174750 [details] [diff] [review] did. am-identity-edit.xul: the label's control refers to the old menulist, it needs to be updated, also the prefattribute makes the line rather long, am-mail.xul fits it on the end of the next line (although it too gets the label's control wrong). am-smtp.js: missing ; at the end of the const declaration. Also there's a comment after the code that it's commenting on, which I find unusual. Something went wrong when I was trying to test this patch, I blame the -w, so would you mind addressing my nits and attaching a non-w patch? Thanks.

1) fixed setting the default smtp server for the new identity case (am-addressing.js throws a JS exception for a new identity which causes problems, it did this before this patch, I'm filing a new bug to fix that issue). 2) fixed the broken control values in am-identity-edit.xul and am-main.xul. Also fixed long line issue. 3)adding missing semicolon to am-smtp.js

(In reply to comment #140) >1) fixed setting the default smtp server for the new identity case >(am-addressing.js throws a JS exception for a new identity which causes >problems, it did this before this patch, I'm filing a new bug to fix that >issue). Ah, that was the issue I was running in to before. I like your new version, with all new identities defaulting to the account's primary server. >2) fixed the broken control values in am-identity-edit.xul and am-main.xul. >Also fixed long line issue. Actually it just dawned on me that am-identity-edit.xul doesn't need any of those pref attributes, they can just be removed. Make sure you kill the trailing space you introduced on that line at the same time :-P Speaking of white space, in am-server.js you left a blank line before the }, and in am-smtp.js you added a line solely containing four extraneous spaces. Was your decision to remove the (default) from the menulists delibarate? Finally, I had to fiddle around with the Modern background colour, as my original suggestion clashed with the inset. Eventually I settled on #BBC6D1 r=me with these nits fixed.

this addresses Neil's last round of comments and adds back the default server text to the menu lists which I accidentally removed during one of the earlier reviewsions. Carrying forward Neil's r and david's sr.

fixed!

Scott: Looks like you forgot to check-in the changes to mailnews/base/resources/locale/en-US/messenger.properties

Might have caused bug 284798.

Mscott, I've filed Bug 284926.

*** Bug 247454 has been marked as a duplicate of this bug. ***

There is one thing we could improve: At this moment the "Identity" panel is the main account panel. If an account tree is opened, users don't know that the account name is one of the panels in the tree. IMHO it would be usefull to have a separate "Identity" panel (which is exactly the same as yet) in the tree. A click on the account name should open the tree and switch automatically to the "Identity" panel. Maybe we could name this panel "Identity & SMTP". This would be more clear for users.

(In reply to comment #148) > A click on the account name should open > the tree and switch automatically to the "Identity" panel. That sounds like a good idea, because it makes it easier to describe to the users how to find/open that important panel. I also wonder why the "Outgoung *Server* (SMTP)" in on the "Account Settings" panel and not on the "*Server* Settings" panel, as SMTP is a *server* setting - at least that's where I would expect it to be. :-\ The "Server Settings" panel probably needs to be slighly reorganized to better reflect the groupings: incoming, outgoing, and "general" settings. My rough suggestion (based on an IMAP account): +- Server Settings ---------------------------------------------------+ | | | +- Incoming Server -----------------------------------------------+ | | | Server Type: [ ] | | | | Server Name: [ ] Port [ ] Default xxx | | | | User Name: [ ] | | | | *Security Settings* | | | | Use secure connection: ( ) Never ( ) TLS (o) SSL | | | | [ ] Use secure authentication | | | | *General* <need better term here?> | | | | [x] Check for new mail at startup | | | | [x] Check for new messages every [ 10 ] minutes | | | +-----------------------------------------------------------------+ | | | | +- Outgoing Server (SMTP) ----------------------------------------+ | | | Server Name: [ Use Default Server \/] [ Settings...] | | | +-----------------------------------------------------------------+ | | | | +- General Settings <need better term here? "Other"?> ------------+ | | | When I delete a message [ Mark it as deleted \/] | | | | [x] Clean up ("Expunge") Inbox on Exit | | | | [ ] Empy Trash on Exit | | | | Local directory: | | | | [ c:\bla\bla\bla ] [Browse] | | | +-----------------------------------------------------------------+ | | [ OK ] [ Cancel ] | +---------------------------------------------------------------------+ NOTE: The "Settings..." button by SMTP would take the user to the SMTP settings panel. The "*Security Settings" and "General" might not need those headings. Perhaps a separation via blank row might be better. This arrangement would also make it much easier to explain (e.g., via phone) how & where to set up or modify an account (server settings). If this needs to be a new bug, I'd be glad to file one.

Peter has forgotten, that the SMTP is Identity related - not account related.

*** Bug 291975 has been marked as a duplicate of this bug. ***

*** Bug 293415 has been marked as a duplicate of this bug. ***

*** Bug 262911 has been marked as a duplicate of this bug. ***

*** Bug 306941 has been marked as a duplicate of this bug. ***

*** Bug 311684 has been marked as a duplicate of this bug. ***

*** Bug 294667 has been marked as a duplicate of this bug. ***

*** Bug 119609 has been marked as a duplicate of this bug. ***

*** Bug 154453 has been marked as a duplicate of this bug. ***

*** Bug 305967 has been marked as a duplicate of this bug. ***

It seems that this checkin introduced bug 323723.

I strongly feel that the SMTP setting should be under an account's "Server Settings". It's a server, isn't it? That's the first logical place to look. But it's not there! :( Also, someone mentioned setting the default server in bold. I think this is a good idea, but I would like to suggest putting the account's currently selected server in bold instead. Alternatively, one could put the default server in bold while putting the current setting at the top of the list between two separators.

I must say that the interface has improved a bit since I last complained, but it is still not the real thing. And I just got a brilliant idea: "Send" button must be just like the "Print" button, that is, it should be equipped with a pop-up window (or a pull-down menu) that would allow a poor user to pick up the SMTP server of her convenience at the moment. For me, SMTP servers are just like printers: there is a default one that I use more often than others, and yet in some other circumstances I'd like to switch to a different one without having to click through various settings, but simply by selecting it in print options... err, send options menu.

In comment #161, Tony makes a good point about changing the weight of the default server, and this should be easily accomplished in userChrome.css (I'd have to look). Beyond that, it could be added as a user pref. Insofar as moving the outgoing server settings to each account, I think this is counter-productive. If you have five POP3 accounts, you still have to check them from one location. You have the ability to set the preferred SMTP server for each account already, so keeping the SMTP server settings together at the bottom of the account list makes the most sense (to me, at least). Oxana (comment #162), I think what you are suggesting should be a separate RFE, as the on-the-fly server selection is not really part of the server settings UI, which this bug addresses (and I am not saying that that was your implication; merely pointing out that you might want to open a separate RFE for your idea, which I think is a good start). Apologies to everyone on this closed bug for the bugspam. Lewis

This bug is fixed; if there are suggestions for additional changes, they should be opened as new RFEs -- assuming there isn't such a bug open already.

Bug 599730 is RFE for an additional change, for [Settings...] button in comment #149, opened three years later.