52330 - Support non-default SMTP Port

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P3)

Description

[Ninoschka Baca]

Overview: As referenced in bug# 14354...
The Account Setting's Outgoing SMTP Server panel should have support for 
multiple ports. UI work needs to be done and possibley the backend.

Comment 1

[Alec Flett]

massive reassign of account manager bugs -> sspitzer
please feel free to put me back on the CC if you have any questions/comments

Comment 2

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

mass re-assign of account manager bugs to racham.

Comment 3

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

adding john

Comment 4

[Ninoschka Baca]

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

Comment 5

[Frank Belew]

Changing platform to all, since my linux bug was marked as a dup of this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Denis Antrushin]

Attachment: support for non-default smtp ports

This patch enables setting non-default port for SMTP server (in mailnews
prefs panel).

Comment 8

[Jerome Lacoste]

I would really like the backend of this patch fixed as it fixes bug 106631.
Bug 106631 has also a patch (more or less a hack) which, if applied will 'rot'
that one. Perhaps a dependency should be added between both.

Denis 
- you should perhaps break this patch in 2 (backend and UI), so that if the
backend is applied, bug 106631 could be solved by modifying user preferences.
- I've also had a look to the backend section of your patch (id=58524) and I
have some comments plus some proposed clean-ups. [Note: I do not have any kind
of knowledge about the code involved]

The split and my comments will perhaps help for the whole patch to be
reviewed/applied quicker.



From attachment id=58524

RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpServer.cpp,v
[...]
+nsSmtpServer::GetPort(PRInt32 *aPort)
+{
+       NS_ENSURE_ARG_POINTER(aPort);

NS_ENSURE_ARG_POINTER(aPort); 
should perhaps go after the variable declarations (2 lines under). That's how it
is done in all the other nsSmtpServer::GetXXX() methods. It is perhaps a
security against build bustages with some compilers.

+       nsresult rv;
+       nsCAutoString pref;
+       nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));

missing a check:
  if (NS_FAILED(rv)) return rv; 

+       getPrefString("port", pref);

missing default initialization:
*aPort = PORT_SMTP; (add the missing include file as well)

+       rv = prefs->GetIntPref(pref.get(), aPort);
+       if (NS_FAILED(rv)) *aPort = 25;

replace this if [...] with 
  rv = prefs->GetIntPref(pref.get(), aPort););
  if (NS_FAILED(rv))
    rv = getDefaultIntPref(prefs, 0, "port", aPort);

// note rv is not needed here... cf clean-up section.

+       return NS_OK;

// not needed cf clean-up section.


+}
+
+NS_IMETHODIMP
+nsSmtpServer::SetPort(PRInt32 aPort)
+{
+       nsresult rv;
+       nsCAutoString pref;
+       nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));

missing ??
  if (NS_FAILED(rv)) return rv; 

+       getPrefString("port", pref);
+       if (aPort)
+               return prefs->SetIntPref(pref.get(), aPort);
+       else
+               prefs->ClearUserPref(pref.get()); /* XXX or set it to default
port? */
+       return NS_OK;
+}

I am not sure of this big if (aPort) [...]
It looks like copied from the nsSmtpServer::SetHostname(aHostName) method ;)
I would just have:

  return prefs->SetIntPref(pref.get(), aPort);

[...]
@@ -204,7 +208,7 @@
 
 
	urlSpec += (const char*)aSmtpHostName;
 
	urlSpec += ':';
-
	urlSpec.AppendInt(SMTP_PORT);
+
	urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT);

I am not sure that this line is useful as we already have the default port value
set to SMTP_PORT in getPort().


+++++++++++++++++
 other clean ups
+++++++++++++++++
I've added some other comments and cleaning up.
in file mailnews/compose/src/nsSmtpServer.cpp: 
- in line 154: 
nsSmtpServer::getDefaultIntPref doesn't need a return value as it always return
NS_OK. (appears misleading to have variables affected to that return value,
variables never used)
Should we replace

  if (NS_FAILED(rv))
  { // last resort
    *val = defVal;
    rv = NS_OK;
  }
  return rv;

by
  if (NS_FAILED(rv))
  { // last resort
    *val = defVal;
  }

as a consequence, 
- line 123, method  nsSmtpServer::GetTrySSL(...) doesn't need a parameter neither


while method nsSmtpServer::GetAuthMethod(...) returns rv

Same for getPrefString() which always return NS_OK

- line 105, else not needed (because return value from ClearUserPref() not used.
- line 205 else not needed.
- line 403 else not needed.

- line 427: rv for call to SetRedirectorType(...) ignored 

- line 90, 102, 184, 202, 400, 413
missings NS_FAILED(rv) checks after do_GetService() 

- All these  "if (NS_FAILED(rv)) return rv;"
should perhaps be
  NS_ENSURE_SUCCESS(rv, rv);     // like in line 440?

- no error checking in method  nsSmtpServer::clearPrefEnum() after call to
ClearUserPref(...)

- methods: nsSmtpServer::GetHostname(), ::GetUsername()
lacks NS_ENSURE_ARG_POINTER(trySSL);
++++++++++++++

Hope it helps.

Comment 9

[Denis Antrushin]

Attachment: patch, v.2

Comment 10

[Denis Antrushin]

> - you should perhaps break this patch in 2 (backend and UI), so that if the
> backend is applied, bug 106631 could be solved by modifying user preferences.
Jerome, UI part is just 4 more lines in JS plus some xul cut-n-paste.
It so trivial , so it can be easily checked in together with backend part IMHO

> replace this if [...] with 
> rv = prefs->GetIntPref(pref.get(), aPort););
>  if (NS_FAILED(rv))
>    rv = getDefaultIntPref(prefs, 0, "port", aPort);
Heh, getDefaultIntPref will always return zero ;-)

>       if (aPort)
>               return prefs->SetIntPref(pref.get(), aPort);
>       else
>               prefs->ClearUserPref(pref.get()); 
>
> I am not sure of this big if (aPort) [...]
> It looks like copied from the nsSmtpServer::SetHostname(aHostName) method ;)
> I would just have:
>
> return prefs->SetIntPref(pref.get(), aPort);
I disagree. This is how SetPort implemented for other server as well - Imap, Pop
etc.
This way we not bloating prefs with unneeded values....



>-	urlSpec.AppendInt(SMTP_PORT);
>+	urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT);
>
> I am not sure that this line is useful as we already have the default port value
> set to SMTP_PORT in getPort().

This is harmless and protects (in some sense) against bad values in prefs :-)

- line 105, else not needed (because return value from ClearUserPref() not used.
- line 205 else not needed.
- line 403 else not needed.

> - no error checking in method  nsSmtpServer::clearPrefEnum() after call to
> ClearUserPref(...)
What we would do with them?

Comment 11

[Jerome Lacoste]

> Jerome, UI part is just 4 more lines in JS plus some xul cut-n-paste.
> It so trivial , so it can be easily checked in together with backend part IMHO

That's ok for me. BTW, cut-and-pasted has always been the reason for half of my
bugs :)

>> replace this if [...] with 
>> rv = prefs->GetIntPref(pref.get(), aPort););
>> if (NS_FAILED(rv))
>>   rv = getDefaultIntPref(prefs, 0, "port", aPort);
> Heh, getDefaultIntPref will always return zero ;-)

;) 
OK replace it by 
  if (NS_FAILED(rv))
    getDefaultIntPref(prefs, 0, "port", aPort);

I still think the call to getDefaultIntPref() can help. Otherwise it is
inconsistent with the fact tha all other preferences could have a default value
but not that one.

>> if (aPort)
>> return prefs->SetIntPref(pref.get(), aPort);
>> else
>> prefs->ClearUserPref(pref.get()); 
>>
>> I am not sure of this big if (aPort) [...]
> I disagree. [...] This way we not bloating prefs with unneeded values....

You're the boss

>>-
urlSpec.AppendInt(SMTP_PORT);
>>+
urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT);
>This is harmless and protects (in some sense) against bad values in prefs :-)

agreed

>> - no error checking in method  nsSmtpServer::clearPrefEnum() after call to
>> ClearUserPref(...)
> What we would do with them?

I do not know. I just raised the issue. What about returning it?

So, apart from the missing getDefaultIntPref(), I am happy with your new patch.
Thanks a lot!

I would really like this bug fixed. There are 9 votes for bug 106631. They could
[should] come on that one. Let's try to get somebody review that patch.

Comment 12

[Denis Antrushin]

>I would really like this bug fixed. There are 9 votes for bug 106631. They
could
> [should] come on that one. Let's try to get somebody review that patch.

How'd you do it? Mailnews team looks totally deaf these days...
CC-ing mscott and bievvenu, maybe this will help...

Comment 13

[David Hallowell]

Try tracking down the mailnews people on IRC to request a review. It's very easy
to miss a bug that needs review in the mass of bugzilla notifications they
probably receive.

Comment 14

[David :Bienvenu]

we're in a performance lockdown for the next month (and have been for the past
month). During this time, we're not working on new features, only
performance,footprint, and memory leaks, and recent regressions.

Comment 15

[Jens Krinke]

As one can see from all the duplicate bugs, this is a showstopper for many
people (including myself). Sending mail via a different ports *did* work in
0.9.4 - I would call that a recent regression. 0.9.9 is too far away.

Comment 16

[David :Bienvenu]

I don't disagree, but it's not up to me. I'll try to get special dispensation
from the powers that be.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Adding keywords for Pastis51. This has worked in 0.9.4 -> regression
And changing summary to make it understandable.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Enhanced patch

Your patch looks good, I enhanced it a bit:

- I removed the hard tabs, replaced with spaces.

- I changed nsSmtpServer::GetPort slighty, to have a more similar error
handling as GetHostname.

- There are two listboxes in preferences that list the hostname of a SMTP
server, in "Advanced Account Settings", and in "Advanced Outgoing Server (SMTP)
Settings". If a nonstandard-port is used, those strings now show the port
number, too.

I tested sending to default and non-default ports, both work.

Comment 20

[John G. Myers]

Please keep in mind bug 2679 when designing the interface.  At some point there
will be a difference between "default" and "25".

Comment 21

[Johan Ferner]

Applied the patch to the 0.9.7 source. It works :)
I decided to start over from scratch and deleted my ~/.mozilla.
Then I discovered a quirk... If you use different localhost ports to (ssh)
tunnel your outgoing emails then you will not be able to select them in the
"account settings dialog"/advanced tab. Only one localhost entry is visible.
The others appeared after I edited the servernames to mailfoo1 and mailfoo2. I
aliased the names in my hosts file and it works. But that's a *bad* workaround ;)

Comment 22

[Mauricio Collares [:collares]]

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

Comment 23

[Mauricio Collares [:collares]]

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

Comment 24

[Olav Vitters]

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

Comment 25

[Håkan Waara]

This bug shows up almost every day at npm.mail-news and I've seen many people
ask why it doesn't work in #mozilla, I think it would be a good idea to get the
patch in soonish...

Here are a two posts from npm.mail-news, only from this week, from people
suffering the regression:
<news://news.mozilla.org:119/[e-mail]>
<news://news.mozilla.org:119/[e-mail]>

To reply to comment 14: This is not a feature, it is a regression that many
people are suffering from - regressions are allowed in according to sspitzer's
initial email.

Can this patch get r/sr now?

Comment 26

[Kai Engert [:KaiE:]]

John, re your comment 20, I think my patch is not in conflict with a need to
have a different default port in the future, or do you?
The current code simply assumes, if there is no specific port configured, we use
25. In the future, if other default ports are needed, we simply must make sure
that port numbers are explicitly saved, and it will work with the code suggested
here.

Comment 27

[John G. Myers]

What is in conflict is the assumption that there is a single default port.  With
bug 2679, there would not be a single default port, the default behavior would
be to first try port 587 then try port 25.  That default behavior would be
different than what one would get by explicitly configuring either port 587 or
port 25.

Comment 28

[Kai Engert [:KaiE:]]

John, thanks for clarifying.

Message submission seems to be described in rfc 2476. It seems to me, what is
requested is an "additional initial test action, with fallback if necessary". As
we need to fallback in case of trouble, and the fallback port is the port
requested in this bug 52330, it might make sense to add an additional preference
to the smtp settings. We could have a checkbox that says "try to use message
submission protocol first, using port [587] <- configurable". If you think it
makes sense, that option could then be enabled by default as soon as we have it.
I vote for storing that preference separate from what we otherwise need for
standard smtp.  And I think in case we want to offer overriding the port number
to be used for the message submission port could be configurable and separately
stored.

I'm trying to find out, whether we need to care for bug 2679 now, or whether we
can delay it until we are ready to support message submission. John, do you
agree that my suggestion is viable, or do you request the current patch to this
patch needs to be changed now?

Comment 29

[John G. Myers]

We don't have to implement bug 2679 now, but we should avoid designing the
interfaces in such a way as to make implementing it unnecessarily difficult or
cumbersome.

An additional preference would be unnecessarily complex and confusing.  The only
time you would want port scanning is for the default case, when the user doesn't
know enough or hasn't bothered to configure the SMTP port.  The better interface
would be "if the port field is blank, do port scanning".

So I would much prefer if the patch did not automatically convert between blank
and 25 in the preferences interface.  The SMTP protocol code should simply
interpret a blank preference value as 25.

Comment 30

[Denis Antrushin]

Attachment: patch, no " " <->25 conversions in pref

Here is new patch. Now UI doesn't replaces empty port with port 25.
Also I replaces a bunch of tabs with spaces.

Comment 31

[Denis Antrushin]

Attachment: same as attachment 64483, but with diff -b for easier review

Comment 32

[Denis Antrushin]

Attachment: screenshot of 'new' ;-) UI, as requested

Comment 33

[Gilles Durys]

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

Comment 34

[Matthias Versen [:Matti]]

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

Comment 35

[Gilles Durys]

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

Comment 36

[Gilles Durys]

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

Comment 37

[Quinn Yost (mythdraug)]

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

Comment 38

[Alfonso Martinez]

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

Comment 39

[David Brittain]

This must be heading for most-duped status. Can someone clarify what is holding
this bug up?

Comment 40

[Alfonso Martinez]

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

Comment 41

[Eva]

I really like your product, please continue to support this in the future!

I must use SSH tunneling for sending company mail from home.
I am using Netscape 6.21 which works great, our administrator helped me to
configure it.

Is it a mistake that this bug says nsbeta1- (I looked in keyword help), and
target milestone says 0.9.9, but keyword says 0.9.7?

Thank you

Comment 42

[Kai Engert [:KaiE:]]

Updating keyword to mozilla0.9.9, to match target milestone.

Comment 43

[Mauricio Collares [:collares]]

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

Comment 44

[Denis Antrushin]

Attachment: minot update

Just added one thing: if no port specified and hostname contains ':',
split hostname string into host and port portions...

Comment 45

[Denis Antrushin]

Attachment: diff -w version of the above

Comment 46

[Jerome Lacoste]

[Spam]
This bug has had a patch for more than one month. Still no reviews. 
It is a known regression with more than 20 votes (and probably more if you add
the ones from the 20 duplicates). It is part of the top 30 non-fixed most
reported bugs and part of the 7 non-fixed most reported bugs targeted for 0.9.9.

Could someone please have a look at the patch? Otherwise it will never make it
for 1.0.0

Cc Asa.

Comment 47

[scottputterman]

This will have to wait some more.

Comment 48

[David Brittain]

Can you give a reason why this has to wait some more?

Comment 49

[scottputterman]

The people who are the best reviewers for this have other bugs that they need to
work on first.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Well...  people who are regulars on the project should really be a little more
welcoming than that to people who are trying to help out...

Marking this as blocking "bugs with patches that need review before they rot". 
Will try to get to it this Friday.

Comment 51

[Denis Antrushin]

BTW, fix is quite simple, it won't take too much time to review...

Comment 52

[Kai]

Given the fact that the mail team does not have much time to help on this, let's
try to make the patch perfect, to minimize the risk of further delays because of
unaddressed issues.

If I were to review it, I would have the following comments:

- the patch has hard tabulators in it. That is not allowed. Please replace with
spaces. Also make sure that both indendation and bracket placement matches the
style of the files we modify.

- the patch should allow migration from those version of Mozilla which worked
with the :port notation. I.e., if you upgrade from 0.9.4, and you currently have
a host:port configuration, the code should detect that, and migrate the preferen
automatically. I'm not sure whether we are allowed to write the updated prefs to
disk immediately. Ideally, the migrated prefs should be used in memory and
displayed correctly in the pref screens, but the new pref should now be written
to disk until the user ok's the pref dialog containing the migrated setting.

- implementation of nsSmtpServer::GetPort should more closely match the
implementation of GetHostname. I suggest that instead of
  *aPort = 0;
  rv = prefs->GetIntPref(pref.get(), aPort);
  return NS_OK;
we should write:
  rv = prefs->GetIntPref(pref.get(), aPort);
  if (NS_FAILED(rv)) *aPort = 0;
  return NS_OK;

- Implementation of GetDisplayName(): We should explicitly set port to zero, if
we detect failure of GetIntPref(), because in that cases the value of given out
parameters may be undefined. I suggest that instead of

  PRInt32 port = 0;
  rv = prefs->GetIntPref(ppref.get(), &port);
  if (port<0)
    port = nsISmtpUrl::SMTP_PORT;
we should write:
  PRInt32 port = 0;
  rv = prefs->GetIntPref(ppref.get(), &port);
  if (NS_FAILED(rv))
    port = 0;

As soon as we have that updated patch, we should test it. We could create test
builds, and people subscribed on this patch could help testing. This will allow
us to detect early any issues not addressed by the patch.

If you can please help to created an updated patch.
I will work at the weekend and try to help, too.
I can also create test builds for Linux and Windows.

Comment 53

[Denis Antrushin]

Attachment: yet another minor update

Also removed few unneeded (const char*) casts

Comment 54

[Denis Antrushin]

Attachment: diff -w version of the above

please do 'man diff', if you don't know what -w means!

Comment 55

[Boris Zbarsky [:bzbarsky]]

Denis, does your latest patch address the "the patch should allow migration from
those version of Mozilla which worked with the :port notation" comment?  (It
looks like it makes the other changes kaie suggested..

Comment 56

[Denis Antrushin]

Yep. 
And it will migrate to new version (using port pref) after user 
visited prefs panel and pressed OK button

Comment 57

[Boris Zbarsky [:bzbarsky]]

OK.  Reading through that code it seems quite sensible to me...  hwaara, would
you be able to revie this?

Comment 58

[Håkan Waara]

Comment on attachment 70913 [details] [diff] [review]
diff -w version of the above

>@@ -85,6 +80,8 @@
>     attribute nsIAuthPrompt authPrompt;
>     attribute nsIInterfaceRequestor notificationCallbacks;
>     attribute nsISmtpServer smtpServer;
>+    
>+    const PRInt32 SMTP_PORT = 25;
> };

I suggest you rename this constant to DEFAULT_SMTP_PORT to avoid confusion in
the future.

>+// if GetPort returns 0, it means default port

Instead of making all the callers check for 0 and use the default port, can't
you just make this getter smart enough to return 25 if necessary? 

Everything else looks great.  r=hwaara with the minor changes above.

Comment 59

[Denis Antrushin]

> Instead of making all the callers check for 0 and use the default port, can't
> you just make this getter smart enough to return 25 if necessary? 

It was so in early versions of the patch :-)
I don't recall now why I changed it, but ok, I'll change it back

Comment 60

[Kai Engert [:KaiE:]]

Did you do it, because of John's request to support the case, where the default
port could be something else but 25? Instead of hard coding 25 everywhere, by
storing zero we remember the state whether a port has not been set yet. In that
case, a global option changing the default port, would automatically be used.

Comment 61

[Håkan Waara]

"global option changing the default.."?  The point is, either you use your
customized port (in which case it isn't default anymore) or you use the default,
which will be 25 - because it's most common one.

Comment 62

[Ben Johnson]

See John G. Myers comments #27
http://bugzilla.mozilla.org/show_bug.cgi?id=52330#c27 and #29
http://bugzilla.mozilla.org/show_bug.cgi?id=52330#c29

Comment 63

[Kai]

John requested in comment 27 and comment 29, to NOT store port number 25 in
preferences.

> > Instead of making all the callers check for 0 and use the default port, can't
> > you just make this getter smart enough to return 25 if necessary? 
> 
> It was so in early versions of the patch :-)
> I don't recall now why I changed it, but ok, I'll change it back

Therefore we should not do it. In order to support bug 2679, we should leave it
as the patch currently is, i.e. check for !port in the code, which means, try
the default port.

Håkan, do you agree?


> I suggest you rename this constant to DEFAULT_SMTP_PORT to avoid confusion in
> the future.

Ok, good idea.


> > Denis, does your latest patch address the "the patch should allow migration from
> > those version of Mozilla which worked with the :port notation" comment?  (It
> > looks like it makes the other changes kaie suggested..
> Yep. 
> And it will migrate to new version (using port pref) after user 
> visited prefs panel and pressed OK button

Your code tries to do automatic conversion in one place only: 
When the user ok's mail preferences, and the hostname is still in 
the old notation, and no port has been specified yet.
In that case, a correct conversion is done.

However, sending mail does not work until you have done the above.
Thinking more about it, we have two options how the application should behave.

Option 1)
Suppose we do automatic migration on program start.
This will mean:
- Sending mail will work immediately, no user action required.
- We automatically modify the profile, which might not be what the user wants,
  because on going back to older version of Mozilla, sending will not work.
  I don't know whether me must support this, but if we don't, we will break
  the environment of those people who just want to try out a newer Mozilla,
  but decide to continue to use the older version (with the same profile).
- The patch needs to be enhanced more, because we need to find the right place
  to add the automatic migration.
  
Option 2)
Suppose we do NOT migrate automatically on program start.
This will mean:
- Sending mail will NOT work immediately when switching to the newer version of
Mozilla.
- We do not modify the profile, therefore temporarily trying out a newer version
  of Mozilla will not change the profile that works with the older version of
Mozilla.
- A user has to know that it is required to make the change.
  However, those users who found the hack in the past to use to :port notation,
  will likely open the preferences dialog to see what's configured,
  and notice the new field for the port value, and fix it manually.
  If we assume that, that automatic conversion on saving is not even necessary.


I'd now suggest:
Let's go with Option 2.
Be safe (no automatic migration) and simple (no more code needs to be developed).
Suppose only power users used the alternate port in the past, so we only risk to
surpise power users.
In addition, that alternative port was not really supported in the past, so no
need for migration.
On the other hand, that automatic conversion on save can't hurt, so let's leave
it in.

Please comment if you disagree.
If you agree, I'll make test builds available.

Comment 64

[mark]

Great to see that this is being worked on.

With regard to the comments about conversion from 'host:port' notations: I agree
that both solutions may cause problems for some users.

So, at the risk of being naive: why not be flexible and allow that system to
work too? If someone has host:port in the profile, leave it that way, and
interpret it correctly.

Then no-one will have upgrade problems.

Comment 65

[Denis Antrushin]

> However, sending mail does not work until you have done the above.

What makes you say that? Did you ever tried it? If no, please do it now.
Sending mail works even with old profile with 'server:port' hostname w/o
visiting preferences page.
This is what "if(!PL_strchr(aSmtpHostName':'))" for (in nsSmtpService.cpp).

Comment 66

[Kai]

> > However, sending mail does not work until you have done the above.
> 
> What makes you say that? Did you ever tried it? If no, please do it now.
> Sending mail works even with old profile with 'server:port' hostname w/o
> visiting preferences page.
> This is what "if(!PL_strchr(aSmtpHostName':'))" for (in nsSmtpService.cpp).

Oops, sorry. I missed that change. I just tested, and indeed, it works.

You added the workaround that was originally suggested in bug 106631 - but which
had been rejected at that time. I agree the code makes sense, because it makes
it work without having to worry about the migration issues I mentioned.

But now that the other requests from the mail team have been implemented, I'd
argue this change helps migrating users, and should be used.

I finally think the last patch is very good. Given the fact the patch makes it
work with both preference notations, the only thing that is left is the question:

Should pressing enter in preferences automatically split host:port or not?
If we not do the automatic split, we ensure that older versions of Mozilla will
still work with the profile. The user has to manually make that change if he
wants to, but is not required to.

Comment 67

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

we'll need jglick to approve the UI changes and update the spec.
http://www.mozilla.org/mailnews/specs/accounts/#AccountSettings

Comment 68

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

as far as UI goes, this patch also affects the "edit" advanced SMTP server
dialog as well, right?

Comment 69

[Denis Antrushin]

Yes, because "edit" uses same smtp server overlay.

Comment 70

[jglick]

The only UI change is adding a "Port" text field to the "Outgoing Server 
Settings" panel and the Advanced "SMTP Server" dialog (used to add a new, or 
edit an existing, SMTP server)?  And the text field will be pre-populated with 
an appropriate default?

http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerSmtp1.gif
http://www.mozilla.org/mailnews/specs/accounts/images/AcctSmtpAdvDiag1.gif

If that is the case, UI change is fine with me and I will update the spec.

Comment 71

[Kai]

> The only UI change is adding a "Port" text field to the "Outgoing Server 
> Settings" panel and the Advanced "SMTP Server" dialog (used to add a new, or 
> edit an existing, SMTP server)?  

Yes, only the dialogs you listed have been changed.


> And the text field will be pre-populated with 
> an appropriate default?

Good point, not yet, let's fix that.

Right now, the port number field is empty when the default port is configured.

I suggest: When these dialogs are constructed, and no port setting is detected,
we fill the edit field with the default port number.

On closing the dialog, we check whether the port field contains the default port
number. If it does, we set the preference internally back to the default setting
(which is zero), and store that.

(Sideeffect: I removed the automatic host:port split on save. I think we do not
need it, because the smtp procotol code works anyway.)

I'll attach a new patch. That patch will implement my suggestion and prefills
the port number fields.

It also contains the suggested global replacement SMTP_PORT => DEFAULT_SMTP_PORT.

Comment 72

[Kai]

Attachment: New patch with filled port fields

Comment 73

[Kai]

Attachment: New patch with filled port fields (diff -w version)

Comment 74

[Denis Antrushin]

When bug 2679 will be fixed, "server" and "server:25" will mean different
things!

Comment 75

[Kai]

> When bug 2679 will be fixed, "server" and "server:25" will mean different
> things!

*sigh*, this is really complicated :)

In my opinion, as long as bug 2679 is not implemented, server IS identical with
server:25. Maybe it is not identical as soon as message submission protocol
becomes available, but I suggest to fix that later when bug 2679 gets implemented.

In my opinion, if bug 2679 gets implemented, the user should have a way to
control this in the UI. The application should not just try out port 587,
without power users being aware of that fact. So I'd personally prefer a setting
(later), that controls whether "port submission on 587" will be tried or not.

By the time we have that new setting, we can make the UI more intelligent. It
will still be detectable whether a user has ever configured a nonstandard port
or not, because the port setting will be zero.

By the time bug 2679 gets fixed, the user interface can detect that, and
indicate in the UI whatever is necessary. It could, for example ,show:
- default (first try port submission, then try SMTP)
- only use port submission
- only use SMTP
  _____
 | 587 | port for message submission
 -------
 _______
 | 25  | port for SMTP
 -------

As soon as message submission protocol becomes available, power users will
recognize that anyway, and manually configure their client appropriately.

If users had ever configured an alternate port, the future implementation could
default to the setting "only use SMTP".

But I really think we don't have to solve that now. The only thing that we are
doing is: We remember correctly, whether a user has ever configured an alternate
port or not, and that is all what the implementors of bug 2679 will need IMHO.
And the implementators of bug 2679 can then decide how Mozilla should behave,
depending on what has been configured in the past.

Comment 76

[Denis Antrushin]

> (Sideeffect: I removed the automatic host:port split on save. 
> I think we do not need it, because the smtp procotol code works anyway.)

This makes new UI useless/redudant ;-)

Comment 77

[Kai]

Test builds that use the previous patch are available at
  http://www.kuix.de/mozilla/52330/

Comment 78

[Kai]

> > (Sideeffect: I removed the automatic host:port split on save. 
> > I think we do not need it, because the smtp procotol code works anyway.)
>
> This makes new UI useless/redudant ;-)

I don't think so. The :port notation was never officially supported. While your
patch support migration, because of the smtp protocol code behaviour, we are
free to remove support for that notation later, but the UI will survive. And the
new UI is consistent with what we are doing for other email protocols, that is
offering a edit field for port configuration, not forcing a user to user
host:port notation.

Comment 79

[John G. Myers]

Please do not fill in the default port.  Please see comments #27 and #29.

Comment 80

[John G. Myers]

In response to comment #75, if mozilla fills in the default port, how do you
expect it to later detect whether or not the user explictly configured port 25?

With bug 2679 implemented, the UI would remain the same.  If the port field were
left blank, the code would do the default behavior of first trying port 587 then
25.  The user could disable checking port 587 by explicitly configuring port 25
and could disable checking port 25 by explicitly configuring port 587.  It does
not make sense to configure two ports--if the user is going through the trouble
of changing the defaults, they can just configure the one port their particular
server uses.

Comment 81

[David :Bienvenu]

Comment on attachment 71473 [details] [diff] [review]
New patch with filled port fields (diff -w version)

I'll let mscott deal with the substantive issues, if any, but a stylistic
comment first:

We like the error returns to be on a separate line (makes it easier to set
breakpoints) so instead of

if (NS_FAILED(rv)) return rv;

should be 

if (NS_FAILED(rv))
  return rv;

Do we really need to show the port number in the display name?

Comment 82

[Kai]

> In response to comment #75, if mozilla fills in the default port, how do you
> expect it to later detect whether or not the user explictly configured port 25?

Sorry, I was assuming this does not make a difference.


> With bug 2679 implemented, the UI would remain the same.  If the port field were
> left blank, the code would do the default behavior of first trying port 587 then
> 25.  The user could disable checking port 587 by explicitly configuring port 25
> and could disable checking port 25 by explicitly configuring port 587.  It does
> not make sense to configure two ports--if the user is going through the trouble
> of changing the defaults, they can just configure the one port their particular
> server uses.

Thanks for explaining it again.
So I think we have two options:


Option 1)
=========
Leave the port field empty, as it is currently implemented.

This behaves different from the other mail preferences, which always show a
number in the port field.

If everybody likes that approach, it's fine with me, too.
However, Jennifer expected that field would be filled, so I'd like to make
another suggestion:


Option 2)
=========
  Server   ________________________

  x  use default port
  O  use alternate port: _____

I.e., a radio button, and a field to explicitly enter the port number.
If "use default port" is selected, the edit field for the alternate port is
disabled.


Jennifer, John, what do you think?

Comment 83

[Kai]

As a consequence of the previous request, the patch will have to be updated, to
always store the port number, even if it is equal to the default port.

If "default port" is selected, the preferences would store port number zero.

If "alternate port" is selected the preferences would store the entered port number.

Comment 84

[Kai]

> I'll let mscott deal with the substantive issues, if any, but a stylistic
> comment first:

I'd like to state my commitment that I'll do whatever is required to land this
patch for Mozilla 1.0.


> We like the error returns to be on a separate line (makes it easier to set
> breakpoints) so instead of

Sure, I will fix this in the patch.


> Do we really need to show the port number in the display name?

The port number is only displayed, if it is different from the default port number.
It seems one scenario for the alternate port is tunneling. And if you use
tunneling, all your host names will be the same. I think it is a common scenario
to have two alternate servers for sending mail. So both host names would be
displayed as "127.0.0.1" and you are only able to differ them by the port number.

I notice that the current implementation uses a display notation of "host:25".
As we do not support that notation, should we display "host (port 25)" instead?

Comment 85

[Kai]

Sorry, for the spam, I correct myself:

> The port number is only displayed, if it is different from the default port
number.

In the alternate model, having a checkbox to decide whether to use the default
port or an alternate port, this sentence is correctly:

"The port number in the display string is only displayed, if it has been
explicitly configured". I think this affects only the advanced smtp server
dialog, where multiple servers are listed in the list box.

Comment 86

[John G. Myers]

Re comment #82, either option is fine by me.  As long as we can later
distinguish between "default" and "explicit 25".

Comment 87

[Kai]

Attachment: Screen Shot of enhanced UI (1 of 4)

Comment 88

[Kai]

Attachment: Screen Shot of enhanced UI (2 of 4)

Comment 89

[Kai]

Attachment: Screen Shot of enhanced UI (3 of 4)

Comment 90

[Kai]

Attachment: Screen Shot of enhanced UI (4 of 4)

Comment 91

[Kai]

jglick, can you please review this UI?
It realizes your wish to not have empty port fields.

Comment 92

[Kai]

Attachment: Suggested updated patch

This patch implements the alternate UI suggestion as shown by the previous
screen shots.

The code realizes jgmyers's request to be able to distinguish the different
user choices.

The code layout has been fixed to follow bienvenu's request.

Comment 93

[Kai]

Attachment: Same patch as before, but diff -w -u10 for easier reviewing

Comment 94

[jglick]

>Option 1)
>=========
>Leave the port field empty, as it is currently implemented.

This is fine with me.

Option 2)
When you have radio buttons, you should have descriptive test above the radio 
buttons, so another line would need to be added.

When sending messages: 
   x  use the default port
   O  use an alternate port: _____

Hence, to keep things simple and take up less space, Option 1 is fine with me. 

Comment 95

[scottputterman]

Marking nsbeta1-.  You can still get this into 1.0 with the proper reviews and
drivers approval.  I'd like to reassign this to [e-mail] but bugzilla doesn't
appear to be letting me do it.  Moving to 1.2.  Once it gets reassigned you can
put this back in 1.0

Comment 96

[Kai]

Assigning to me, back to milestone 1.0.

Comment 97

[Kai]

Attachment: Final enhanced patch, ready for code review

Thanks for the confirmation on the easier UI.
(I'm obsoleting the screenshot images that we are not be using.)

This patch has all the code layout changes from the previous patch, but uses
the simpler UI, where the port field is empty in the default case.

Please review the code in this patch.

Comment 98

[Kai]

Attachment: (same as previous, diff -w -u10 for easier reviewing)

Comment 99

[Håkan Waara]

Comment on attachment 72835 [details] [diff] [review]
(same as previous, diff -w -u10 for easier reviewing)

Looks good to me.

I think it would be best if bienvenu or mscott did official review though,
since they know more about this code than me.

I suggest you email them a review request, and CC [e-mail]

Comment 100

[David :Bienvenu]

Comment on attachment 72835 [details] [diff] [review]
(same as previous, diff -w -u10 for easier reviewing)

r=bienvenu, I'd try mscott for an sr.

Comment 101

[Johan Ferner]

Version 1.103+ of nsSmtpService.cpp "breaks" the patch (can't just run patch
anymore). This makes the patch unusable on mozilla 0.9.9. I'd be really happy if
I got a new patch and someone *please* integrate this patch into the CVS.

Thanks!

// Johan

Comment 102

[Kai]

Attachment: (Resolved small conflict to make it apply to current tree)

> Version 1.103+ of nsSmtpService.cpp "breaks" the patch (can't just run patch
> anymore). This makes the patch unusable on mozilla 0.9.9. 

Just a minor conflict in the context, I'm attaching a new patch that applies
cleanly.


> I'd be really happy if
> I got a new patch and someone *please* integrate this patch into the CVS.

I already posted twice to [e-mail], but no reaction yet.

Comment 103

[Kai]

Brendan, can you please superreview?

Comment 104

[Ben Bucksch (:BenB)]

> I already posted twice to [e-mail], but no reaction yet.

You have to add the relevant people to the To addresses, see
<http://www.mozilla.org/hacking/reviewers.html>.

Comment 105

[Kai]

> You have to add the relevant people to the To addresses, see
> <http://www.mozilla.org/hacking/reviewers.html>.

I did, it was addressed to mscott as bienvenu suggested, but I guess he did not
have time.

Comment 106

[Brendan Eich [:brendan]]

Comment on attachment 73961 [details] [diff] [review]
(Resolved small conflict to make it apply to current tree)

with r=bienvenu, I'm happy to consider stamping sr=[e-mail].

Note to anyone who cares: mailnews/compose/public/*.idl files do not agree on
whether to use long or PRInt32, e.g.  I think the latter are better, at a
glance you don't have to shift frame from C/C++ (where long is not a portable
type in general) to IDL (where it has a specific XP size).

But why not use PRUint32 and avoid the ugly cast in nsSmtpProtocol.cpp?

IP ports are unsigned, so I think the IDL should use an unsigned integral type.

Another thing: if LoadUrl fails, won't we leak due to the QI-result non-COMPtr?

+	 NS_ADDREF(smtpProtocol);
+	 rv = smtpProtocol->LoadUrl(aUrl, aConsumer); // protocol will get
destroyed when url is completed...
+	 smtpProtocol->QueryInterface(NS_GET_IID(nsIRequest), (void **)
aRequest);
+	 NS_RELEASE(smtpProtocol);

I think you want to QI iff NS_SUCCEEDED(rv).

Fix these issues if you agree, and sr=[e-mail].

/be

Comment 107

[Kai]

> mailnews/compose/public/*.idl files do not agree on
> whether to use long or PRInt32, e.g.  I think the latter are better, at a
> glance you don't have to shift frame from C/C++ (where long is not a portable
> type in general) to IDL (where it has a specific XP size).

In our patch, we introduced a variable with type "long".
I changed that to PRInt32.

(I can't speak for the other types in the existing mail code that we are not
touching with this patch.)


> But why not use PRUint32 and avoid the ugly cast in nsSmtpProtocol.cpp?
> IP ports are unsigned, so I think the IDL should use an unsigned integral type.

When I first read this sentence, I agreed this were a good idea.
I started to change all occurrences in our patch to unsigned.

However, I finally did not do it, because:
- the preferences code stores a signed int
- I also would have to adjust nsSmtpService::GetDefaultPort, or the "ugly cast"
would happen there.
- However, method GetDefaultPort is widespread across the source tree, and
everybody used a signed int, so I think we should be consistent.

Therefore, in order to avoid the ugly cast, my next idea was to change the type
of m_port.

In the end I saw that this variable is completely irrelevant.

For safety reasons I even scanned the whole sources below mailnews/, and that
variable is only accessed inside
  nsSmtpProtocol::Initialize()
and only values are assigned to that variable, but it's contents are never used!

I removed this variable from the header, and the useless lines in
nsSmtpProtocol.cpp, including the line with the ugly cast.
Everything still compiles and works fine.

Note that this makes our patch even simpler, because those lines which
manipulated m_port where introduced by our patch,
and because of that I think there is no need for the mail team to review again.


> Another thing: if LoadUrl fails, won't we leak due to the QI-result non-COMPtr?
> 
> +	 NS_ADDREF(smtpProtocol);
> +	 rv = smtpProtocol->LoadUrl(aUrl, aConsumer); // protocol will get
> destroyed when url is completed...
> +	 smtpProtocol->QueryInterface(NS_GET_IID(nsIRequest), (void **)
> aRequest);
> +	 NS_RELEASE(smtpProtocol);

> I think you want to QI iff NS_SUCCEEDED(rv).

The line containing LoadUrl is not changed by this patch.
It is not contained in the diff -w patch.
We just fixed some whitespace in that area to make it match the general style of
the file.
Therefore I would like to transfer your comment to a new bug.


> Fix these issues if you agree, and sr=[e-mail].

Thanks a lot!

Comment 108

[Kai]

Attachment: Patch after working on Brendan's suggestions

Comment 109

[Kai]

Attachment: (Same patch, diff -w -u10)

Comment 110

[Kai]

Bienvenu, to help you agreeing with the new patch, here is what changed since
you reviwed it:

You saw:
+    attribute long   port;
Now it's:
+    attribute PRInt32 port;


You saw:
-    m_port = SMTP_PORT;
+    m_port = 0;
Now it's:
-    m_port = SMTP_PORT;


Code that assigned values to m_port removed from the patch.


New change:

Index: mozilla/mailnews/compose/src/nsSmtpProtocol.h
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpProtocol.h,v
retrieving revision 1.38
diff -u -d -r1.38 nsSmtpProtocol.h
--- mozilla/mailnews/compose/src/nsSmtpProtocol.h	28 Sep 2001 20:06:57 -0000	1.38
+++ mozilla/mailnews/compose/src/nsSmtpProtocol.h	17 Mar 2002 04:36:48 -0000
@@ -161,7 +161,6 @@
 	PRInt32 	m_previousResponseCode; 
 	PRInt32
	m_continuationResponse;
   nsCString m_responseText;   /* text returned from Smtp server */
-
PRUint32  m_port;
 
 	char
   *m_addressCopy;
 	char
   *m_addresses;

(I could have omitted this change, but because that variable is never used, I
removed it.)

Comment 111

[Kai]

Because I changed the type of port in nsISmtpServer from long to PRInt32, I just
feared that I had forgotten to also change this type in the implementation
methods GetPort and SetPort.

But I noticed that these methods were already using PRInt32, so we improved
correctness by changing the interface type.


BTW: I filed bug 131522 for the LoadUrl issue Brendan mentioned.

Comment 112

[Brendan Eich [:brendan]]

Comment on attachment 74586 [details] [diff] [review]
Patch after working on Brendan's suggestions

I read the -w -u10 patch, thanks for attaching that.  I'd like bienvenu to
stamp his r= here, I won't presume to do it for him.

/be

Comment 113

[David :Bienvenu]

Comment on attachment 74586 [details] [diff] [review]
Patch after working on Brendan's suggestions

r=bienvenu, thx, kye.

Comment 114

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 74586 [details] [diff] [review]
Patch after working on Brendan's suggestions

a=dbaron for trunk checkin

Comment 115

[Kai]

Patch has been checked in, resolving as FIXED !

Thanks a lot to everbody who helped.

Comment 116

[Matthias Versen [:Matti]]

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

Comment 117

[R.K.Aa.]

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

Comment 118

[Ninoschka Baca]

Trunk build 2002-04-01: WinMe, Linux RH 7.1, Mac 10.1.3
Verified Fixed.