Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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
5 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.