202468 - Simpler, more consolidated UI for SMTP server settings

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, enhancement)

Description

[Christian Eyrich]

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.

Comment 1

[Christian Eyrich]

Attachment: example for new UI

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

Comment 2

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 3

[Christian Eyrich]

Attachment: updated example

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

Comment 4

[WADA:World Anti-bad-Duping Agency]

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.

Comment 5

[Christian Eyrich]

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.

Comment 6

[Stefan Borggraefe]

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. ;-)

Comment 7

[WADA:World Anti-bad-Duping Agency]

Bug 151774 can be a DUPE of this bug.

Comment 8

[Joachim Otahal]

Attachment: Where SMTP-config should be

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.

Comment 9

[Joachim Otahal]

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".

Comment 10

[Christian Eyrich]

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.

Comment 11

[Joachim Otahal]

Attachment: A litte update - nothing left in my opinion now.

Comment 12

[WADA:World Anti-bad-Duping Agency]

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   : [e-mail]
      ... 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   : [e-mail]
      ... 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

Comment 13

[Joachim Otahal]

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.

Comment 14

[Christian Eyrich]

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 15

[WADA:World Anti-bad-Duping Agency]

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 ---

Comment 16

[WADA:World Anti-bad-Duping Agency]

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.

Comment 17

[Joachim Otahal]

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.

Comment 18

[Andy Ozment]

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

Comment 19

[R.K.Aa.]

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

Comment 20

[Joachim Otahal]

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).

Comment 21

[Christian Eyrich]

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/

Comment 22

[Joachim Otahal]

.. 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.

Comment 23

[Christian Eyrich]

Attachment: proposed patch

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.

Comment 24

[Christian Eyrich]

Attachment: screenshot to show the drop-down for the server panel

Comment 25

[Christian Eyrich]

Attachment: screenshot to show the new SMTP server panel

Comment 26

[Christian Eyrich]

Attachment: screenshot to show the drop-down for the server panel - updated

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

Comment 27

[Andy Ozment]

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!

Comment 28

[Christian Eyrich]

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.

Comment 29

[Andy Ozment]

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!

Comment 30

[Christian Eyrich]

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.

Comment 31

[Joachim Otahal]

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 ??

Comment 32

[Christian Eyrich]

> 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.

Comment 33

[[e-mail]]

I think top.selectServer(NC_NS + "PageTitleSMTP") should select the SMTP panel.

Also note that bug 235872 could become useful here.

Comment 34

[WADA:World Anti-bad-Duping Agency]

(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?

Comment 35

[Christian Eyrich]

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.

Comment 36

[Christian Eyrich]

Attachment: patch v5

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.

Comment 37

[Christian Eyrich]

Attachment: screenshot of the new SMTP server panel (v5)

Comment 38

[Christian Eyrich]

Attachment: screenshot of the drop-down in the main panel (v5)

Comment 39

[Christian Eyrich]

Attachment: screenshot of the drop-down in the identity window (v5)

Comment 40

[Oxana Smirnova]

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

Comment 41

[Joachim Otahal]

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)

Comment 42

[Christian Eyrich]

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.

Comment 43

[Stefan Borggraefe]

(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. ;-)

Comment 44

[Christian Eyrich]

> 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.

Comment 45

[[e-mail]]

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?

Comment 46

[Christian Eyrich]

> 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.

Comment 47

[Christian Eyrich]

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).

Comment 48

[Christian Eyrich]

Attachment: patch v6

up-to-date patch

Comment 49

[Christian Eyrich]

Comment on attachment 151360 [details] [diff] [review]
patch v6

Trying to get a comment from a reviewer.

Comment 50

[Gene Wood]

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.

Comment 51

[WADA:World Anti-bad-Duping Agency]

(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 52

[[e-mail]]

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.

Comment 53

[Christian Eyrich]

(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?

Comment 54

[[e-mail]]

(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 <[e-mail]>

Comment 55

[Christian Eyrich]

(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.

Comment 56

[[e-mail]]

(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...

Comment 57

[Jan Varga [:janv]]

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.

Comment 58

[Christian Eyrich]

(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?

Comment 59

[Stefan Borggraefe]

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

Comment 60

[[e-mail]]

(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.

Comment 61

[Christian Eyrich]

(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.

Comment 62

[[e-mail]]

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.

Comment 63

[Christian Eyrich]

(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).

Comment 64

[Jungshik Shin]

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.

Comment 65

[Jungshik Shin]

nominating for TB 1.0 (perhaps, some TB specific changes in mail/ need to be added)

Christian, is your patch ready for review?

Comment 66

[Scott MacGregor]

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

Comment 67

[WADA:World Anti-bad-Duping Agency]

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?

Comment 68

[Steve Holden]

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.

Comment 69

[[e-mail]]

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 :-/

Comment 70

[Christian Eyrich]

(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).

Comment 71

[Christian Eyrich]

(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.

Comment 72

[Steve Holden]

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

Comment 73

[WADA:World Anti-bad-Duping Agency]

(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.

Comment 74

[[e-mail]]

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.

Comment 75

[Christian Eyrich]

(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.

Comment 76

[Christian Eyrich]

(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?

Comment 77

[[e-mail]]

(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?

Comment 78

[Christian Eyrich]

(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.

Comment 79

[[e-mail]]

(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();

Comment 80

[Christian Eyrich]

(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?

Comment 81

[[e-mail]]

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 :-(

Comment 82

[Christian Eyrich]

Attachment: patch v9

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 83

[[e-mail]]

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.

Comment 84

[Stephen Donner [:stephend] Not actively reading bugmail]

(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. 

Comment 85

[Jungshik Shin]

(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'.
 

Comment 86

[Christian Eyrich]

(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.

Comment 87

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 88

[Christian Eyrich]

Attachment: patch v10

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 89

[[e-mail]]

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.

Comment 90

[Christian Eyrich]

(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.

Comment 91

[WADA:World Anti-bad-Duping Agency]

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

Comment 92

[Christian Eyrich]

Attachment: patch v11

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.

Comment 93

[Jose Fandos]

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
[e-mail] won't allow sending messages from [e-mail]. To
send messages from [e-mail]  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 94

[[e-mail]]

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.

Comment 95

[Christian Eyrich]

(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.

Comment 96

[Jungshik Shin]

(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|

Comment 97

[[e-mail]]

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).

Comment 98

[Christian Eyrich]

(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);

Comment 99

[Christian Eyrich]

Attachment: patch v12

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

Comment 100

[David :Bienvenu]

Comment on attachment 172008 [details] [diff] [review]
patch v12

Scott does want to review this, since he has some concerns...

Comment 101

[Philippe Fremy]

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

Comment 102

[WADA:World Anti-bad-Duping Agency]

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

Comment 103

[WADA:World Anti-bad-Duping Agency]

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

Comment 104

[WADA:World Anti-bad-Duping Agency]

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

Comment 105

[Johannes Walther]

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.

Comment 106

[Joachim Otahal]

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.

Comment 107

[Jungshik Shin]

(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.

Comment 108

[Joachim Otahal]

>  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.

Comment 109

[Jens Bannmann]

> 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

Comment 110

[Joachim Otahal]

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

Works. Thank you. Lots.

Comment 111

[Max]

hey, what about Thunderbird ????

Comment 112

[Scott MacGregor]

Attachment: screen shot with the latest patch

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

Comment 113

[Scott MacGregor]

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. 

Comment 114

[Joachim Otahal]

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)

Comment 115

[[e-mail]]

(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 :-(

Comment 116

[Joachim Otahal]

(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).

Comment 117

[[e-mail]]

(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.

Comment 118

[Max]

'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?

Comment 119

[Scott MacGregor]

Attachment: rough screen shot showing what I was trying to describe in my review comments

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

Comment 120

[Scott MacGregor]

Attachment: screen shots of all the new windows

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.

Comment 121

[Scott MacGregor]

Attachment: proposed fix

Comment 122

[Jens Bannmann]

(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 123

[Scott MacGregor]

Attachment: cvs diff -u (no -uw) version of the patch

Comment 124

[David :Bienvenu]

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.

Comment 125

[Jose Fandos]

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.

Comment 126

[Scott MacGregor]

(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.

Comment 127

[Scott MacGregor]

Attachment: mailnews\compose portion of the fix

I left this out of the original patch by mistake

Comment 128

[Scott MacGregor]

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 129

[[e-mail]]

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 130

[[e-mail]]

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.

Comment 131

[Scott MacGregor]

Attachment: updated patch based on review comments

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.

Comment 132

[Alex Ihrig [:Thunderbird_Mail_DE]]

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

Comment 133

[Alex Ihrig [:Thunderbird_Mail_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.

Comment 134

[Max]

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

Comment 135

[Alex Ihrig [:Thunderbird_Mail_DE]]

Both.

See above, there are Thunderbird-Screenshots.

Comment 136

[Jungshik Shin]

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

Comment 137

[[e-mail]]

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).

Comment 138

[Scott MacGregor]

Attachment: another updated patch based on the latest review comments

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 139

[[e-mail]]

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.

Comment 140

[Scott MacGregor]

Attachment: updated patch based on the latest review comments

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

Comment 141

[[e-mail]]

(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.

Comment 142

[Scott MacGregor]

Attachment: updated patch ready for checkin

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.

Comment 143

[Scott MacGregor]

fixed!

Comment 144

[Stefan Borggraefe]

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

Comment 145

[Stephen Donner [:stephend] Not actively reading bugmail]

Might have caused bug 284798.

Comment 146

[WADA:World Anti-bad-Duping Agency]

Mscott, I've filed Bug 284926.

Comment 147

[Doug Wright]

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

Comment 148

[Alex Ihrig [:Thunderbird_Mail_DE]]

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.

Comment 149

[Peter Lairo]

(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.

Comment 150

[Alex Ihrig [:Thunderbird_Mail_DE]]

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

Comment 151

[u63580]

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

Comment 152

[Matthias Versen [:Matti]]

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

Comment 153

[David :Bienvenu]

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

Comment 154

[Adam Guthrie]

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

Comment 155

[Adam Guthrie]

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

Comment 156

[Ville Pohjanheimo (inactive)]

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

Comment 157

[OstGote!]

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

Comment 158

[OstGote!]

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

Comment 159

[Mike Cowperthwaite]

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

Comment 160

[zug_treno]

It seems that this checkin introduced bug 323723.

Comment 161

[Tony Rogers]

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.

Comment 162

[Oxana Smirnova]

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.

Comment 163

[Lewis Rosenthal]

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

Comment 164

[Mike Cowperthwaite]

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.

Comment 165

[WADA:World Anti-bad-Duping Agency]

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