tree - no tree navigator

[jrieken]

blocks #64727

the outline uses the tree navigator to select previous/next elements when having searched. this will be a lot more cumbersome using getNode API and navigation APIs

[joaomoreno]

when having searched

Can you elaborate?

[jrieken]

Please look at how the outline uses the navigator API today

[joaomoreno]

This is the only thing I could find: https://github.com/Microsoft/vscode/blob/dbb6f6d807f63429c0eb6913e86885f431b3832b/src/vs/editor/contrib/documentSymbols/outlineTree.ts#L271

We already discussed that you'd get expansion state preservation for free...

So, I'm guessing there's another place...?

[jrieken]

yes, outlinepanel.ts

[joaomoreno]

So, let me elaborate this for myself:

• There are outline.focusDownHighlighted and outline.focusUpHighlighted commands which let the user focus the previous or next item.
• The navigator is used to figure out what's the previous/next element which is an OutlineElement with a score (filter?).
• Once you find a candidate, you focus and reveal it.

There are already focusNext and focusPrevious methods on the tree which could do the hard work for you. I think what's missing for them is a filter function which lets you control whether an element would be suitable for focusing or not. Would that work?

Is the another situation?

[jrieken]

Yes, that would work for that usage in the outline panel. Tho, looking at how getNavigatior is used in breadcrumbs, search, etc it might not be enough

[roblourens]

I am trying to reimplement the navigator (outside of the tree) but I don't think it's possible to tell whether an element is filtered out or not, is it?

[jrieken]

I am trying to reimplement the navigator (outside of the tree) but

🤞 we aren't all doing this. @roblourens do you build your navigator for re-use please? then I can use it for breadcrumbs.

[joaomoreno]

Since I think I haven't laid out my argument against a tree navigator, here it is: it inverts the model-view pattern by exposing a view as a model. Data flow ceases to be one directional and things become harder to reason about.

Because of being against it, I've been trying to be helpful in understanding each use case for it and attempting to provide alternatives, like I've done for outline: #64793 (comment). I'm pretty sure the same could be done for all current usages of the tree navigator, and would be willing to expose tree API to make it so. At the same time, I feel I'm wasting my time in dealing with unhelpful comments like:

Can you elaborate?

Please look at how the outline uses the navigator API today

So I basically stopped caring. I've pushed a TreeNavigator with some tests. It might not be bug free so file an issue if you hit any problems, I'll fix them asap.

You can use it in the ObjectTree by calling .navigate on it.

@roblourens Sorry if you wasted time on this attempting to create a navigator using getNode. It is definitely possible to do one, but it would be pretty cumbersome, just as the old tree navigator was.

[roblourens]

No worries, thanks for making one. I understand your argument and I'm not opposed to getting rid of it in the future. Search is a very heavy user of it (more than I realized originally) and since I'm making lots of changes already, it's much easier to work with the same interface and not have to figure out how to refactor around it.

[roblourens]

@jrieken note that this navigator doesn't update its indexes if the list view is modified, so be careful if you are expanding/collapsing nodes while navigating. Search can work around it by recreating the navigator from a certain element.

[joaomoreno]

Yeah, I forgot the necessary warning: do not use it across tree modifications, it will definitely break.

[jrieken]

Not looping and navigating but missing that fact that the old navigator could start at any element. It seems I now have to fast forward till the anchor element and then go bwd/fwd to the actual element

[roblourens]

I added that to the navigator yesterday @jrieken

[roblourens]

Oh I guess it's only in my branch. I can put it in master.

[roblourens]

73d7889