Last Comment Bug 739555 - Having an IM account defined breaks changing the local directory for mail storage
: Having an IM account defined breaks changing the local directory for mail sto...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: 13
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 739908
  Show dependency treegraph
 
Reported: 2012-03-27 02:57 PDT by Mark Banner (:standard8)
Modified: 2012-04-23 12:37 PDT (History)
6 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
fixed


Attachments
patch (2.90 KB, patch)
2012-03-29 11:23 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v2 (2.78 KB, patch)
2012-04-03 12:04 PDT, :aceman
mozilla: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2012-03-27 02:57:46 PDT
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 2012-03-27 09:00:17 PDT
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] [:flo] (PTO until August 29th) 2012-03-27 09:18:01 PDT
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 2012-03-27 10:59:47 PDT
I'll better use the !currentServer.localPath version as it should be more future-proof.
Comment 4 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-03-27 12:09:50 PDT
(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 2012-03-27 12:20:02 PDT
We'll see.
Comment 6 :aceman 2012-03-27 12:54:16 PDT
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 2012-03-27 13:51:59 PDT
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] [:flo] (PTO until August 29th) 2012-03-27 14:57:39 PDT
(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 2012-03-28 00:36:00 PDT
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 2012-03-29 11:23:36 PDT
Created attachment 610611 [details] [diff] [review]
patch
Comment 11 David :Bienvenu 2012-03-30 11:09:27 PDT
(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 2012-03-30 11:32:58 PDT
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] [:flo] (PTO until August 29th) 2012-04-03 07:42:30 PDT
(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 2012-04-03 07:48:48 PDT
(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] [:flo] (PTO until August 29th) 2012-04-03 07:49:49 PDT
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 2012-04-03 08:15:12 PDT
I still do not understand what is good and what is hacky here. But please make it clear in bug 739908.
Comment 17 :aceman 2012-04-03 12:04:12 PDT
Created attachment 611922 [details] [diff] [review]
patch v2

Thanks for the hints Florian, I have fixed them.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-04-05 15:25:36 PDT
http://hg.mozilla.org/comm-central/rev/fdf5dd7d3855

Possible to write a test for this?
Comment 19 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-04-06 02:03:57 PDT
Comment on attachment 611922 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 714733
Comment 20 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-04-13 03:35:06 PDT
http://hg.mozilla.org/releases/comm-aurora/rev/bcbac54d419e

Note You need to log in before you can comment on or make changes to this bug.