733687 - Persist (remember) expanded state of File Bookmark folder tree

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

In the File Bookmark dialog, the area below the Folder drop-down and the twisty button to its right should remember their expanded state (visibility).

With bug 605786 fixed, the dialog will remember its position and dimensions (width, height), but the expanded state of the folder tree will not persist, i.e. it will continue to always be collapsed when the dialog opens. This needs to change, and we need to take care that any extra height of the dialog (which is persisted) is used for the folder tree instead of the Description field in case the folder tree is expanded.

Comment 1

[[e-mail]]

Actually it doesn't remember its height, but that's easy to fix.

What's harder to fix is persisting the expanded state - the folder and tags selectors are shared with the bookmarks manager.

Comment 2

[[e-mail]]

Attachment: Proposed patch

Switched from uninitPanel(true) to uninitPanel(false) to actually give us something we can persist. (In fact it was doing unnecessary work!)
Moved the flex="1" on editBookmarkPanelContent to bookmark properties; the other consumers don't want it to flex and were broken by bug 605786 :-(
Added a spacer in the bookmarks manager so that the expander still appears at the bottom of the info box.
Explicitly set the collapsed attribute to "false" so that it can persist.
Added persist on the things that the toggle changes so that they persist.
Added persist on the row heights so that the vertical size persists.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 608494 [details] [diff] [review]
Proposed patch

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

First things first: I found that if the folder tree is expanded and you close the dialog and open it again, the tree area is empty. I guess there's some init missing. I feel this is serious enough to give r- since we must not let this slip and leave for a follow-up. [Obviously, if we don't persist the expanded state like we do now, there is no issue since the init happens if you toggle to expand.] The rest works beautifully, so basically r+ on everything but the above issue if you can answer the below questions.

Also I sometimes see the below in the Error Console. If it's not directly due to the issue described above, please check that it is not introduced through this bug in any other way.

Error: result is null
Source File: chrome://communicator/content/places/tree.xml
Line: 638

(In reply to [e-mail] from comment #2)
> Switched from uninitPanel(true) to uninitPanel(false) to actually give us
> something we can persist. (In fact it was doing unnecessary work!)

Agreed.

> Moved the flex="1" on editBookmarkPanelContent to bookmark properties; the
> other consumers don't want it to flex and were broken by bug 605786 :-(

Ah I see. So that part (only checked the BM) is a regression that needs to be fixed for 2.10 (currently on Aurora). If this bug here doesn't make it to Aurora (which would be a pity), then please file a bug for fixing that regression on Aurora, or provide a patch for that here or somewhere else and request Aurora approval.

Where can the change to placesOverlay.xul be experienced (IOW, how can I check the effect of the change)?

> Added a spacer in the bookmarks manager so that the expander still appears
> at the bottom of the info box.

Hmm, removed the spacer using DOMI and didn't see a difference. Care to check/elaborate please?

> Explicitly set the collapsed attribute to "false" so that it can persist.
> Added persist on the things that the toggle changes so that they persist.
> Added persist on the row heights so that the vertical size persists.

Agreed.

Comment 4

[[e-mail]]

(In reply to Jens Hatlak from comment #3)
> First things first: I found that if the folder tree is expanded and you
> close the dialog and open it again, the tree area is empty. I guess there's
> some init missing. I feel this is serious enough to give r- since we must
> not let this slip and leave for a follow-up. [Obviously, if we don't persist
> the expanded state like we do now, there is no issue since the init happens
> if you toggle to expand.] The rest works beautifully, so basically r+ on
> everything but the above issue if you can answer the below questions.
Oops, I didn't think to check the content of tree area :-(

> Also I sometimes see the below in the Error Console. If it's not directly
> due to the issue described above, please check that it is not introduced
> through this bug in any other way.
> 
> Error: result is null
> Source File: chrome://communicator/content/places/tree.xml
> Line: 638
Indeed, this sounds relevant to the above issue.

> (In reply to [e-mail] from comment #2)
> Where can the change to placesOverlay.xul be experienced (IOW, how can I
> check the effect of the change)?
This is for the star popup.

> > Added a spacer in the bookmarks manager so that the expander still appears
> > at the bottom of the info box.
> Hmm, removed the spacer using DOMI and didn't see a difference. Care to
> check/elaborate please?
When you have everything collapsed, then the area reserved for the editing pane is actually slightly more than necessary, so without the spacer the expander button will rise up slightly.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #4)
> > (In reply to [e-mail] from comment #2)
> > Where can the change to placesOverlay.xul be experienced (IOW, how can I
> > check the effect of the change)?
> This is for the star popup.

Ah OK. Cannot see any difference there, so I guess that's alright.

> > > Added a spacer in the bookmarks manager so that the expander still appears
> > > at the bottom of the info box.
> > Hmm, removed the spacer using DOMI and didn't see a difference. Care to
> > check/elaborate please?
> When you have everything collapsed, then the area reserved for the editing
> pane is actually slightly more than necessary, so without the spacer the
> expander button will rise up slightly.

Oh yes, now I see it. In fact I might have removed the wrong spacer the first time. ;-) So that's OK, too.

Comment 6

[[e-mail]]

(In reply to Jens Hatlak from comment #5)
> (In reply to comment #4)
> > (In reply to Jens Hatlak from comment #3)
> > > Where can the change to placesOverlay.xul be experienced (IOW, how can I
> > > check the effect of the change)?
> > This is for the star popup.
> Ah OK. Cannot see any difference there, so I guess that's alright.
You should the difference if you try to toggle the folder or tags ;-)

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #6)
> You should the difference if you try to toggle the folder or tags ;-)

No, don't see a difference. Are we both talking about the star UI (which I quoted)?

Comment 8

[[e-mail]]

Attachment: Star UI

This is what I see without (upper) and with (middle and lower) the patch.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #8)
> This is what I see without (upper) and with (middle and lower) the patch.

Can't reproduce. Fresh profile, current trunk nightly (i.e. from the FTP server, *not* self-built), Win7 and Linux, Modern and Classic theme. I only see what you have in the lower part of your screen shot.

Anyway, if the problem you see is gone with the patch, I'm fine. Once you landed bug 739049, please confirm your issue is gone with the next nightly by transitioning that bug from RESOLVED to VERIFIED.

Comment 10

[[e-mail]]

Attachment: Fixed patch

I copied the pref usage from editBookmarkOverlay.js, I didn't want to think too hard about where the import of Services should have gone.

I toggle the expanders before listening to them because the listeners don't expect to fire before the dialog has loaded.

Comment 11

[[e-mail]]

Bah, I forgot to include browser-prefs.js in my patch...

+pref("browser.bookmarks.editDialog.expandTags", false);
+pref("browser.bookmarks.editDialog.expandFolders", false);

(In reply to Jens Hatlak from comment #9)
> (In reply to comment #8)
> > This is what I see without (upper) and with (middle and lower) the patch.
> Can't reproduce.
I tried another build and I can't reproduce there either... but it makes sense to remove it for consistency anyway.

Comment 12

[[e-mail]]

Oh, and unless you've been testing with a separate profile, if you used the previous patch then you should collapse the toggles before removing it, otherwise you will have to edit or delete localstore.rdf to fix your profile.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #11)
> Bah, I forgot to include browser-prefs.js in my patch...
> 
> +pref("browser.bookmarks.editDialog.expandTags", false);
> +pref("browser.bookmarks.editDialog.expandFolders", false);

I guess you'll want to put these where the browser.bookmarks.editDialog.firstEditField pref is. In that case, the comment there needs to be adapted slightly (plural form, and while you're there, a full stop).

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 609150 [details] [diff] [review]
Fixed patch

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

Actually I was close to giving you r- when I checked the Bookmarks Manager. Fortunately I remembered to apply the patch for bug 739049 again. ;-)

r=me with the nits addressed (including the prefs, see comments 11 and 13). Thanks very much! Having this fixed in our code is /so/ much better than creating an add-on just for getting the old usability back.

This should bake a bit on trunk, and then I'll happily a+ for Aurora. :-)

(In reply to [e-mail] from comment #10)
> I copied the pref usage from editBookmarkOverlay.js, I didn't want to think
> too hard about where the import of Services should have gone.

No problem for me; this can be revisited once the non-Services pref access count in suite JS code is near zero.

::: suite/common/bookmarks/bm-props.js
@@ +349,5 @@
>          break;
>      }
>  
> +    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                          .getService(Components.interfaces.nsIPrefBranch);

I think this looks a bit lost here, care to move it after the comment block?

@@ +498,5 @@
> +    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                          .getService(Components.interfaces.nsIPrefBranch);
> +    if (!this._element("tagsRow").collapsed)
> +      prefs.setBoolPref("browser.bookmarks.editDialog.expandTags",
> +        !this._element("tagsSelectorRow").collapsed);

Please align the two setBoolPref arguments (here and below), there's enough space to do it.

Comment 15

[[e-mail]]

Attachment: Addressed review comments

Comment 16

[[e-mail]]

Pushed changeset e238f833f5db to comm-central.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 609168 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Regression caused by (bug #):                          Bug 498596
User impact if declined:                               Continued bad UX
Testing completed (on m-c, etc.):                      Works with latest nightly
Risk to taking this patch (and alternatives if risky): None I know
String changes made by this patch:                     None

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 609168 [details] [diff] [review]
Addressed review comments

http://hg.mozilla.org/releases/comm-aurora/rev/5ca85f7ac64e
http://hg.mozilla.org/releases/comm-beta/rev/be6985999277