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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 2

[Asaf Romano (gone)]

I'm going into look at this tomorrow. Should be trivial.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the proper way to implement this would indeed be bug 408991, without that we only have non-performant ways to do that.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

It's unclear to me how often this operation would happen - but yes, synchronous is not ideal...

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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.

[1]: http://people.mozilla.com/~shorlander/panel-experiment-03/panel-experiment.html

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Stealing this since I'm working on the Australis view, I'll appreciate a review from Mano though!

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Nothing more than I was expecting, not too bad.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

the placesCommandHook helper should also be able to use this

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 13

[Justin Dolske [:Dolske]]

P1 on the basis that bookmarks and this widget are a pretty fundamental usecase.

Comment 14

[Asaf Romano (gone)]

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 15

[Asaf Romano (gone)]

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

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

this should address your points

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/493686bf9295

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/493686bf9295