Last Comment Bug 713277 - When the folder tree pane shows no folders and account, the account central pane shows all actions (for all account types) and clicking some of them produces "GetSelectedMsgFolder() is null" error
: When the folder tree pane shows no folders and account, the account central p...
Status: RESOLVED FIXED
: polish
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
: 516285 730564 730565 (view as bug list)
Depends on:
Blocks: 552065 746095 516285 681735 720661
  Show dependency treegraph
 
Reported: 2011-12-23 12:13 PST by :aceman
Modified: 2012-10-14 04:24 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix and cleanup of the file (11.52 KB, patch)
2011-12-23 13:50 PST, :aceman
bwinton: ui‑review+
Details | Diff | Review
proof of concept patch implementing comment 2 (9.87 KB, patch)
2011-12-26 13:52 PST, :aceman
bwinton: feedback+
iann_bugzilla: feedback+
Details | Diff | Review
combined patch with fix for comment 12 (34.17 KB, patch)
2012-01-09 14:17 PST, :aceman
bwinton: ui‑review-
iann_bugzilla: feedback-
Details | Diff | Review
patch, current state of things (36.21 KB, patch)
2012-01-21 09:54 PST, :aceman
iann_bugzilla: feedback-
Details | Diff | Review
patch, fixed nits and refactor trys (38.19 KB, patch)
2012-01-29 06:46 PST, :aceman
bwinton: ui‑review+
iann_bugzilla: feedback+
Details | Diff | Review
complete patch (38.12 KB, patch)
2012-02-20 11:19 PST, :aceman
iann_bugzilla: review+
Details | Diff | Review
complete patch v2 (37.94 KB, patch)
2012-02-22 13:03 PST, :aceman
standard8: review-
Details | Diff | Review
complete patch v3 (38.17 KB, patch)
2012-04-01 12:53 PDT, :aceman
no flags Details | Diff | Review
complete patch v4 (38.58 KB, patch)
2012-04-02 13:14 PDT, :aceman
no flags Details | Diff | Review
complete patch v5 (36.65 KB, patch)
2012-04-17 13:05 PDT, :aceman
standard8: review+
Details | Diff | Review

Description :aceman 2011-12-23 12:13:49 PST
Similar to bug 490749:
The problem happens when using View->Folders->Favorite. When there are no folders to show (no favorites), the left folder tree pane is empty. Restarting TB comes up with the msgAccountCentral.js pane showing about every action available, because it does not know which account is selected (none is). Clicking some of the commands listed throws an exception like:

Error: GetSelectedMsgFolder() is null
Source File: chrome://messenger/content/msgAccountCentral.js
Line: 224

It even exposes functions that are not ready to be shown there (are unimplemented), like JunkSettings.
Comment 1 :aceman 2011-12-23 13:50:27 PST
Created attachment 584120 [details] [diff] [review]
fix and cleanup of the file

The real fix for this bug is in function GetSelectedMsgFolder. If no usable folder/account is selected, choose the default one.

But I made some cleanup in the whole file, like removing trailing whitespace, style alignments and fixing dump messages. There were many spots needing it.

I am not sure why there are those large "try {} catch {}" blocks in ArrangeAccountCentralItems and OnInit. I was running without them and didn't see any exceptions, when viewing the account central page on each of POP3, IMAP, RSS, NNTP account. Are there any situations needing hiding some exceptions? Is it just safety so that nothing (some extension) accidentally leaves the pane empty due to exception?
Comment 2 :aceman 2011-12-26 11:13:17 PST
Actually users can get into this state even after clean install of TB 9, when they do not create a new account immediately. The account central shows all actions (even the secret Junk settings ones) and Error console has tons of errors. My patch probably does not yet fix that, because there wouldn't be any default account I think. There aren't even Local folders created.
Fixing this would need to update all the commands to handle folder==null case, or hide the commands in the account central pane. I think I would be able to do this second case. The only action to be shown would be the "create new account" and there could be some text saying you have not created any accounts yet. Otherwise the pane would be empty. This could affect seamonkey too.

Any ideas?
Comment 3 :aceman 2011-12-26 13:52:39 PST
Created attachment 584347 [details] [diff] [review]
proof of concept patch implementing comment 2

This is just to show what I meant in comment 2. To be applied on top of the 
first patch. Try it with a new profile and cancel all account creation prompts, restart TB.
Comment 4 Blake Winton (:bwinton) (:☕️) 2011-12-27 14:28:08 PST
Comment on attachment 584120 [details] [diff] [review]
fix and cleanup of the file

That makes sense, although it would be nice to catch this case, and let the user know that the reason they aren't seeing anything is likely because they need to switch folder views…

r=me with that change, and I guess r=me without it, too…
Comment 5 Blake Winton (:bwinton) (:☕️) 2011-12-27 14:29:00 PST
(Uh, I mean ui-r=me, not r=me…)
Comment 6 :aceman 2011-12-27 23:49:07 PST
(In reply to Blake Winton [On vacation until Jan 9th] (:bwinton - Thunderbird UX) from comment #4)
> Comment on attachment 584120 [details] [diff] [review]
> fix and cleanup of the file
> 
> That makes sense, although it would be nice to catch this case, and let the
> user know that the reason they aren't seeing anything is likely because they
> need to switch folder views…
> 
> r=me with that change, and I guess r=me without it, too…

I am not sure I understand this. The patch you reviewed does not make user see nothing. It just switches from seeing everything (all actions) to seeing actions relevant to some account that is set as default.

The other patch 584347 makes him see nothing but only in case there are really no accounts existing. In that case, only the "create account" action is shown with an explanation. That patch is not ready for review, it is just to ask whether I should continue working in this direction.
Comment 7 Ian Neal 2011-12-28 02:38:40 PST
Will this fix bug 516285 too?
Comment 8 :aceman 2011-12-28 03:18:12 PST
I am developing this on Linux, I think there is not much system integration so I can't test that. But as the real problem is that while that dialog is open there is no folder selected I think that is exactly what this bug is about to solve. It tries to solve the 'mega confusion' mentioned in bug 516285 comment 1. That is attachment 584120 [details] [diff] [review].
Comment 9 Ian Neal 2011-12-28 03:30:57 PST
Comment on attachment 584347 [details] [diff] [review]
proof of concept patch implementing comment 2

>+++ b/mailnews/base/content/msgAccountCentral.js
>@@ -41,16 +41,18 @@ var nsPrefBranch   = null;
> 
> function OnInit()
> {
>     var title        = null;
>     var titleElement = null;
>     var brandName    = null;
>     var acctType     = null;
>     var acctName     = null;
>+    var serverType   = null;
>+    var protocolInfo = null;
Currently this file is formatted with 4 spaces instead of 2, there are several schools of thought on this - leave as 4 throughout, when introducing new functions use 2 space and finally when changing lines use 2 space. I tend to use the latter but mail owner has the final say.

>     var brandBundle;
>     var messengerBundle;
> 
>     try {
Are we removing everything that could possibly throw?

>         if (!nsPrefBranch) {
>             nsPrefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>                                      .getService()
>                                      .QueryInterface(Components.interfaces.nsIPrefService)
Outside of scope of this bug, but can Services be used for this?

>@@ -71,78 +73,84 @@ function OnInit()
>+
>+        if (selectedServer) {
>+            serverType = selectedServer.type;
>+            if (serverType == "nntp")
>+                acctType = messengerBundle.getString("newsAcctType");
>+            else
>+                acctType = messengerBundle.getString("mailAcctType");
Is an RSS account a mail, news or another account type? Any other account types we should be considering?
Not strictly within bug scope but potentially a nice polish.
>+
>+            // Get the account name
>+            var msgFolder = GetSelectedMsgFolder();
>+            acctName = msgFolder.prettyName;
>+            // Display and collapse items presented to the user based on account type
>+            protocolInfo = Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + serverType]
>+                                             .getService(Components.interfaces.nsIMsgProtocolInfo);
Nit: line up the dots.
>+
>+        }
> 
>         title = messengerBundle.getFormattedString("acctCentralTitleFormat",
>                                                    [brandName,
>                                                     acctType,
>                                                     acctName]);
>+// TODO Ace: if server is not set, the resulting title will look like "Thunderbird (null) - (null)"
> 
>         titleElement.setAttribute("value", title);
Just brandName should be sufficient.

> function ArrangeAccountCentralItems(server, protocolInfo, msgFolder)
> {
>     try {
Again are we removing anything that might throw?

>+        var displayRssHeader = server && (server.type == 'rss');
Nit: I don't think the brackets are needed.

>         // Junk mail settings
Nit: should mention it is currently always set to false in this comment.
>+        var canControlJunkEmail = protocolInfo && protocolInfo.canGetIncomingMessages && protocolInfo.canGetMessages && false;  // && false, until ready for prime time
Nit: remove comment about false and over 80 characters so split over more than one line.

>         // Subscribe to Newsgroups
>-        var canSubscribe = (msgFolder.canSubscribe) && !(protocolInfo.canGetMessages);
>+        var canSubscribe = msgFolder && protocolInfo && (msgFolder.canSubscribe) && !(protocolInfo.canGetMessages);
Nit: brackets were never needed here, again more than 80 characters so split over more than one line.

>         // Junk news settings
Nit: should mention it is currently always set to false in this comment.
>+        var canControlJunkNews = protocolInfo && protocolInfo.canGetIncomingMessages && !(protocolInfo.canGetMessages) && false;  // && false, until ready for prime time
Nit: remove comment about false, brackets were never needed and over 80 characters so split over more than one line.

>         /***** Accounts : Begin *****/
> 
>+        SetItemDisplay("AccountsNeeded", (server == null));
>+
>+        SetItemDisplay("AccountSettings", (server != null));
>+
Nit: extra brackets around arguments should not be needed.

>         // Subscribe to IMAP Folders
>-        var canSubscribeImapFolders = msgFolder.canSubscribe && protocolInfo.canGetMessages;
>+        var canSubscribeImapFolders = msgFolder && protocolInfo && msgFolder.canSubscribe && protocolInfo.canGetMessages;
Nit: again more than 80 characters so split over more than one line.

>         // Offline Settings
>-        var supportsOffline = (server.offlineSupportLevel != 0);
>+        var supportsOffline = server && (server.offlineSupportLevel != 0);
Nit: brackets were never needed here.

>@@ -213,23 +225,26 @@ function CollapseSectionSeparators(separ
> {
>     for (let i=1; i <= 3; i++) {
Nit: not in scope but noticed spacing around i=1 should be i = 1

>+// From the current folder tree, return the selected server (or null)
> function GetSelectedServer()
> {
>-  return GetSelectedMsgFolder().server;
>+  let currentFolder = GetSelectedMsgFolder();
>+  if (currentFolder)
>+      currentFolder = currentFolder.server;
>+  return currentFolder;
I would suggest drop the if statement and write:
return currentFolder ? currentFolder.server : null;

>+++ b/mailnews/base/content/msgAccountCentral.xul
>+      <row id="AccountsNeeded">
>+        <hbox>
>+          <description>You need to set up at least one Mail/News/RSS account to use TB usefuly...</description>
Obviously you will be turning this into an entity.

As far as I can see generally the right direction, so f+
Comment 10 Magnus Melin 2011-12-28 04:20:42 PST
> >+          <description>You need to set up at least one Mail/News/RSS account to use TB usefuly...</description>

"one account" should be enough, besides, it's not officially called an RSS account. Also, "usefully".
Comment 11 :aceman 2011-12-28 05:56:21 PST
Great feedback, thanks Ian.
But I can't answer some of your questions as I have asked them myself earlier, like the "try-catch" blocks. I am adding many checks to not call things that might throw (calling properties of null objects). But I can't say if every spot is fixed by this. Whether it can't throw in some other condition (non-null).

I don't know how to call Services. If you can write the proper command I can include it in the patch.

I can fix the rest of your nits.

I need somebody to decide if I should either:
1. move attachment 584347 [details] [diff] [review] into bug 516285
2. merge the 2 patches into one.

The patches fix 2 problems (+style fixes while at it) that can be separated, but also can be done in one big patch.
Comment 12 Blake Winton (:bwinton) (:☕️) 2011-12-28 08:44:25 PST
Comment on attachment 584347 [details] [diff] [review]
proof of concept patch implementing comment 2

(In reply to :aceman from comment #6)
> > That makes sense, although it would be nice to catch this case, and let the
> > user know that the reason they aren't seeing anything is likely because they
> > need to switch folder views…
> I am not sure I understand this. The patch you reviewed does not make user
> see nothing. It just switches from seeing everything (all actions) to seeing
> actions relevant to some account that is set as default.

Yes, I meant "aren't seeing anything in the folder pane".  (When nothing shows up in the folder pane, it often scares our users into thinking that all their email has been deleted…)  So having the actions, along with some text letting people know why there aren't any folders showing up and how to change that would be a win, I think. 

As for the feedback, I mostly like this, and it's definitely the way to go, I think, but I'm not a huge fan of the "Daily  (null) - (null)", and the missing title on the window and tab are a little odd looking, as seen in http://dl.dropbox.com/u/2301433/Screenshots/DailyNullNull.png

Thanks,
Blake.
Comment 13 :aceman 2011-12-28 12:33:42 PST
The caption with (null)s is only WIP.

I don't think I can show anything in the left folder pane. This bug is only 
about the Account central pane, that is where I am able to show some content.
If users had any accounts with emails, the account central will show the name of the default account and working actions for it. That is the main improvement here. Till now, they got no caption and all actions but none of them worked. That could be quite scary.
If users had no accounts, they would not be scared that they lost anything. They could just worry what to do next. And that is improved here, instead of not working actions they get an explanation.

If you wish to show something some explanation in the folder pane, please file that as a new bug, I don't know how to fix that.
If you mean to show some explanation in the account central pane if no folder is selected then I can do that, just tell me where and some wording.

I am not sure I can do about the window title but I'll check.
Comment 14 Blake Winton (:bwinton) (:☕️) 2011-12-28 13:03:42 PST
(In reply to :aceman from comment #13)
> I don't think I can show anything in the left folder pane. This bug is only 
> about the Account central pane, that is where I am able to show some content.
>
> If you mean to show some explanation in the account central pane if no
> folder is selected then I can do that, just tell me where and some wording.

Showing content in the account central pane would be fine.  Something like:

      You're seeing no accounts
<---- over here, because you have
      switched to Favourite Folders
      mode.  To switch back, click
      something then whatever…

I'm not sure where it would be easy to put that content, but if you wanted to play around with it, and post a screenshot or two, I'll think about what the words should actually be.

Thanks,
Blake.
Comment 15 :aceman 2011-12-28 16:13:07 PST
OK, I can try that. But sometimes there will be something in the folder pane (when user really has some favorites) but I do not know how to check for that.
Only thing I can currently check is whether there exist any accounts and whether any folder is selected. I do not know if any folders are shown but not selected.
Comment 16 :aceman 2012-01-04 15:15:06 PST
I managed to get some pieces together, I query window.parent.gFolderTreeView to get the number of objects (folders/accounts) shown in the left folder pane and even to know which folder mode (all, favorites, etc.) is selected. Two problems:
1. I don't know if that object is available in Seamonkey.
2. I don't know yet how to query if the folder pane is hidden (via menu option). In that case the info should not be shown.
Comment 17 Jim Porter (:squib) 2012-01-04 15:24:26 PST
(In reply to Blake Winton [On vacation until Jan 9th] (:bwinton - Thunderbird UX) from comment #14)
> (In reply to :aceman from comment #13)
> > I don't think I can show anything in the left folder pane. This bug is only 
> > about the Account central pane, that is where I am able to show some content.
> >
> > If you mean to show some explanation in the account central pane if no
> > folder is selected then I can do that, just tell me where and some wording.
> 
> Showing content in the account central pane would be fine.  Something like:
> 
>       You're seeing no accounts
> <---- over here, because you have
>       switched to Favourite Folders
>       mode.  To switch back, click
>       something then whatever…

It might be best to put content like this in a separate page entirely and have it load that page instead when you go to account central. (I say this in part because it would help future-proof this for when Mail Summaries moves into Thunderbird.)
Comment 18 :aceman 2012-01-09 14:17:42 PST
Created attachment 587140 [details] [diff] [review]
combined patch with fix for comment 12

Changes from previous patch:
- I merged both patches together as there are changes all over the place so it would be tedious to maintain separately.
- I changed the default state of all elements to be collapsed and the tests for available features expose them as needed. This is so that if anything happens (like an exception) unwanted elements (actions) are not left exposed (as was the case now with the Junk settings).
- I added another warning to tell about broken gFolderTreeView (as can happen due to bugs like 7073290.
- I temporarily disabled the try {} catch () blocks to see exceptions. I'll add them back once I finish this.

All texts just as examples, will be refined according to comments.

One problem in the code, this does not work: document.getElementById("WarningText").value = statusWarning;

It displays the entity unparsed (as the entity ID). I tried .innerHTML but that was even worse. Any ideas how to make TB interpret the entity and show the text behind it?

Still need answers to comment 16.

I can't create a separate pane.
Comment 19 :aceman 2012-01-10 13:21:57 PST
Is there any way I can force refresh of the account central pane when the folder view mode is changed? Or decide whether it should be even shown? That would make it really nice reacting to current view.

Looks like this function in folderDisplay.js could do it:
  _updateContextDisplay: function FolderDisplayWidget__updateContextDisplay()

But I don't know how to call it. Which object contains it?
Comment 20 Blake Winton (:bwinton) (:☕️) 2012-01-12 12:10:54 PST
Comment on attachment 587140 [details] [diff] [review]
combined patch with fix for comment 12

Yeah, including entities from javascript doesn't work the way you think it does...  http://dl.dropbox.com/u/2301433/Screenshots/FoldersNotSeen.png  I think you need to use a properties file, and look up the actual text.

So, ui-r- for that, but also because I don't think "View Settings for This Account" makes much sense if there's no account, and finally, mostly because I want to check out the next version of this patch.  ;)

Thanks,
Blake.
Comment 21 :aceman 2012-01-12 12:26:21 PST
You see "View Settings for This Account" because it shows your Local folders account so there is an account. The 'foldersNotSeen' entity is shown where there exist accounts, but you do not see any in the folder pane (as you wished in comment 12). If you see this action even when no accounts exist, then it is a bug but you do not prove that on that screenshot.

So let's look up how a properties file is used...
Comment 22 Blake Winton (:bwinton) (:☕️) 2012-01-12 12:35:43 PST
There is no indication of the Local Folders "account" in the folder pane, so it's unclear what the "This Account" I'm Viewing Settings of is.  Perhaps we should change the label in that case?
Comment 23 :aceman 2012-01-12 12:56:25 PST
Well, that is the point of this bug. If you have nothing visible/selected in the folder pane, we still try to select some (default) folder and show its name after "Daily mail - ". It fixes exceptions and other (visible and invisible) problems. But if that is visually misleading, then it is no change from the previous state (I didn't do anything to the title display). Fell free to propose any easy improvements while I am there :)
Comment 24 Ian Neal 2012-01-21 07:40:25 PST
Comment on attachment 587140 [details] [diff] [review]
combined patch with fix for comment 12

Either the status warnings should be in a properties file and set from there or you have all three in the xul and just hide those not needed.
I also get in the error console:
Timestamp: 21/01/12 15:35:55
Warning: reference to undefined property window.parent.gFolderTreeView
Source File: chrome://messenger/content/msgAccountCentral.js
Line: 123
Comment 25 :aceman 2012-01-21 08:00:13 PST
(In reply to Ian Neal from comment #24)
> Either the status warnings should be in a properties file and set from there
> or you have all three in the xul and just hide those not needed.
Thanks, I already solved this.

> I also get in the error console:
> Timestamp: 21/01/12 15:35:55
> Warning: reference to undefined property window.parent.gFolderTreeView
> Source File: chrome://messenger/content/msgAccountCentral.js
> Line: 123
This is what I needed from you (comment 16). Is this existent in Seamonkey? Which object should I use instead?
Comment 26 Ian Neal 2012-01-21 08:11:57 PST
(In reply to :aceman from comment #25)
> (In reply to Ian Neal from comment #24)
> > I also get in the error console:
> > Timestamp: 21/01/12 15:35:55
> > Warning: reference to undefined property window.parent.gFolderTreeView
> > Source File: chrome://messenger/content/msgAccountCentral.js
> > Line: 123
> This is what I needed from you (comment 16). Is this existent in Seamonkey?
> Which object should I use instead?
SeaMonkey only has the one folder view at the moment, so it does not exist and there is nothing to use instead.
Comment 27 :aceman 2012-01-21 08:19:20 PST
Thanks, that should simplify things.
Comment 28 :aceman 2012-01-21 09:54:36 PST
Created attachment 590488 [details] [diff] [review]
patch, current state of things
Comment 29 Ian Neal 2012-01-22 11:24:21 PST
At the moment I am having problems testing the no accounts configured case for SeaMonkey, as the msgAccountCentral code is not being loaded when you cancel out of the Account Wizard. I believe it might be around SeaMonkey not having the equivalent code in openFirstTab in tabmail.xml that calls some code to load the account central. TB calls openFirstTab in mailTabs.js which in turns calls aTab.folderDisplay.makeActive(). SM has similar code to makeActive called showTab() but, as far as I can see, it does not get called from SM's tabmail.xml
Comment 30 Ian Neal 2012-01-24 03:59:34 PST
Comment on attachment 590488 [details] [diff] [review]
patch, current state of things

>+++ b/mailnews/base/content/msgAccountCentral.js

> function OnInit()
> {
>     var title        = null;
>     var titleElement = null;
>     var brandName    = null;
>+    var protocolInfo = null;
>+    var msgFolder    = null;
>     var messengerBundle;
> 
>     try {
>         // Get title element from the document
>         titleElement = document.getElementById("AccountCentralTitle");
You could just inline titleElement when it is used later, or at least move the setting of it to where it is used. Do not need to define before the try statement either.
> 
>         // Get the brand name
>+        brandName = document.getElementById("bundle_brand").getString("brandShortName");
>+
>+        messengerBundle = document.getElementById("bundle_messenger");
Both brandName and messengerBundle are only used within this try, so you could remove the early define for them and just do it here. Rest of defines could be moved to beginning of try statement rather than before the try statement.

> function ArrangeAccountCentralItems(server, protocolInfo, msgFolder)
> {
>     try {
>+
>+        var statusWarning = null;
>+        // If no account/server found, show warning
>+        if (server == null) { statusWarning = "accountsNeeded"; }
Nit: not on a single line please.

>+        if (statusWarning) {
>+            let brandName = document.getElementById("bundle_brand").getString("brandShortName");
Nit: 80 character limit, do the .getString() on the next line.
>+            let messengerBundle = document.getElementById("bundle_messenger");
>+            document.getElementById("WarningText")
>+                    .setAttribute("value", messengerBundle.getFormattedString(statusWarning, [brandName]));
Nit: split maybe to avoid breaking 80 character limit?

>+// From the current folder tree, return the selected folder,
>+// the root folder of default account or null
> function GetSelectedMsgFolder()
> {
>+  let currentFolder = window.parent.GetSelectedMsgFolders()[0];
>+  if (!currentFolder)
>+      currentFolder = window.parent.GetDefaultAccountRootFolder();
>+  return currentFolder;
I think this can be written:
return window.parent.GetSelectedMsgFolders()[0] ||
       window.parent.GetDefaultAccountRootFolder();

> function ReadMessages()
> {
>+    if (!selectedServer) return;
Nit: return on separate line please.

> // Open Subscribe dialog
> function Subscribe()
> {

>+    if (!selectedServer) return;
Nit: return on separate line please.

>+++ b/mailnews/base/content/msgAccountCentral.xul

>+      <separator id="EmailHeader.separator" class="thin" collapsed="true"/>
>+      <row id="ReadMessages" class="acctCentralRow" collapsed="true">
>+        <hbox>
>+          <label class="acctCentralText acctCentralLinkText" value="&readMsgsLink.label;" chromedir="&locale.dir;" onclick="ReadMessages();"/>
Nit: over 80 character limit, so split with one attribute per line, and for most of the rest of the new <label> lines.

>+++ b/suite/locales/en-US/chrome/mailnews/msgAccountCentral.dtd
> 
>+<!ENTITY accountsNeeded.label         "You need to set up at least one account to use &brandShortName; usefully.">
Is this still required?

f- for the moment as too many nits/corrections.

Bug 720661 will sort out displaying account central for SeaMonkey when there are no accounts / no default account.
Comment 31 :aceman 2012-01-24 04:37:14 PST
Thanks for feedback. I just think you want to change things that already passed as being OK, like the GetSelectedMsgFolder function ;)

(In reply to Ian Neal from comment #30)
> >+<!ENTITY accountsNeeded.label         "You need to set up at least one account to use &brandShortName; usefully.">
> Is this still required?
Good catch, seems to be a dupe, it moved to .properties.
Comment 32 Ian Neal 2012-01-24 13:07:03 PST
(In reply to :aceman from comment #31)
> Thanks for feedback. I just think you want to change things that already
> passed as being OK, like the GetSelectedMsgFolder function ;)
> 
Sorry. I didn't catch them first time around, but doesn't mean they should not be fixed.
Comment 33 Blake Winton (:bwinton) (:☕️) 2012-01-24 14:05:54 PST
Comment on attachment 590488 [details] [diff] [review]
patch, current state of things

(I'll give my "f" on the next version, once you've fixed the things in Ian's comments…  ;)
Comment 34 :aceman 2012-01-24 14:25:46 PST
You could have looked at it as the changes will not affect the UI appearance.
Comment 35 Blake Winton (:bwinton) (:☕️) 2012-01-24 14:29:49 PST
Comment on attachment 590488 [details] [diff] [review]
patch, current state of things

Yeah, that's true.  (It's a shame there's no ui-feedback flag…)
Comment 36 :aceman 2012-01-29 06:46:59 PST
Created attachment 592499 [details] [diff] [review]
patch, fixed nits and refactor trys

Adressed all nits.
But I refactored the catching of exceptions in the account central pane. I realized now when all actions are hidden by default, any exception thrown would cause all actions below that point to remain hidden, potentially whole account central. That could be even worse than the original state (all existing actions exposed after exception). So I catch exceptions for every single possible action separately so that it does not influence the next one. If one action throws, it stays hidden, but the next one is evaluated on its own and potentially exposed if needed.

Bwinton, I think the patch nears completion. Could you please start to define the wording and appearance of the 3 new warnings added here? That issue is still open.
Comment 37 Blake Winton (:bwinton) (:☕️) 2012-01-29 07:12:17 PST
(Just to leave a note, I'm kind of swamped with other things for the next few weeks, but I'll get to this as soon as I can.)
Comment 38 Ian Neal 2012-01-29 09:00:38 PST
Comment on attachment 592499 [details] [diff] [review]
patch, fixed nits and refactor trys

>+++ b/mailnews/base/content/msgAccountCentral.js
>+        var protocolInfo = null;
>+        var msgFolder    = null;
Add var title here rather than defining twice in the if/then/else later.

>+    var exception = null;
>+
>+    } catch (e) { exception = e; }
Do we want to just capture the last exception or capture then all?

> // Open Subscribe dialog
> function Subscribe()
> {
>+    if (!selectedServer) return;
Nit: return on new line.

f+ as we're almost there.
Comment 39 :aceman 2012-01-29 09:25:12 PST
(In reply to Ian Neal from comment #38)
> >+    var exception = null;
> >+
> >+    } catch (e) { exception = e; }
> Do we want to just capture the last exception or capture then all?

I don't know. Previously we would only see the first one. Now we'll see the last one dumped. I don't know how good a tool the dump is, who will see the message. It will not be in the Error console, so the user will not see it. 
But if you wish I could try to convert it to an array and dump all the exceptions so it is an improvement too.
Comment 40 Ian Neal 2012-01-29 10:13:28 PST
(In reply to :aceman from comment #39)
> (In reply to Ian Neal from comment #38)
> > >+    var exception = null;
> > >+
> > >+    } catch (e) { exception = e; }
> > Do we want to just capture the last exception or capture then all?
> 
> I don't know. Previously we would only see the first one. Now we'll see the
> last one dumped. I don't know how good a tool the dump is, who will see the
> message. It will not be in the Error console, so the user will not see it. 
Depends if they enable dump to error console or not in prefs.

> But if you wish I could try to convert it to an array and dump all the
> exceptions so it is an improvement too.
Yes, probably easiest then you can just push them on to it.
Comment 41 Blake Winton (:bwinton) (:☕️) 2012-02-13 16:06:09 PST
Comment on attachment 592499 [details] [diff] [review]
patch, fixed nits and refactor trys

So, looking at it, I wonder if we could have some hidden elements which we show at the appropriate times?  That would let us have much nicer styling, instead of using "<-" and "->"…

For the text:
>+folderTreeBroken=<- The folder pane encountered an error. Look into Tools -> Error console and report a bug if there are any errors.

What do you think about using Unicode, and having it be:
⬅ The folder pane encountered an error.  More details may be found in the Error Console, available from the Tools menu.
or
← The folder pane encountered an error.  More details may be found in the Error Console, available from the Tools menu.

(Reports from IRC is that ascii is better than unicode, so perhaps leave it as "<-" at the start, if we're not going to make it an actual styled element…)

>+foldersNotSeen=<- You see no accounts or folders in the list because no folder matches the criteria you selected. To see all your folders use View -> Folders -> All.

<- The view you have chosen has no accounts or folders to show.  To see all your folders use "View > Folders > All".

>+# LOCALIZATION NOTE(accountsNeeded): %1$S is brand (&brandShortName;)
>+accountsNeeded=You need to set up at least one account to use %1$S usefully.

I think the double-use of the word "use" is a little odd.  How about:
%1$S works best with at least one account set up.

And I'll say ui-r=me with either a styled element or the new text.

Thanks,
Blake.
Comment 42 :aceman 2012-02-14 00:18:17 PST
What is a styled element and how would it replace the arrows?
Comment 43 Jim Porter (:squib) 2012-02-14 09:00:50 PST
An image, for instance. In any case, I don't think we need an arrow of any kind. At most, you could say something like "The folder pane (to the left) ...", but I think people can put two and two together (or, if not, they're not going to be able to fix the issue themselves anyway).
Comment 44 :aceman 2012-02-20 11:19:03 PST
Created attachment 598927 [details] [diff] [review]
complete patch

Let's go with the simpler version for now so that the main changes finally land. I can try some more elegant design in a subsequent bug.

There is still this error if there is nothing in the folder pane and "Create account it pressed but the AccountWizard is dismissed":
Error: document.getElementById("account-tree-children") is null
Source File: chrome://messenger/content/AccountManager.js
Line: 168

I couldn't fix that. I think selectServer() should not be called if AW is closed. But that is out of my reach currently. So leave it to another bug.
Comment 45 Ian Neal 2012-02-21 16:12:53 PST
Comment on attachment 598927 [details] [diff] [review]
complete patch


>     try {
>+        if (!nsPrefBranch)
>+            nsPrefBranch = Services.prefs.getBranch(null);
>     }
>     catch (ex) {
>+        dump("Error getting pref service: " + ex + "\n");
>     }
Not sure if anything would cause this to throw anymore and, as it is only used in one place, isn't it best to just inline it?

>+    /***** Accounts : Begin *****/
>+
>+    // Account Settings if a server is found
>+    var canShowAccountSettings = server != null;
>+    SetItemDisplay("AccountSettings", canShowAccountSettings);
>+
>+    // Show New Mail Account Wizard if not prohibited by pref
>+    var canShowCreateAccount = false;
>+    try {
>+        canShowCreateAccount = !nsPrefBranch.prefIsLocked("mail.disable_new_account_addition");
>+    } catch (e) { exceptions.push(e); }

r=me with that answered/addressed
Comment 46 :aceman 2012-02-21 23:31:41 PST
I can move it to the place where it is used. Then even if it would throw, there is another catch and the ShowCreateAccount would stay FALSE. That could be OK. Maybe it does not have much sense to create accounts if prefs are not working.
Comment 47 :aceman 2012-02-22 11:32:25 PST
Ian, I have forgotten to update the Seamonkey string to the new version from Bwinton (I've done it only for TB). Which string do you wish now ('accountsNeeded')?
Comment 48 Ian Neal 2012-02-22 12:52:37 PST
(In reply to :aceman from comment #47)
> Ian, I have forgotten to update the Seamonkey string to the new version from
> Bwinton (I've done it only for TB). Which string do you wish now
> ('accountsNeeded')?

Have SeaMonkey strings the same as Thunderbird ones please.
Comment 49 :aceman 2012-02-22 13:03:14 PST
Created attachment 599743 [details] [diff] [review]
complete patch v2

Ok, thanks.
Comment 50 :aceman 2012-02-28 00:59:03 PST
*** Bug 730564 has been marked as a duplicate of this bug. ***
Comment 51 :aceman 2012-02-28 01:23:37 PST
*** Bug 730565 has been marked as a duplicate of this bug. ***
Comment 52 Mark Banner (:standard8) 2012-03-26 03:40:29 PDT
Comment on attachment 599743 [details] [diff] [review]
complete patch v2

Review of attachment 599743 [details] [diff] [review]:
-----------------------------------------------------------------

I've just been trying to test this and I've been seeing a few issues/things I don't quite understand:

1) Start up TB with a fresh profile. Don't create an account, get the new message about creating an account (fine so far). Then create an account, the folder pane refreshes, but account central doesn't - it still says to create an account.

2) If I go View -> Folders -> Favourites with no folders lists, there is no info displayed as to the view having no folders. If I restart TB, then the view just goes back to all. I think that's unexpected from reading the earlier bug comments.

3) The create an account link links to the new account wizard, I think it should link to the account provisioning in Thunderbird, if the pref for that is enabled (new profile, the user will likely want to set up an account).
Comment 53 :aceman 2012-03-26 06:50:36 PDT
1) is simply the way Account central works. It refreshes only when an account/folder is clicked or on TB start.
I asked if this can be made more dynamic/refresh at more events in comment 19.

2) That should not happen, after restart you still should be in the Favourites view and you should get the message about not seeing any folders. Is corruption of any of the .json files (as handled recently) hitting you? I specifically tested this scenario. I can check again.

3) That is the way it was before, this patch shouldn't touch that. It is not the point of this bug. But if you want to hold back review due to this, I can look into it. But only if it is a matter of exchanging the function being called from that item.
Comment 54 :aceman 2012-04-01 12:53:29 PDT
Created attachment 611309 [details] [diff] [review]
complete patch v3

3) done.

2) can't reproduce. The favourites view sticks fine after restart.
Comment 55 Magnus Melin 2012-04-02 00:12:32 PDT
Comment on attachment 611309 [details] [diff] [review]
complete patch v3

Review of attachment 611309 [details] [diff] [review]:
-----------------------------------------------------------------

Driveby.

::: mailnews/base/content/msgAccountCentral.js
@@ +106,5 @@
> +                statusWarning = "foldersNotSeen";
> +        } catch (ex) {
> +            // for bugs like 707329
> +            statusWarning = "folderTreeBroken";
> +            dump("Error getting gFolderTreeView properties: " + ex + "\n");

Unneeded dump.

@@ +128,3 @@
>      try {
> +        displayRssHeader = server && server.type == 'rss';
> +    } catch (e) { exceptions.push(e); }

How would that exception occur?

@@ +308,4 @@
>  }
>  
>  // Open Inbox for selected server.
> +// If needed, open the twisty and select Inbox.

Here and otherwise, please make function documentation doxygen style when you touch it.

@@ +317,5 @@
>          window.parent.OpenInboxForServer(selectedServer);
>      }
>      catch(ex) {
> +        dump("Error opening Inbox for server: " + ex + "\n");
> +    }

Why isn't this reportError? That's much better esp. when trying to figure out what's wrong with an end user.

@@ +382,1 @@
>      window.parent.MsgJunkMail();

A comment is fine, but a silent failure would just waste everyone's time.
Comment 56 :aceman 2012-04-02 00:32:01 PDT
(In reply to Magnus Melin from comment #55)
> @@ +128,3 @@
> >      try {
> > +        displayRssHeader = server && server.type == 'rss';
> > +    } catch (e) { exceptions.push(e); }
> 
> How would that exception occur?
Just for safety and consistency (other actions are wrapped too). I try to make the account central bulletproof.

> @@ +317,5 @@
> >          window.parent.OpenInboxForServer(selectedServer);
> >      }
> >      catch(ex) {
> > +        dump("Error opening Inbox for server: " + ex + "\n");
> > +    }
> 
> Why isn't this reportError? That's much better esp. when trying to figure
> out what's wrong with an end user.
Will it go to the Error console? That would be great, I'll try it.

> @@ +382,1 @@
> >      window.parent.MsgJunkMail();
> 
> A comment is fine, but a silent failure would just waste everyone's time.

The function seems intentionally undefined yet. I just preserve the status quo and even hide the useless exception that everybody would get (if Junk Mail action is exposed, which this patch should prevent, but anyway).
Comment 57 Magnus Melin 2012-04-02 00:53:21 PDT
(In reply to :aceman from comment #56)
> (In reply to Magnus Melin from comment #55)
> > @@ +128,3 @@
> > >      try {
> > > +        displayRssHeader = server && server.type == 'rss';
> > > +    } catch (e) { exceptions.push(e); }
> > 
> > How would that exception occur?
> Just for safety and consistency (other actions are wrapped too). I try to
> make the account central bulletproof.

Its no safety as it can't (reasonably) throw. Everything can't be in try/catch.

> > @@ +317,5 @@
> > >          window.parent.OpenInboxForServer(selectedServer);
> > >      }
> > >      catch(ex) {
> > > +        dump("Error opening Inbox for server: " + ex + "\n");
> > > +    }
> > 
> > Why isn't this reportError? That's much better esp. when trying to figure
> > out what's wrong with an end user.
> Will it go to the Error console? That would be great, I'll try it.

Yes it will (meant Components.utils.reportError of course)

> > @@ +382,1 @@
> > >      window.parent.MsgJunkMail();
> > 
> > A comment is fine, but a silent failure would just waste everyone's time.
> 
> The function seems intentionally undefined yet. I just preserve the status
> quo and even hide the useless exception that everybody would get (if Junk
> Mail action is exposed, which this patch should prevent, but anyway).

I don't see the point, a valid error in the console doesn't hurt anyone is way better than nothing => someone wasting time trying to figure out why it doesn't do anything.
Comment 58 :aceman 2012-04-02 13:14:08 PDT
Created attachment 611565 [details] [diff] [review]
complete patch v4
Comment 59 :aceman 2012-04-12 07:07:37 PDT
Standard8, can you make a decision here? 
If you still insist on the problem "1)" to be fixed somehow, can I propose splitting it into a separate bug? I'll move all the new messages infrastructure to a follow-up bug that will be solved only when a more dynamic version can be made so that the messages react (Account central refreshes) to new events (new account created, folder view changed).

But the rest of the bug could go in, which is the point of this bug: add more checks so that exceptions either do not happen or are handled better.
Comment 60 Mark Banner (:standard8) 2012-04-17 02:33:55 PDT
(In reply to :aceman from comment #59)
> Standard8, can you make a decision here? 
> If you still insist on the problem "1)" to be fixed somehow, can I propose
> splitting it into a separate bug?

Yes, I think splitting that out to a separate bug would be fine. I'll get to the review soon.
Comment 61 :aceman 2012-04-17 03:02:09 PDT
Thanks, filed bug 746095 for that.
I'll try to attach the reduced patch today.
Comment 62 :aceman 2012-04-17 13:05:16 PDT
Created attachment 615838 [details] [diff] [review]
complete patch v5

The same patch with the info messages removed.
Comment 63 Jim Porter (:squib) 2012-04-17 23:31:55 PDT
This is a little off-topic, but I've been working on a similar page for my Mail Summaries add-on (which may get merged into Thunderbird one of these days). Here's what it looks like:

http://i.imgur.com/5H8tT.png
http://i.imgur.com/m5WyW.png
Comment 64 Magnus Melin 2012-04-17 23:54:13 PDT
Looks really useful!
Comment 65 :aceman 2012-04-18 03:00:49 PDT
Thanks, please post it into bug 746095 where these info messages moved. I didn't want to create such a big different UI just for 1 line of info. Also No account selected is not such a big problem, we choose the default one and show actions for it. But I'd like to know if you can refresh the pane after the user clicks "create new account" or "show all folders" on your pane in the screenshot. But please post it into bug 746095, thanks.
Comment 66 :aceman 2012-04-21 05:50:56 PDT
Standard8, this could still make TB14 :)
Comment 67 Mark Banner (:standard8) 2012-04-23 05:49:12 PDT
Comment on attachment 615838 [details] [diff] [review]
complete patch v5

Review of attachment 615838 [details] [diff] [review]:
-----------------------------------------------------------------

Please make sure we've got follow-up bugs filed for items 1 of comment 52.

::: mail/locales/en-US/chrome/messenger/msgAccountCentral.dtd
@@ +7,2 @@
>  
> +<!ENTITY feedsSectionHdr.label        "Feeds">

Don't we also need this addition for the suite/ version of this file?
Comment 68 :aceman 2012-04-23 05:57:42 PDT
Thanks!

(In reply to Mark Banner (:standard8) from comment #67)
> Comment on attachment 615838 [details] [diff] [review]
> complete patch v5
> 
> Review of attachment 615838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make sure we've got follow-up bugs filed for items 1 of comment 52.

Yes, that is bug 746095.

> ::: mail/locales/en-US/chrome/messenger/msgAccountCentral.dtd
> @@ +7,2 @@
> > +<!ENTITY feedsSectionHdr.label        "Feeds">
> Don't we also need this addition for the suite/ version of this file?

Notice that is not an addition, just shuffling around in that file (see -<!ENTITY feedsSectionHdr.label        "Feeds"> at the end of the diff of that file).
There is addition of +feedsAcctType=Feeds and that is in both
suite/locales/en-US/chrome/mailnews/messenger.properties and
mail/locales/en-US/chrome/messenger/messenger.properties .
Comment 69 Ian Neal 2012-04-23 06:02:41 PDT
(In reply to Mark Banner (:standard8) from comment #67)
> Comment on attachment 615838 [details] [diff] [review]

> ::: mail/locales/en-US/chrome/messenger/msgAccountCentral.dtd
> @@ +7,2 @@
> >  
> > +<!ENTITY feedsSectionHdr.label        "Feeds">
> 
> Don't we also need this addition for the suite/ version of this file?

There wasn't a requirement to do the shuffle / whitespace trimming for suite/ but I'd not complain if it happened.
Comment 70 :aceman 2012-04-23 06:13:44 PDT
Surely, I just do not touch suite if not strictly necessary as I couldn't test it. Yes, it is just shuffling of lines but I could make a typo.
Comment 71 Ryan VanderMeulen [:RyanVM] 2012-04-23 16:09:03 PDT
http://hg.mozilla.org/comm-central/rev/75dc16fb5638
Comment 72 :aceman 2012-10-14 04:24:53 PDT
*** Bug 516285 has been marked as a duplicate of this bug. ***

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