lsp-treemacs-symbols: remember the folding of nodes

[nbfalcon]

lsp-treemacs-symbols allows folding and unfolding nodes. However, after the next refresh cycle, all folding information is lost and the modified nodes go back to their normal, unfolded states. That is especially an issue for dap-mode, where losing variable folding is annoying.

This could be implemented as follows:

• A minor mode could be added, that, for treemacs buffers, records the paths of nodes where the user executed treemacs-TAB-COMMAND into a hash table
• After a treemacs tree is constructed, it is recursively walked and nodes whose paths were stored are expanded. Not-needed and lazily-generated :children would be ignored.

Garbage-collecting the hash-table could be done by recording the last usage cycle for each mapped node, dropping all nodes that were unused for some (configurable) number of cycles. This has the advantage that, for example, deleting a class and undoing that would restore its fold state. When moving a class into a new namespace though, that wouldn't be the case anymore.

I'd like to hear feedback on my approach, and to then implement it in upstream treemacs.

[yyoncho]

A minor mode could be added, that, for treemacs buffers, records the paths of nodes where the user executed treemacs-TAB-COMMAND into a hash table

IMHO it will be simpler to store the state on context switch and restore it when we go back. E. g. the symbols will keep the data in buffer local. I think that I even had a POC which was persisting the string content + text properties of the buffer which is kind of cool way to persist state in emacs.

I would rather implement that in the upstream, @Alexander-Miller any thoughts? Do you think it will be easy to implement a function like treemacs-get-state-snapshot and treemacs-apply-state-snapshot?

[nbfalcon]

@yyoncho but isn't the issue here that we redraw the entire tree every time the symbol-list is refreshed? The minor-mode is only to modify treemacs-RET-action for this to work. The folding data would be stored in a buffer-local variable.

[yyoncho]

@yyoncho but isn't the issue here that we redraw the entire tree every time the symbol-list is refreshed?

Yes, and if we have a mechanism which is "go to this state" the issue will be solved. Implementing ...

After a treemacs tree is constructed, it is recursively walked and nodes whose paths were stored are expanded. Not-needed and lazily-generated :children would be ignored.

... won't be that simple since there are async trees which most likely cause blinking. On the other side if we just restore a snapshot it won't have that issue and then refresh it we won't have that issue.

[nbfalcon]

I'll admit that I haven't looked much at treemacs' API yet, but couldn't we just modify whatever function it uses for async-rendering to take folding into account?

I may not have understood your snapshot idea entirely, but I'll assume you mean to just save and restore the DOM? Wouldn't the issue with that approach be that we would show outdated data?

[yyoncho]

I'll admit that I haven't looked much at treemacs' API yet, but couldn't we just modify whatever function it uses for async-rendering to take folding into account?

I think ATM you cannot tell treemacs from the extension api to do render that node open and you have to do it using the iteration approach. Async is implemented in lsp-treemacs.

Wouldn't the issue with that approach be that we would show outdated data?

Yes. And then we call refresh on that tree and it will refresh the nodes where needed smartly. Treemacs keeps in its state which node is expanded and which isn't expanded. E. g. if you expand a node and you collapse its parent and then expand the parent the child will be expanded. IMO it does not make sense to duplicate that state - if you are implementing that in the upstream, you won't introduce one more place to keep the treemacs node state but you will use the existing one, right?

[yyoncho]

As a side note, the locals nodes in dap-mode won't be closed up if the breakpoint is hit within 0.2 ms.

[nbfalcon]

There would necessary be some duplication, even though I want to implement this upstream. The hash table would only store information about the user's interaction with the tree's folding, while the DOM would represents the current state. The issue with reusing the DOM nodes' folding states is that it is often destroyed and rebuilt from scratch (symbol refresh, stopped events, ...).

[yyoncho]

while the DOM would represents the current state.

current state = user's interaction with the tree's folding

even though I want to implement this upstream.

If you implement it in upstream you won't be able to follow the approach you suggested since upstream does not know about async.

[nbfalcon]

current state = user's interaction with the tree's folding

Yes. However, a DOM is not a good data-structure for this sort of thing, as we would have find every node in the previous snapshot to get its desired fold state, which would be O(nxm) for each level. With a buffer-local, the DOM would have to walked only once to check if some path was folded or unfolded.

Also, my approach does not depend on any treemacs async facities; it only needs to integrate with them.

[yyoncho]

I am not sure even what you are talking about. The snapshot will set the old dom and then restore the tree as described in the dom (or even directly restore the string content in the buffer). It carries all the information that you need, no need for cache invalidation, additional variables, or whatever. The complexity is O(n) with N being the number of open(visible) nodes.

[nbfalcon]

Sorry for being unclear; the problem with snapshots is that they don't reflect the current state of whatever treemacs should be displaying, which is undesirable. We cannot sacrifice live-data for better folding (old values in dap-variables vs folding).

My approach was as follows: if a user runs the fold action, the PATH to the node is stored in a hash table, along with its new folding state. So if I have a folded function "encode" in namespace "foo", and I press TAB on it,

(puthash (list "foo" "encode") ; DOM path
               t ; now folded
               treemacs--fold-states)

would get executed (pseudocode, there is no implementation yet). Then, after refreshing, the path of each node would get looked up in treemacs--fold-states, which would yield results for some them. Those would then be unfolded. The DOM can be discard at any time, but treemacs--fold-states (or whatever it will be called) cannot.

I hope that clears things up.

[yyoncho]

the problem with snapshots is that they don't reflect the current state

That is what the snapshots will be all about... The method treemacs-get-state-snapshot will return

1. DOM data
2. Selected item
3. Position in the buffer
4. Expanded items
5. Cursor position
6. ?

Then treemacs-get-apply-snapshot will restore all this. Let me know if you need more clarification.

[nbfalcon]

1. Why store the DOM data? Don't we only care about the fold states of user-modified nodes?
2. Unlike with my approach, if a node gets deleted and added again (delete a class and undo), its fold state would be lost, or am I missing something here?

[yyoncho]

1. Why store the DOM data? Don't we only care about the fold states of user-modified nodes?

I already explained that - this solution works. Your proposal does not work with async nodes. Imagine that you have several nested nodes and each one of them takes 0.5 seconds to load. Then when you switch back to that context it will take 1.5 seconds to show the view and it will show Loading... for each node and it will provide poor experience. This is not how the context switch/restore should work.

2. Unlike with my approach, if a node gets deleted and added again (delete a class and undo), its fold state would be lost, or am I missing something here?

It works like that in vscode. I doubt that anyone will complain about that behaviour. In case we want that - then it should be implemented in treemacs itself.

[nbfalcon]

1. Actually, that wouldn't be the case at all; in an async node, when some branch is ready, it would just have to walked once to set the fold states, and then drawn. The entire tree wouldn't need to be rewalked, only the new branch.

2. I think having Emacs be as good of an editor as possible is a good thing, so even if VSCode doesn't have something, we should still pursue it if it makes sense.

[yyoncho]

1. Actually, that wouldn't be the case at all; in an async node, when some branch is ready, it would just have to walked once to set the fold states, and then drawn. The entire tree wouldn't need to be rewalked, only the new branch.

I am not saying that the entire tree has to be rewalked. Please avoid putting words in my mouth. I am saying that it will take 1.5 seconds to render that tree. I will ask you simple question: which behaviour will be better:

a) Restore the position/selection/buffer position right away and refresh without the user noticing and without blinking.
b) Wait N seconds the tree to be rerendered, to restore the selection and restore the buffer position.

[Alexander-Miller]

Pretty sure the thing you are asking for is already default behavior. Normal file nodes definitely remember when their children were opened. The behavior you describe is either a bug in the extension api or caused by a strange way of running a refresh.

I am also pretty sure that there's already a ticket for saving and restoring a dom state, but as I understood it it is aimed at restoring an entire session from scratch, not just surviving a refresh. Can't find it now, but I see your ticket for rendering a node that's instantly expanded. So all that stuff is on the agenda.

If this is something that needs action on the extension api on my end I would rather get through Alexander-Miller/treemacs#677 first. I will push the first prototype later today. It is far from ready for productive use, but it will allow you to get a feel for the new api. That means now is also the time to write your wish list because I want to get it right on the first try, without bolting on extras and deprecating arguments after the fact, as with my first attempt.

[nbfalcon]

@Alexander-Miller I think you have misunderstood; what we are doing here is rebuilding the entire DOM entirely from scratch each time (because we don't know what changed, only the new state, for example for dap's variables view, ...). However, this means that we lose folding information, which we want to keep. So if I am debugging something with dap-mode, expand some array, and then continue to the next breakpoint, I want that array to still be expanded. However, as I've said, we are rebuilding the entire locals view DOM from scratch, so that information is lost and needs to be remembered some other way.

[yyoncho]

Pretty sure the thing you are asking for is already default behavior. Normal file nodes definitely remember when their children were opened. The behavior you describe is either a bug in the extension api or caused by a strange way of running a refresh.

All this works. The issue is: we show symbols, for buffer A and then we go to buffer B, and when going back to A won't restore the state of the buffer (pretty much the feature request which I have forgotten).

[Alexander-Miller]

Ok, now I get it. Yeah, that is an entirely different animal. Proper dom saving is a long ways off, so I suggest what I call buffer-cycling as a solution:

Don't display the symbols in just a single buffer, instead keep a cache of some 5 or 10 of the last used symbol buffers. So if you need to show symbols for a new code buffer first check the if there's already a symbol buffer for it in the cache. If yes display it in the symbols-view window and use treemacs' refresh mechanism.

That won't fully solve the issue, but it can make it much less prevalent.

[yyoncho]

I was thinking about the same. It will definitely work for dap buffers since they have only two "versions". We should measure what is the memory usage of the average symbols buffer because doubling the buffers might be problematic.