739555 - Having an IM account defined breaks changing the local directory for mail storage

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Mark Banner (:standard8)]

STR:

1) Have an IM account defined (in my case it was an irc account)
2) Go onto Pop or local folder account properties and change the local directory under server settings

Note that there is an error on the console:

Error: currentServer.localPath is null
Source File: chrome://messenger/content/amUtils.js
Line: 73

and the path doesn't change.

To get this to work, I have to delete the IM account.

Comment 1

[:aceman]

So either IM account implement .localPath (if it has any sense for them) or I can check if currentServer.localPath is null and then skip the .equals() test. In the test the loop is doing (check for duplicate paths) it would be OK to not have a localPath, then it is not a duplicate.

I can do the second version.

Comment 2

[Florian Quèze [:florian]]

I would just add this in the test:
  currentServer.type != "im"

Please don't CC my @goinfre.org address (I use it only for watching some components where I'm not longer really active). And by the way, you don't really need to CC me in bugs filed in the "Instant Messaging" component as I watch this component.

Comment 3

[:aceman]

I'll better use the !currentServer.localPath version as it should be more future-proof.

Comment 4

[Florian Quèze [:florian]]

(In reply to :aceman from comment #3)
> I'll better use the !currentServer.localPath version as it should be more
> future-proof.

Won't it cause a JS strict warning saying currentServer.localPath is undefined?

Comment 5

[:aceman]

We'll see.

Comment 6

[:aceman]

Yes, there is a strict warning.
So I tried ("localPath" in currentServer) but that still returns true even though using it return "currentServer.localPath is null" (error).

Comment 7

[:aceman]

I would not like to put (currentServer.type != "im") everywhere, I'd like to test for the existence of the needed properties so it is future proof (for new account types and also changes in "im" server).

Any new ideas how to test for the usability of "localPath" ?

Comment 8

[Florian Quèze [:florian]]

(In reply to :aceman from comment #7)
> I would not like to put (currentServer.type != "im") everywhere,

As much as I dislike it, I've already added lots of them in lots of different places... so I don't think adding one more would be much worse.

> I'd like to
> test for the existence of the needed properties so it is future proof (for
> new account types and also changes in "im" server).

I think the nsIMsgIncomingServer interface isn't future proof anyway, but a less ugly way to handle this may be to add some more readonly boolean attributes to it, for example a "supportsFolders" attribute. Doing this and replacing all the special cases I've added for type == "im" would be a large patch though, way out of the scope of this bug.

Comment 9

[:aceman]

OK, so I'll add the test for now.

I think the adding of new attributes advertising capabilities of the server would be very nice. I'll file it.

Comment 10

[:aceman]

Attachment: patch

Comment 11

[David :Bienvenu]

(In reply to Florian Quèze from comment #8)

> I think the nsIMsgIncomingServer interface isn't future proof anyway, but a
> less ugly way to handle this may be to add some more readonly boolean
> attributes to it, for example a "supportsFolders" attribute. Doing this and
> replacing all the special cases I've added for type == "im" would be a large
> patch though, way out of the scope of this bug.

We've had this discussion before, but I'd much rather specialize the account settings code to deal with these kind of accounts, than try to teach the whole rest of the code that some incoming servers don't have folders. This would either mean that chat accounts do have folders, or account settings code knows how to display things that aren't incoming servers (or we define a new interface for servers that don't involve messages or folders, and teach account settings about that interface).

Comment 12

[:aceman]

David, I think I don't understand what you mean. Also we filed bug 739908 for a more general solution, maybe you can post your general opinion in there too. Thanks.

Comment 13

[Florian Quèze [:florian]]

(In reply to David :Bienvenu from comment #11)

> We've had this discussion before

Right, we discussed this before I started integrating IM accounts in the mailnews account manager, so I wasn't aware yet that the current approach would force us to add some many |if (server.type != im")| checks.

>, but I'd much rather specialize the account
> settings code to deal with these kind of accounts, than try to teach the
> whole rest of the code that some incoming servers don't have folders. This
> would either mean that chat accounts do have folders, or account settings
> code knows how to display things that aren't incoming servers (or we define
> a new interface for servers that don't involve messages or folders, and
> teach account settings about that interface).

Right, all of these approaches are possible. We could also create a new interface that has all the properties the account manager really needs, and make all accounts implement it. It feels wrong to me that currently the account manager code uses properties of the root folder to create the list of accounts; even for accounts that do have mail folders.

I don't think there's much value in discussing this further for now as I don't think anybody wants to actually start this work right now.

Comment 14

[David :Bienvenu]

(In reply to Florian Quèze from comment #13)

> I don't think there's much value in discussing this further for now as I
> don't think anybody wants to actually start this work right now.

Yeah, my reason for bringing this up was to try to prevent hackier approaches before they started.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 610611 [details] [diff] [review]
patch

>diff --git a/mailnews/base/prefs/content/amUtils.js b/mailnews/base/prefs/content/amUtils.js

>+    let currentServer = allServers.QueryElementAt(i,
>+      Components.interfaces.nsIMsgIncomingServer);

This indent seems a bit strange, but I'm not sure we can do better.

>+    // IM server type does not have a .localPath
>+    // TODO: change this to a proper check of server capabilities
>+    // once bug 739908 is fixed

It seems David disagrees with my comment that led you to file this bug, so I think we shouldn't include this TODO comment in the code.

>+    if ((currentServer.key == gServer.key) || (currentServer.type == "im"))
Except if I got confused with operator priorities in JS again, I think this is enough:
if (currentServer.key == gServer.key || currentServer.type == "im")


aceman, you may get a faster review if you request it from David, as he has already reviewed several similar small patches recently.

Comment 16

[:aceman]

I still do not understand what is good and what is hacky here. But please make it clear in bug 739908.

Comment 17

[:aceman]

Attachment: patch v2

Thanks for the hints Florian, I have fixed them.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/fdf5dd7d3855

Possible to write a test for this?

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 611922 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 714733

Comment 20

[Florian Quèze [:florian]]

http://hg.mozilla.org/releases/comm-aurora/rev/bcbac54d419e