Last Comment Bug 536248 - Accounts are always expanded by default in account settings window
: Accounts are always expanded by default in account settings window
Status: RESOLVED FIXED
: polish, ux-consistency, ux-control, ux-visual-hierarchy
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 815283
  Show dependency treegraph
 
Reported: 2009-12-21 13:35 PST by mlissner@michaeljaylissner.com
Modified: 2013-02-04 11:53 PST (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.61 KB, patch)
2012-09-22 15:20 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (3.58 KB, patch)
2012-10-01 13:54 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v3 (4.07 KB, patch)
2012-10-20 13:51 PDT, :aceman
mconley: review+
neil: feedback+
Details | Diff | Splinter Review
patch v4 (4.99 KB, patch)
2012-10-30 12:15 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description mlissner@michaeljaylissner.com 2009-12-21 13:35:38 PST
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
Comment 1 Charles 2010-02-15 12:12:47 PST
I agree, this is a bit irritating... just added a vote...
Comment 2 :aceman 2012-09-20 01:00:54 PDT
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?
Comment 3 neil@parkwaycc.co.uk 2012-09-20 01:26:27 PDT
(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.
Comment 4 :aceman 2012-09-20 01:35:26 PDT
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 neil@parkwaycc.co.uk 2012-09-20 02:16:22 PDT
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.
Comment 6 :aceman 2012-09-20 02:48:40 PDT
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.
Comment 7 neil@parkwaycc.co.uk 2012-09-20 02:53:24 PDT
The hint was that Places does it; I just haven't got around to MXRing for the appropriate links yet.
Comment 8 :aceman 2012-09-20 03:04:25 PDT
OK, thanks :)

The attribute is called "persist", not "preserve". And it can also be called by code, so I look at that.
Comment 9 Charles 2012-09-20 04:45:44 PDT
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?
Comment 10 :aceman 2012-09-20 04:49:21 PDT
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.
Comment 11 :aceman 2012-09-20 05:53:52 PDT
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);
...
}
Comment 12 neil@parkwaycc.co.uk 2012-09-20 06:39:11 PDT
> let fpbResource = _rdf.GetResource(AMprefix + account.key);
I'd use document.documentURI + "#" + account.key
Comment 13 :aceman 2012-09-20 06:49:01 PDT
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 Charles 2012-09-21 04:02:11 PDT
(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!
Comment 15 :aceman 2012-09-21 04:13:11 PDT
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 Charles 2012-09-21 10:40:57 PDT
Excellent! and thanks again for taking this on... :)
Comment 17 :aceman 2012-09-22 15:20:33 PDT
Created attachment 663766 [details] [diff] [review]
patch
Comment 18 Blake Winton (:bwinton) (:☕️) 2012-09-24 08:48:37 PDT
Comment on attachment 663766 [details] [diff] [review]
patch

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

Thanks,
Blake.
Comment 19 neil@parkwaycc.co.uk 2012-09-30 14:30:06 PDT
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 20 neil@parkwaycc.co.uk 2012-09-30 14:46:55 PDT
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"? ;-)
Comment 21 :aceman 2012-10-01 13:54:12 PDT
Created attachment 666700 [details] [diff] [review]
patch v2
Comment 22 neil@parkwaycc.co.uk 2012-10-20 13:02:50 PDT
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.
Comment 23 :aceman 2012-10-20 13:51:06 PDT
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.
Comment 24 neil@parkwaycc.co.uk 2012-10-20 17:35:17 PDT
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).
Comment 25 Mike Conley (:mconley) - (Needinfo me!) 2012-10-29 18:33:20 PDT
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
Comment 26 :aceman 2012-10-30 12:15:40 PDT
Created attachment 676703 [details] [diff] [review]
patch v4
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-10-30 14:54:27 PDT
https://hg.mozilla.org/comm-central/rev/1c9e043dbd0a
Comment 28 Ryan VanderMeulen [:RyanVM] 2013-02-04 11:53:52 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.