Accounts are always expanded by default in account settings window

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Account Manager
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mlissner@michaeljaylissner.com, Assigned: aceman)

Tracking

(4 keywords)

Trunk
Thunderbird 19.0
polish, ux-consistency, ux-control, ux-visual-hierarchy
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

4.99 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.7pre) Gecko/20091218 Ubuntu/9.10 (karmic) Firefox/3.5.2
Build Identifier: 3.0

For those people that have lots of accounts, seeing them in the collapsed state in the account settings window is nice. Unfortunately though, collapsing them all, then reopening the window results in them being all expanded again. Every time. 

They should remember their state and use that when the window is opened. Should be a pretty easy bug, methinks. 

Reproducible: Always
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Component: General → Account Manager
Ever confirmed: true
OS: Linux → All
QA Contact: general → account-manager
Hardware: x86 → All

Comment 1

7 years ago
I agree, this is a bit irritating... just added a vote...
(Assignee)

Comment 2

5 years ago
As the tree of accounts is always regenerated at opening the Account manager, it is probably hard for the builtin mechanisms (like localstore.rdf and persist attributes) to store the state of the collapsing.
It would be easy to do this via a pref that stores the IDs of collapsed accounts, but that does not look like the correct way. I think this state should be stored in localstore.rdf but I don't yet know how. IanN, Neil, mconley, any idea how to do this?
Keywords: polish, ux-consistency, ux-control, ux-visual-hierarchy
Version: unspecified → Trunk

Comment 3

5 years ago
(In reply to aceman from comment #2)
> It would be easy to do this via a pref that stores the IDs of collapsed
> accounts, but that does not look like the correct way. I think this state
> should be stored in localstore.rdf but I don't yet know how. IanN, Neil,
> mconley, any idea how to do this?

While it is possible to do this using localstore.rdf (Places does this for instance) I don't see why you can't use a pref, particularly as I've recently rewritten nsMsgAccount.cpp to use a pref branch. I don't know whether it would be worth porting the whole of the get/setXXXValue functions from nsIMsgIncomingServer but it should be easy enough to create an attribute.
(Assignee)

Comment 4

5 years ago
I'd like it to be in localstore.rdf because the state or other widgets is stored there. It is not that precious data and can be killed if needed. prefs.js is much more valuable.

I surely not want to do it via a per account pref and add new C++ code just for this.

If a pref is to be used, I'd like it to be a single pref for whole profile containing just the account ids, like "mail.accountmanager.accounts" has:
mail.accountmanager.collapsedAccounts = "account1,account3"

The pref would only be set on OKing the Account Manager and read when the AM is entered, via Services.prefs.getCharPref(). So no need for C++ backend code. Or do you see it being useful in other places too?

Comment 5

5 years ago
I didn't say you couldn't use localstore.rdf, I just wanted to suggest using an attribute on the account. I deliberately didn't suggest having a separate pref because I wasn't sure how well that would deal with creating/deleting accounts.
(Assignee)

Comment 6

5 years ago
The loading code in AM would ignore account IDs in the one pref that are no longer existing.
Accounts created inside AM are fine, the saving code just goes through all collapsed existing accounts and puts them into the one pref.

So could you give me a hint how to do this via localstore.rdf? :)
Would the "preserve" attribute be enough, if the id of the account row element (treeitem) is the same each time the AM is opened? It seems the "open" attribute controls the collapsed state.
Component: Account Manager → Account Manager
Product: Thunderbird → MailNews Core

Comment 7

5 years ago
The hint was that Places does it; I just haven't got around to MXRing for the appropriate links yet.
(Assignee)

Comment 8

5 years ago
OK, thanks :)

The attribute is called "persist", not "preserve". And it can also be called by code, so I look at that.

Comment 9

5 years ago
Why is a pref needed at all?

It seems to me the way this should work is simple...

1. If Account Settings is opened via 'Tools > Account Settings', open with all accounts collapsed

2. If Account Settings is opened via the 'View Settings for this account' link on  the accounts main info page, open to that Accounts settings expanded, with all others collapsed

Why complicate things with a pref?
(Assignee)

Comment 10

5 years ago
Because the original request was for the user to choose which accounts are collapsed and preserve the state.

In that way you could collapse some inactive accounts permanently to not be in the way.
(Assignee)

Comment 11

5 years ago
Neil, I got it working via localStore.rdf. 
I could not find any ready made function for retrieving the persisted state (opposite of document.persist), so I can use code from bug 712395, and something like this works now (just added code parts shown):

var gAccountTree = {
...
  _getPersist: function MailMigrator__getPersist(aSource, aProperty) {
    // The code for this was ported from
    // mozilla/browser/components/nsBrowserGlue.js.

    let target = this._dataSource.GetTarget(aSource, aProperty, true);
    if (target instanceof Components.interfaces.nsIRDFLiteral)
      return target.Value;
    return null;
  },
  
  _dataSource: null,

  _build: function at_build() {
....
        treeitem.setAttribute("id", account.key);
        treeitem.setAttribute("persist", "open");

var AMprefix = "chrome://messenger/content/AccountManager.xul#";
var _rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);
this._dataSource = _rdf.GetDataSource("rdf:local-store");
let fpbResource = _rdf.GetResource(AMprefix + account.key);
let collapsedResource = _rdf.GetResource("open");
let collapsed = this._getPersist(fpbResource, collapsedResource);

        treeitem.setAttribute("open", collapsed);
...
}
> let fpbResource = _rdf.GetResource(AMprefix + account.key);
I'd use document.documentURI + "#" + account.key
(Assignee)

Comment 13

5 years ago
Thanks, this was a crude proof of concept. I'll optimize it more like you suggest. Would this solution be acceptable to you?
Only the loading of the value must be coded like this, the saving is done automatically via the persist attribute.

Comment 14

5 years ago
(In reply to :aceman from comment #10)
> Because the original request was for the user to choose which accounts are
> collapsed and preserve the state.
> 
> In that way you could collapse some inactive accounts permanently to not be
> in the way.

Sorry, I thought what was being discussed was a UI pref, like a checkbox saying 'Always expand this account'... I see now it is just a pref for remembering the state for each account...

No worries, just glad this is being worked on... thanks aceman!
(Assignee)

Comment 15

5 years ago
Yeah, no pref in the UI:)
It would be a hidden about:config pref. But I still want to avoid a pref, just use the generic persiting of UI state in localstore.rdf, that is used in all other places. And it seems to work, I attach a real patch soon.

But I will actually implement your proposal from comment 9, point 2. Because we need to ensure that when a specific account/pane is selected via the selectServer() function it will get expanded automatically. And I think that function is called e.g. when 'View Settings for this account' is clicked.

In your point 1. the accounts will be collapsed as the user left them before.

Comment 16

5 years ago
Excellent! and thanks again for taking this on... :)
(Assignee)

Comment 17

5 years ago
Created attachment 663766 [details] [diff] [review]
patch
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #663766 - Flags: ui-review?(bwinton)
Comment on attachment 663766 [details] [diff] [review]
patch

Yes, I love this.  ui-r=me!

Thanks,
Blake.
Attachment #663766 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Updated

5 years ago
Attachment #663766 - Flags: review?(neil)
Useless observation: had this still been an RDF tree, persisting would have been trivially accomplished by simply removing the open="true" attribute that had been added to override the automatic persistence. (Of course this change doesn't take comment #9 into account.)
Comment on attachment 663766 [details] [diff] [review]
patch

Unfortunately all of my builds have the DBLI code which is messing things up, so I'm not sure whether I am seeing issues with your patch or not :-(

>+  _getAccountOpenState: function _getAccountOpenState(aAccountKey) {
File style is to call this function at_getAccountOpenState

>+    let collapsedAttribute = this._rdf.GetResource("open");
Hmm, collapsedAttribute = "open"? ;-)
(Assignee)

Comment 21

5 years ago
Created attachment 666700 [details] [diff] [review]
patch v2
Attachment #663766 - Attachment is obsolete: true
Attachment #663766 - Flags: review?(neil)
Attachment #666700 - Flags: review?(neil)
Comment on attachment 666700 [details] [diff] [review]
patch v2

Sorry for the delay.

>+    let _dataSource = this._rdf.GetDataSource("rdf:local-store");
>+
>+    // Retrieve the persisted value from localstore.rdf.
>+    // It is stored under the URI of the current document and ID of the XUL element.
>+    let resource = this._rdf.GetResource(document.documentURI + "#" + aAccountKey);
>+    let openAttribute = this._rdf.GetResource("open");
[Apart from inlining the function, I can't see an obvious way to stop getting the data source and attribute each time...]

>+        treeitem.setAttribute("persist", "open");
>+        treeitem.setAttribute("open", this._getAccountOpenState(account.key));
Would you mind setting open before persist? This way we don't touch localstore for people who leave all of their accounts open. r=me with that fixed.
Attachment #666700 - Flags: review?(neil) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 673617 [details] [diff] [review]
patch v3

Thanks, so I have added vars for the datasource and openAttribute values so they do not need to be always retrieved.
Attachment #666700 - Attachment is obsolete: true
Attachment #673617 - Flags: review?(mconley)
Attachment #673617 - Flags: feedback?(neil)
Comment on attachment 673617 [details] [diff] [review]
patch v3

>+        // Let the localstore.rdf automatically save the 'open' state of the
>+        // account when the dialog is closed.
Actually I believe it gets saved when the attribute is changed (i.e. if the row is ever toggled).
Attachment #673617 - Flags: feedback?(neil) → feedback+
Comment on attachment 673617 [details] [diff] [review]
patch v3

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

Looks good - thanks!

::: mailnews/base/prefs/content/AccountManager.js
@@ +1257,5 @@
>    onServerChanged: function at_onServerChanged(aServer) {},
>  
> +  _rdf: Components.classes["@mozilla.org/rdf/rdf-service;1"]
> +                  .getService(Components.interfaces.nsIRDFService),
> +  _rdf_dataSource: null,

I don't really like the mixing of potholes with camelcase. We should stick to one - so these should be

_rdfDataSource and _rdfOpenAttribute
Attachment #673617 - Flags: review?(mconley) → review+
(Assignee)

Comment 26

5 years ago
Created attachment 676703 [details] [diff] [review]
patch v4
Attachment #673617 - Attachment is obsolete: true
Attachment #676703 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1c9e043dbd0a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.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.