Closed
Bug 733687
Opened 12 years ago
Closed 12 years ago
Persist (remember) expanded state of File Bookmark folder tree
Categories
(SeaMonkey :: Bookmarks & History, enhancement)
SeaMonkey
Bookmarks & History
Tracking
(seamonkey2.9 fixed, seamonkey2.10 fixed)
RESOLVED
FIXED
seamonkey2.11
People
(Reporter: InvisibleSmiley, Assigned: neil)
References
Details
Attachments
(2 files, 2 obsolete files)
21.10 KB,
image/png
|
Details | |
7.59 KB,
patch
|
neil
:
review+
Callek
:
approval-comm-aurora+
Callek
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Reporter | ||
Comment 3•12 years ago
|
||
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 neil@parkwaycc.co.uk 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.
Attachment #608494 -
Flags: review?(jh) → review-
Assignee | ||
Comment 4•12 years ago
|
||
(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 neil@parkwaycc.co.uk 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.
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #4) > > (In reply to neil@parkwaycc.co.uk 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.
Assignee | ||
Comment 6•12 years ago
|
||
(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 ;-)
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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)?
Assignee | ||
Comment 8•12 years ago
|
||
This is what I see without (upper) and with (middle and lower) the patch.
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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.
Assignee | ||
Comment 10•12 years ago
|
||
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.
Attachment #608494 -
Attachment is obsolete: true
Attachment #609150 -
Flags: review?(jh)
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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.
Reporter | ||
Comment 13•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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).
Reporter | ||
Comment 14•12 years ago
|
||
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 neil@parkwaycc.co.uk 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.
Attachment #609150 -
Flags: review?(jh) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #609150 -
Attachment is obsolete: true
Attachment #609168 -
Flags: review+
Assignee | ||
Comment 16•12 years ago
|
||
Pushed changeset e238f833f5db to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Target Milestone: --- → seamonkey2.11
Reporter | ||
Comment 17•12 years ago
|
||
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
Attachment #609168 -
Flags: approval-comm-beta?
Attachment #609168 -
Flags: approval-comm-aurora?
Updated•12 years ago
|
Attachment #609168 -
Flags: approval-comm-beta?
Attachment #609168 -
Flags: approval-comm-beta+
Attachment #609168 -
Flags: approval-comm-aurora?
Attachment #609168 -
Flags: approval-comm-aurora+
Reporter | ||
Comment 18•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
status-seamonkey2.10:
--- → fixed
status-seamonkey2.9:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•