show the default account name in bold in the Account manager tree of accounts

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({regression})

Trunk
Thunderbird 14.0
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #611030 - Flags: ui-review?(bwinton)
Attachment #611030 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 738810

Comment 3

5 years ago
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
Attachment #611030 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 613726 [details] [diff] [review]
patch v2

Thanks, good optimization.
Attachment #611030 - Attachment is obsolete: true
Attachment #611030 - Flags: ui-review?(bwinton)
Attachment #613726 - Flags: ui-review?(bwinton)
Attachment #613726 - Flags: review?(mbanner)
(Assignee)

Comment 5

5 years ago
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.
Attachment #613726 - Attachment is obsolete: true
Attachment #613726 - Flags: ui-review?(bwinton)
Attachment #613726 - Flags: review?(mbanner)
Attachment #614099 - Flags: ui-review?(bwinton)
Attachment #614099 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Blocks: 58713
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.
Attachment #614099 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 7

5 years ago
Created attachment 615856 [details] [diff] [review]
patch v4

OK, try this.
Attachment #614099 - Attachment is obsolete: true
Attachment #614099 - Flags: review?(mbanner)
Attachment #615856 - Flags: ui-review?(bwinton)
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.
Attachment #615856 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 9

5 years ago
I wanted a tooltip but I think somebody (Neil?) told me there can't be any on a tree cell element.
(Assignee)

Updated

5 years ago
Attachment #615856 - Flags: review?(mconley)
(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 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]);
Ah, I've just noticed that both gnomestripe and qute seem to have that CSS rule covered.
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.
Attachment #615856 - Flags: review?(mconley) → review+
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
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 :)
Attachment #615856 - Attachment is obsolete: true
Attachment #617579 - Flags: review+
(Assignee)

Comment 16

5 years ago
fix=>fits ;)
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/da0cbfbfbeb7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Updated

5 years ago
Flags: in-testsuite- → in-testsuite?
(Assignee)

Updated

5 years ago
Blocks: 815283
Tests checked in.
https://hg.mozilla.org/comm-central/rev/0cf5688a02e9
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.