Last Comment Bug 740617 - show the default account name in bold in the Account manager tree of accounts
: show the default account name in bold in the Account manager tree of accounts
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 58713 738810 815283
  Show dependency treegraph
 
Reported: 2012-03-29 14:59 PDT by :aceman
Modified: 2013-02-04 11:53 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.44 KB, patch)
2012-03-30 14:23 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v2 (7.77 KB, patch)
2012-04-10 12:57 PDT, :aceman
no flags Details | Diff | Review
patch v3 (10.66 KB, patch)
2012-04-11 12:14 PDT, :aceman
bwinton: ui‑review-
Details | Diff | Review
patch v4 (11.24 KB, patch)
2012-04-17 13:45 PDT, :aceman
mconley: review+
bwinton: ui‑review+
Details | Diff | Review
patch v5 (11.40 KB, patch)
2012-04-23 11:57 PDT, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2012-03-29 14:59:10 PDT
I don't know of a good way to see which account is the default one (yes, seeing if the "Set as Default" meun item is disabled is a way, but not very good).
I suggest showing the default account name in bold (or other form of highlighting) in the Account manager's tree of accounts. This feature was there at least in Thunderbird 2.0. I could find out why it vanished.

Either it was intentionally removed or it is a regression.
So until a proof of intent is found, I'll mark it a regression.

I'd even throw in a tooltip saying what the bold color means.
Comment 1 :aceman 2012-03-30 14:23:00 PDT
Created attachment 611030 [details] [diff] [review]
patch

Here is the patch. It popped out that all the themes still contained the "treechildren::-moz-tree-cell-text(isDefaultServer-true) { font-weight: bold; }" declarations. But no element was ever marked properties="isDefaultServer-true" so that was the regression.
The real meat of the patch is the markDefaultServer function. All the rest in the 'function at_build()' is just cleanup (services/JS warning).

When another account is made default, the bold highlight moves onto the new server.
Comment 2 :aceman 2012-03-30 14:27:57 PDT
Actually it looks like only the pinstripe (Mac?) theme is missing the style for this:
http://mxr.mozilla.org/comm-central/search?string=isDefaultServer-true&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
So I can add it there if this is approved.
Comment 3 Ian Neal 2012-04-08 14:52:16 PDT
Comment on attachment 611030 [details] [diff] [review]
patch

>+function markDefaultServer(oldDefault) {
As both callers to this function already have MailServices.accounts, could pass defaultAccount as first argument.
function markDefaultServer(defaultAccount, oldDefault) {
>+  let accountTree = document.getElementById("account-tree-children");
>+  let defaultAccount = MailServices.accounts.defaultAccount;
Then this could then be dropped.

>+  let previousDefault = MailServices.accounts.defaultAccount;
>+  MailServices.accounts.defaultAccount = currentAccount;
>+  markDefaultServer(previousDefault);
markDefaultServer(currentAccount, previousDefault);

>+    markDefaultServer();
markDefaultServer(mgr.defaultAccount);

r=me with that address/answered
Comment 4 :aceman 2012-04-10 12:57:18 PDT
Created attachment 613726 [details] [diff] [review]
patch v2

Thanks, good optimization.
Comment 5 :aceman 2012-04-11 12:14:16 PDT
Created attachment 614099 [details] [diff] [review]
patch v3

Sorry I missed that when an account is deleted, the default account can change and needs to be highlighted anew. So this patch updates onDeleteAccount for this.
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-04-17 12:25:20 PDT
Comment on attachment 614099 [details] [diff] [review]
patch v3

Mac doesn't have the "treechildren::-moz-tree-cell-text(isDefaultServer-true)" rule, so it doesn't show up in bold there…

Thanks,
Blake.
Comment 7 :aceman 2012-04-17 13:45:51 PDT
Created attachment 615856 [details] [diff] [review]
patch v4

OK, try this.
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-04-18 15:19:57 PDT
Comment on attachment 615856 [details] [diff] [review]
patch v4

Looks good to me.  We might want to also add a tooltip saying that this is the default account, but even if you didn't want to do that part, this bit gets a ui-r=me.

Thanks,
Blake.
Comment 9 :aceman 2012-04-19 00:00:21 PDT
I wanted a tooltip but I think somebody (Neil?) told me there can't be any on a tree cell element.
Comment 10 neil@parkwaycc.co.uk 2012-04-19 00:35:25 PDT
(In reply to aceman from comment #9)
> I wanted a tooltip but I think somebody (Neil?) told me there can't be any
> on a tree cell element.
Nothing under the <treechildren> exists from the point of view of layout. (This is assuming you're using a DOM-driven tree in the first place, of course). So you can't attach a tooltip to it.

While you can attach a tooltip to the <treechildren>, this will override the default tooltip that shows the whole text for a cropped value.
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2012-04-23 11:13:50 PDT
Comment on attachment 615856 [details] [diff] [review]
patch v4

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

Before I pass judgement on this, I've noticed that you've only altered accountManage.css for pinstripe.  Is it true that we only want this for OSX, and not for Ubuntu or Windows?

::: mailnews/base/prefs/content/AccountManager.js
@@ +433,5 @@
> +function markDefaultServer(newDefault, oldDefault) {
> +  let accountTree = document.getElementById("account-tree-children");
> +  for each (let accountNode in accountTree.childNodes) {
> +    if (newDefault == accountNode._account) {
> +      accountNode.firstChild.firstChild

Let's break this up, like:

accountNode.firstChild
           .firstChild
           .setAttribute("properties", "isDefaultServer-true");

@@ +437,5 @@
> +      accountNode.firstChild.firstChild
> +                            .setAttribute("properties", "isDefaultServer-true");
> +    }
> +    if (oldDefault && oldDefault == accountNode._account) {
> +      accountNode.firstChild.firstChild

Same as above.

@@ +469,5 @@
> +  let prettyName = server.prettyName;
> +
> +  let canDelete = false;
> +  if (Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + type]
> +      .getService(Components.interfaces.nsIMsgProtocolInfo).canDelete)

Let's line up the period before getService with the period between Components and classes.  If that puts us over 80 chars, we can shorted even more by aliasing, like:

let info = Components.classes["@mozilla.org/messenger/protocol/info;1?type" + type]
                     .getService(Components.interfaces.nsIMsgProtocolInfo);

if (info.canDelete)
  canDelete = true;
else
  canDelete = server.canDelete;

@@ +478,5 @@
> +  if (!canDelete)
> +    return;
> +
> +  let bundle = document.getElementById("bundle_prefs");
> +  let confirmRemoveAccount =

Nit - let's try:

let confirmRemoveAccount = bundle.getFormattedString("confirmRemoveAccount",
                                                     [prettyName]);
Comment 12 Mike Conley (:mconley) - (needinfo me!) 2012-04-23 11:17:31 PDT
Ah, I've just noticed that both gnomestripe and qute seem to have that CSS rule covered.
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2012-04-23 11:37:27 PDT
Comment on attachment 615856 [details] [diff] [review]
patch v4

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

Ok, this seems to work as advertised, and I dig the code-cleanup.

With the nits I found fixed, r=me.
Comment 14 :aceman 2012-04-23 11:39:58 PDT
(In reply to Mike Conley (:mconley) from comment #12)
> Ah, I've just noticed that both gnomestripe and qute seem to have that CSS
> rule covered.
Yeah, that is the proof this is a regression :)

(In reply to Mike Conley (:mconley) from comment #11)
> let info = Components.classes["@mozilla.org/messenger/protocol/info;1?type"
> + type]
>                      .getService(Components.interfaces.nsIMsgProtocolInfo);
> 
> if (info.canDelete)
>   canDelete = true;
> else
>   canDelete = server.canDelete;

This was previously there, I just wanted to inline it. I can revert it if it is ugly.
Comment 15 :aceman 2012-04-23 11:57:44 PDT
Created attachment 617579 [details] [diff] [review]
patch v5

Thanks, all fixed. The info-protocol line fix exactly at 80 chars so I didn't need to un-inline it :)
Comment 16 :aceman 2012-04-23 12:00:14 PDT
fix=>fits ;)
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-04-23 16:09:25 PDT
http://hg.mozilla.org/comm-central/rev/da0cbfbfbeb7
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-02-04 11:53:59 PST
Tests checked in.
https://hg.mozilla.org/comm-central/rev/0cf5688a02e9

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