Make it possible for Library to open looking at a particular folder

RESOLVED FIXED in Firefox 25



Bookmarks & History
5 years ago
5 years ago


(Reporter: mconley, Assigned: mak)


(Blocks: 1 bug)

Firefox 25
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [Australis:M?][Australis:P1])


(1 attachment, 2 obsolete attachments)

The Australis bookmark widget (bug 855805) has a requirement that has us open up a particular folder in the Library window.

The Library doesn't currently allow us to open at a folder like this. We need to make that happen.
Hm, looking at treeView.js and places.js, I think I'm a little out of my depth here.

Marco - any pointers or tips on how to approach this?
Flags: needinfo?(mak77)
I'm going into look at this tomorrow. Should be trivial.
Assignee: mconley → mano

Comment 3

5 years ago
this may be quite unefficient since given an id you have to walk up the hierarchy and each parent request is a synchronous query, so I hope it's only used on very special cases and not often.
Flags: needinfo?(mak77)

Comment 4

5 years ago
the proper way to implement this would indeed be bug 408991, without that we only have non-performant ways to do that.
It's unclear to me how often this operation would happen - but yes, synchronous is not ideal...

Comment 6

5 years ago
the problem is not just that is synchronous, all of bookmarks are, the problem is that the number of queries directly depends on the nesting level of the requested folder. Though we clearly don't have better solutions atm.

What I meant by how often is whether it is just used by the user when he is editing a bookmark and he wants to check where it is... basically what's the use-case?
UX has requested a bookmark star button that, when clicked, displays a list of bookmarks (similar to what the bookmark menu button currently does) in a panel. 

When the user clicks on a folder within that list, that folder is supposed to open in a "subview", which is a little hard to explain, but if you look at this demo[1] and click "History", you'll see the effect that they're looking for.

In the event that the user has folders within that first folder, clicking on that folder within the subview should open that folder in the Library.

So I think you're looking at a maximum depth of 2 folders:  BOOKMARKS_MENU > X > Target.


Comment 8

5 years ago
Well, since you basically know the hierarchy when you are there, you may likely make an util that can receive directly a known hierarchy, then it's easy and no perf loss.
Whiteboard: [Australis:M6] → [Australis:M7]

Comment 9

5 years ago
Stealing this since I'm working on the Australis view, I'll appreciate a review from Mano though!
Assignee: mano → mak77

Comment 10

5 years ago
Created attachment 761997 [details] [diff] [review]
patch v1.0

Nothing more than I was expecting, not too bad.
Attachment #761997 - Flags: review?(mano)

Comment 11

5 years ago
Created attachment 764205 [details] [diff] [review]
patch v1.1

the placesCommandHook helper should also be able to use this
Attachment #761997 - Attachment is obsolete: true
Attachment #761997 - Flags: review?(mano)
Attachment #764205 - Flags: review?(mano)
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
P1 on the basis that bookmarks and this widget are a pretty fundamental usecase.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
Comment on attachment 764205 [details] [diff] [review]
patch v1.1

Shouldn't we ensure select is only called once? As it's written now, each level would force a full reload of the right pane, wouldn't it?
Comment on attachment 764205 [details] [diff] [review]
patch v1.1

> + let hierarchy = [].concat(aLeftPaneRoot);
> + organizer.PlacesOrganizer.selectLeftPaneContainerByHierarchy(hierarchy);

If you want showPlacesOrganizer to accept an hierarchy, the argument should be renamed. If not, then "[aLeftPaneRoot]" is enough.

Alternatively, you could do the magic inside selectLeftPaneContainerByHierarchy.

Feedback+; will r+ once the multiple loads issue is sorted out
Attachment #764205 - Flags: review?(mano) → feedback+

Comment 16

5 years ago
Created attachment 766816 [details] [diff] [review]
patch v1.2

this should address your points
Attachment #764205 - Attachment is obsolete: true
Attachment #766816 - Flags: review?(mano)

Comment 17

5 years ago
Flags: in-testsuite+
Target Milestone: --- → Firefox 25
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.