Treemacs: Towards a polymorphic extensions implementation

[Alexander-Miller]

The extension api is due for an overhaul. It contains many obsolete atavisms that complicate the implementation and by now can be safely deleted. The biggest problem however is the use of macros to generate many specialized functions which greatly complicates development, debugging and introspection.

Fortunately the problem space is ideal for an OO-based approach - all the different node types are all just different implementations of the same behavior, declaring their icons, faces query-functions etc. Tracking state is not necessary either, so using static functions will be well enough.

I see 2 possible downsides that must be considered:

Performance: eieio method calls are about 4 times slower than normal function calls. Should this have an actual impact on performance a workaround can be applied, albeit at the cost of slightly more complex architecture: the node objects methods, instead of directly producing values like their faces, will need to instead output a function. That will eliminate performace concerns, since using funcall is just as fast as a normal function call.

Backward compatibility: Most of the rebuild should focus on interior implementation details, but if breakage does turn out to be unavoidable it's probably best to publish the new implementation in a new module with a different name (and delete the old one some time down the road), so packages using the extension api can upgrade whenever they are ready.

[Alexander-Miller]

ping @yyoncho As a heavy user of extensions I am sure you have plenty of ideas how things can be improved.

[yyoncho]

I personally would vote for a data-driven solution in which the model is readable. We have created a wrapper providing this functionality. The model is just a plist with :icon/:label/:children/:children-async, and related keys. This probably will lead to slower access to the fields, but this is not an issue for us.

[Alexander-Miller]

Would that plist have to be descriptive or prescriptive? If it's the former I can produce one as a side effect, or even build something interactive that pretty prints all available info.

[yyoncho]

I am not sure I understand the difference between the two in this context, here it a runnable(after you load lsp-treemacs) example:

(display-buffer
 (lsp-treemacs-render
  '((:key "foo"
          :label "Root"
          :icon dir-open
          :children-async (lambda (_ callback)
                            (run-with-idle-timer
                             2
                             nil
                             (lambda (callback)
                               (funcall 
                                callback 
                                '((:key "foo" 
                                        :label "Async" :icon dir-open
                                        :children ((:key "foo"
                                                         :label "Sync"
                                                         :icon dir-open
                                                         :ret-action ignore
                                                         :actions (["Right Click selected" ignore])))))))
                             callback))
          :ret-action ignore
          :actions (["Right Click selected" ignore])))
  "Demo"
  nil
  nil
  '(["Right Click Nothing selected" ignore])))

[Alexander-Miller]

I mean for the prescriptive version you would be able to put the plist into some variable, and changing that variable would then change the behavior it represents. So you could change things programmatically, without any need re-eval.

But your example is also very interesting. It looks like you have defined a single generic node type whose exact behavior you specify with the plist. Is that right?

I'll have to think about how to integrate that into my existing plans. At the very least I would want to natively support async query-functions.

[yyoncho]

I mean for the prescriptive version you would be able to put the plist into some variable, and changing that variable would then change the behavior it represents. So you could change things programmatically, without any need re-eval.

I do that something like that in lsp-treemacs-render in current implementation - but it is wrapped in the lsp-treemacs-render.

But your example is also very interesting. It looks like you have defined a single _generic_ node type whose exact behavior you specify with the plist. Is that right?

Yes.

The async part is challenging, especially for the functionality like expand all.

[Alexander-Miller]

Small update: things are going very well so far, though there's plenty of work to do yet - there are many things I can do better and, more importantly, simpler.

In the core I went with a lambda-producing struct approach now. It's no plist, but it will give you a lot more introspective power. You write this:

(treemacs-define-expandable-node-type buffer-group
  :closed-icon "x "
  :open-icon "o "
  :label (symbol-name item)
  :face 'font-lock-variable-name-face
  :key item
  :children (treemacs--buffers-by-mode (treemacs-button-get node :major-mode))
  :child-type 'buffer-leaf
  :more-properties `(:major-mode ,item))

and get something like this:

(defconst treemacs-buffer-group-extension-instance
  (treemacs-extension->create!
   :label (lambda (&optional item) "" (ignore item) (symbol-name item))
   :key (lambda (&optional item) "" (ignore item) item)
   :open-icon (lambda (&optional item) "" (ignore item) "o ")
   :closed-icon (lambda (&optional item) "" (ignore item) "x ")
   :children (lambda (&optional node) "" (ignore node) (treemacs--buffers-by-mode (treemacs-button-get node :major-mode)))
   :face (lambda (&optional item) "" (ignore item) 'font-lock-variable-name-face)
   :more-properties (lambda (item) "" (ignore item) `(:major-mode ,item))
   :child-type (lambda nil "" (symbol-value 'treemacs-buffer-leaf-extension-instance))
   :open-state (lambda nil "" 'treemacs-buffer-group-open-state)
   :closed-state (lambda nil "" 'treemacs-buffer-group-closed-state)))

[Alexander-Miller]

It's been quiet here for a long time, so I'd like to show a small progress update:

It's just a test implementation for now, but I am not far off from having something I can upload for you to test.

[yyoncho]

It's no plist, but it will give you a lot more introspective power. You write this:

Can you elaborate what is the idea behind having a definition for each node type? We have line 15+ view with probably 50 different types and for me having to define a nodetype for each entry will be a lot of work and it is not clear what will be the benefit. Also, with plists as model we can create different representations and it won't be coupled to treemacs. E. g. I could create dired like flat browsing with inserting and collapsing the items.

I am interested in support of open/expanded/no-children state being expressed in node itself, not as part of node definition due to the fact that there is no open expanded icons for all types and we prefix the icons with a symbol.

[Alexander-Miller]

Can you elaborate what is the idea behind having a definition for each node type?

(Some small degree of) static typing and separation of concerns. When you put everything into one type you'd have to constantly dispatch on the actual state for every action - if we open a list of classes go collect the classes, if we open a class go collect its members etc, if we open a method go collect its local variables, and so on for other properties like icons and faces.

With the current approach these things would be split into separate node types. I figured that'd be the cleaner approach. It does scale well enough for the kind of extensions I am considering - a buffer list (1 root type, 1 buffer group type, 1 buffer type), a mu4e sidebar (1 root mail account type, 1 mail directory leaf type), a docker sidebar (1 root type for images/containers, 1 leaf type for their instances).

At any rate removing boiler plate necessities is part of my agenda as well, I just haven't been advertising and optimizing for it just yet. My intention is to eventually whip the whole api into a shape that requires only a single node definition, or maybe 2 to deal with the necessary entry-point house keeping.

I am interested in support of open/expanded/no-children state being expressed in node itself, not as part of node definition

Not sure what you mean by that. Can you make an example for what that'd look like?

[Alexander-Miller]

@yyoncho There's enough material now to express different node types with a single definition:

Here's an example of defining both the buffer groupings and the buffer leaves all at once (I'll be calling this the "mono-typed" approach now). Is this what you had in mind? Myself I think the dispatching is a bit cumbersome, but you could put that stuff away in functions once it grows big enough. Other than that I'm not sure what can be done to improve this api.

[yyoncho]

Sorry, I somehow missed the previous reply. I think that your screenshot is close to what I am looking for.

I am interested in support of open/expanded/no-children state being expressed in node itself, not as part of node definition

Not sure what you mean by that. Can you make an example for what that'd look like?

I meant that in the previous api you have to use the proper define method to indicate if the node is leaf or not.

As a site note, in lsp-treemacs we have an icon and prefix for the icon which indicates the state of the node. There is prefix icon for open/closed and empty for no-children. Most of the icon collections do not provide 2 versions of the same icon - thus we need the indicator.

Here it is a working example of what we have right now(it is runnable).

(display-buffer 
 (lsp-treemacs-render
  (->> (buffer-list)
       (-group-by (-partial #'buffer-local-value 'major-mode))
       (-map (-lambda ((mode . buffers))
               (list :key mode
                     :label (symbol-name mode)
                     :icon 'major-mode
                     :children (-map (lambda (buffer) 
                                       (list :key buffer
                                             :label (buffer-name buffer)
                                             :icon 'buffer))
                                     buffers)))))
  "buffer"
  nil))

[yyoncho]

(off topic) WDYT about adding the ability to show things at the end of the line. It could be used as an alternative to the git statuses represented as different faces. E. g. instead of changing the color you could put a circle at the end of the line(this is what vscode does). The approach with different colors works but IMO it is more "noisy". I think that there were text attributes to pin it to the fringe.

[Alexander-Miller]

I meant that in the previous api you have to use the proper define method to indicate if the node is leaf or not.

Still the case for the new api. Except now it is optional. Using a define-leaf is just a short cut to the usual define-expandable-node, but you only need 1 icon and don't have to provide a way to query children.

As a site note, in lsp-treemacs we have an icon and prefix for the icon which indicates the state of the node. There is prefix icon for open/closed and empty for no-children.

Why not just concat the prefix with the actual icon? That would fit with treemacs' existing setup.

Adding a different icon to indicate an empty state is a good idea. I'll see about adding an option for it when I'm done with the current plans.

(off topic) WDYT about adding the ability to show things at the end of the line.

Doable. Fringe-indicator-mode is already putting things in the fringe, adapting the current git-mode should hopefully not be too difficult.

[yyoncho]

Why not just concat the prefix with the actual icon?

This is what we do but it goes in the lsp-treemacs in render method instead of having each extension calculating it. It is not a big issue though, but IMO other extensions could benefit from it. You could build a tree without specifying icons too(check the example). I think that the issue that I had(but I cannot confirm it right now) is that I cannot define single click action on that icon to toggle the state of the node.

[yyoncho]

Adding a different icon to indicate an empty state is a good idea.

prefixing with ▾ ▸ help indicating that. Otherwise, you don't know if it can be expanded and if it does not have children.

[Alexander-Miller]

@yyoncho As promised I've pushed a prototype to the treelib branch now.

There's a set of working examples in Extensions.org that you can load with org-babel-load-file.

[yyoncho]

@yyoncho As promised I've pushed a prototype to the treelib branch now.

There's a set of working examples in Extensions.org that you can load with org-babel-load-file.

Thank you, I will be inactive for some time and I will play with it once I am back.

[shackra]

any update on this?

[Alexander-Miller]

I've a working draft ready on the treelib branch, and was just hoping to get some feedback before I go in with the detail work.

You're welcome to try it yourself if you've a use case for it.

Other than that I'll get back into it for the finishing touches and bug fixes in a few days.

[shackra]

You're welcome to try it yourself if you've a use case for it.

I was coming from https://github.com/ubolonton/emacs-tree-sitter/issues/23 :grimacing: Thanks for the update

[Alexander-Miller]

@yyoncho Looks like like you're active again. Shall we give this another go?

[yyoncho]

@yyoncho yes, I will try to go through the code during the weekend.

[yyoncho]

I am getting (It might be my fault).

Debugger entered--Lisp error: (wrong-number-of-arguments ((t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value 'major-mode it)) (let (result) (let ((list ...) (i 0) it it-index) (ignore it it-index) (while list (setq it ... it-index i i ...) (if ... ...))) (nreverse result))))) 1)
  (closure (t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value 'major-mode it)) (let (result) (let ((list ...) (i 0) it it-index) (ignore it it-index) (while list (setq it ... it-index i i ...) (if ... ...))) (nreverse result)))))(#<marker at 1 in *Showcase Buffers*>)
  funcall((closure (t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value 'major-mode it)) (let (result) (let ((list ...) (i 0) it it-index) (ignore it it-index) (while list (setq it ... it-index i i ...) (if ... ...))) (nreverse result))))) #<marker at 1 in *Showcase Buffers*>)
  (let* ((items (funcall (progn (or (and (memq ... cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list ... ext))) (aref ext 6)) btn)) (btn-path (get-text-property btn :path)) (parent-path (list btn-path)) (parent-dom-node (gethash btn-path treemacs-dom nil)) (child-ext (funcall (progn (or (and (memq ... cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list ... ext))) (aref ext 11)))) (child-state (funcall (progn (or (and (memq ... cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list ... child-ext))) (aref child-ext 2)))) (closed-icon-fn (progn (or (and (memq (type-of child-ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension child-ext))) (aref child-ext 4))) (label-fn (progn (or (and (memq (type-of child-ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension child-ext))) (aref child-ext 9))) (properties-fn (progn (or (and (memq (type-of child-ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension child-ext))) (aref child-ext 10))) (face-fn (progn (or (and (memq (type-of child-ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension child-ext))) (aref child-ext 8))) (key-fn (progn (or (and (memq (type-of child-ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension child-ext))) (aref child-ext 7)))) (prog1 (save-excursion (let* ((p (point))) (let (buffer-read-only) (let* ((val ...)) (put-text-property (or ... ...) (or ... ...) :state val)) (goto-char (or (next-single-property-change btn ...) (point-max))) (progn (insert (apply ... ...))) (progn (let* (...) (let ... ...)) (treemacs--reentry (get-text-property btn :path)))) (count-lines p (point)))) (if treemacs-move-forward-on-expand (progn (let* ((parent (let ... ...)) (child (next-button parent))) (if (equal parent (get-text-property child :parent)) (progn (forward-line 1))))))))
  treemacs--expand-variadic-parent(#<marker at 1 in *Showcase Buffers*> #s(treemacs-extension :name showcase-variadic-buffers :closed-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-closed-state) :open-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-open-state) :closed-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :open-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :children (closure (t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value ... it)) (let (result) (let (... ... it it-index) (ignore it it-index) (while list ... ...)) (nreverse result))))) :key (closure (t) (&optional item) "" (ignore item) 'SHOWCASE-VARIADIC-BUFFERS) :face (closure (t) (&optional item) "" (ignore item) 'font-lock-variable-name-face) :label (closure (t) (&optional item) "" (ignore item) (symbol-name (car item))) :more-properties (closure (t) (item) "" (ignore item) nil) :child-type (closure (t) nil "" (symbol-value 'treemacs-showcase-buffer-group-extension-instance)) :variadic? t :async? nil :entry-point? t))
  (let ((marker (copy-marker (point) t))) (treemacs--expand-variadic-parent button-start ext) (goto-char marker))
  (let* ((key (funcall (progn (or (and (memq ... cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list ... ext))) (aref ext 7)))) (pr (treemacs-project->create! :name (funcall (progn (or (and ... t) (signal ... ...)) (aref ext 9))) :path key :path-status 'extension)) (button-start (point-marker)) (dom-node (record 'treemacs-dom-node key nil nil nil button-start nil nil))) (let* ((position (point-marker))) (prog1 nil (puthash (if (and (memq (type-of key) cl-struct-treemacs-project-tags) t) (progn (or (and ... t) (signal ... ...)) (aref key 2)) key) position treemacs--project-positions))) (prog1 nil (puthash (progn (or (and (memq (type-of dom-node) cl-struct-treemacs-dom-node-tags) t) (signal 'wrong-type-argument (list 'treemacs-dom-node dom-node))) (aref dom-node 1)) dom-node treemacs-dom)) (insert (propertize "Hidden Node\n" 'button '(t) 'category 'default-button 'invisible t 'skip t :custom t :key key :path key :depth -1 :project pr :state (funcall (progn (or (and (memq ... cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list ... ext))) (aref ext 2))))) (let ((marker (copy-marker (point) t))) (treemacs--expand-variadic-parent button-start ext) (goto-char marker)))
  (let (buffer-read-only) (let* ((key (funcall (progn (or (and ... t) (signal ... ...)) (aref ext 7)))) (pr (treemacs-project->create! :name (funcall (progn (or ... ...) (aref ext 9))) :path key :path-status 'extension)) (button-start (point-marker)) (dom-node (record 'treemacs-dom-node key nil nil nil button-start nil nil))) (let* ((position (point-marker))) (prog1 nil (puthash (if (and (memq ... cl-struct-treemacs-project-tags) t) (progn (or ... ...) (aref key 2)) key) position treemacs--project-positions))) (prog1 nil (puthash (progn (or (and (memq ... cl-struct-treemacs-dom-node-tags) t) (signal 'wrong-type-argument (list ... dom-node))) (aref dom-node 1)) dom-node treemacs-dom)) (insert (propertize "Hidden Node\n" 'button '(t) 'category 'default-button 'invisible t 'skip t :custom t :key key :path key :depth -1 :project pr :state (funcall (progn (or (and ... t) (signal ... ...)) (aref ext 2))))) (let ((marker (copy-marker (point) t))) (treemacs--expand-variadic-parent button-start ext) (goto-char marker))))
  (save-excursion (let (buffer-read-only) (let* ((key (funcall (progn (or ... ...) (aref ext 7)))) (pr (treemacs-project->create! :name (funcall (progn ... ...)) :path key :path-status 'extension)) (button-start (point-marker)) (dom-node (record 'treemacs-dom-node key nil nil nil button-start nil nil))) (let* ((position (point-marker))) (prog1 nil (puthash (if (and ... t) (progn ... ...) key) position treemacs--project-positions))) (prog1 nil (puthash (progn (or (and ... t) (signal ... ...)) (aref dom-node 1)) dom-node treemacs-dom)) (insert (propertize "Hidden Node\n" 'button '(t) 'category 'default-button 'invisible t 'skip t :custom t :key key :path key :depth -1 :project pr :state (funcall (progn (or ... ...) (aref ext 2))))) (let ((marker (copy-marker (point) t))) (treemacs--expand-variadic-parent button-start ext) (goto-char marker)))))
  treemacs--variadic-extension-entry-render(#s(treemacs-extension :name showcase-variadic-buffers :closed-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-closed-state) :open-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-open-state) :closed-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :open-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :children (closure (t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value ... it)) (let (result) (let (... ... it it-index) (ignore it it-index) (while list ... ...)) (nreverse result))))) :key (closure (t) (&optional item) "" (ignore item) 'SHOWCASE-VARIADIC-BUFFERS) :face (closure (t) (&optional item) "" (ignore item) 'font-lock-variable-name-face) :label (closure (t) (&optional item) "" (ignore item) (symbol-name (car item))) :more-properties (closure (t) (item) "" (ignore item) nil) :child-type (closure (t) nil "" (symbol-value 'treemacs-showcase-buffer-group-extension-instance)) :variadic? t :async? nil :entry-point? t))
  (if (progn (or (and (memq (type-of ext) cl-struct-treemacs-extension-tags) t) (signal 'wrong-type-argument (list 'treemacs-extension ext))) (aref ext 12)) (treemacs--variadic-extension-entry-render ext) (treemacs--singular-extension-entry-render ext))
  treemacs-render-extension(#s(treemacs-extension :name showcase-variadic-buffers :closed-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-closed-state) :open-state (closure (t) nil "" 'treemacs-showcase-variadic-buffers-open-state) :closed-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :open-icon (closure (t) (&optional item) "" (ignore item) (let* ((theme treemacs--current-theme) (icons (progn (or ... ...) (aref theme 3)))) (gethash 'list icons nil))) :children (closure (t) (btn item) (ignore btn item) (-distinct (mapcar #'(lambda (it) (ignore it) (buffer-local-value ... it)) (let (result) (let (... ... it it-index) (ignore it it-index) (while list ... ...)) (nreverse result))))) :key (closure (t) (&optional item) "" (ignore item) 'SHOWCASE-VARIADIC-BUFFERS) :face (closure (t) (&optional item) "" (ignore item) 'font-lock-variable-name-face) :label (closure (t) (&optional item) "" (ignore item) (symbol-name (car item))) :more-properties (closure (t) (item) "" (ignore item) nil) :child-type (closure (t) nil "" (symbol-value 'treemacs-showcase-buffer-group-extension-instance)) :variadic? t :async? nil :entry-point? t))
  (let* ((buf (get-buffer-create bufname))) (pop-to-buffer buf) (treemacs-initialize) (set (make-local-variable 'treemacs-space-between-root-nodes) nil) (treemacs-render-extension treemacs-showcase-variadic-buffers-extension-instance))
  (let* ((bufname "*Showcase Buffers*")) (let ((it (get-buffer bufname))) (if it (progn (kill-buffer it)))) (let* ((buf (get-buffer-create bufname))) (pop-to-buffer buf) (treemacs-initialize) (set (make-local-variable 'treemacs-space-between-root-nodes) nil) (treemacs-render-extension treemacs-showcase-variadic-buffers-extension-instance)))

Other than that:

1. Will this be a breaking change?
2. This is more like a feature request: will it be possible to add the ability to add text in the end of the line in the extension nodes? This is what vscode does for showing various data.

[Alexander-Miller]

Will this be a breaking change?

To some degree. I tried to avoid this, however there have been some internal changes that probably prevent the old and the new extensions from running at the same time.

At the very least the new api sits in a new module, so when exactly the switch happens is up to you. Your packages will not start breaking all of a sudden because of an update of mine. Treemacs should also publish the new module ahead of time, so more people will have the new api available by the time they update their lsp packages.

will it be possible to add the ability to add text in the end of the line in the extension nodes?

I don't think this is in the scope of just the extension api, but I already had some previous ideas where this would fit in well - basically a general "annotation" api that uses the current deferred git-mode concept for a few more things: faces for git, overlays for diagnostics, and suffix strings for the stuff you want to show after the node.

I am getting (It might be my fault).

Can't reproduce it. Is your treelib branch up to date? Rebasing it on master means I have to force-push changes, so you need to hard-reset your local copy.

[yyoncho]

To some degree. I tried to avoid this, however there have been some internal changes that probably prevent the old and the new extensions from running at the same time.

So, old extension will work until someone uses the new api, right?

Other than that code looks good to me, and once we figure out the migration story it can be merged.

[Alexander-Miller]

So, old extension will work until someone uses the new api, right?

That's the plan.

Other than that code looks good to me, and once we figure out the migration story it can be merged.

There's probably plenty of nitpicking to be done for when you start implementing an actual migration. You've been able to pick up plenty of stuff that I missed in the past and I've no doubt this time will be no different.

Now is also the time to unpack your wish list. Whatever I've previously delayed for a more convenient time, that time is now. So whatever additional features you want - I remember at least mouse interface integration and rendering an extension as already expanded - tell me all about them.

[yyoncho]

I remember at least mouse interface integration and rendering an extension as already expanded - tell me all about them.

Let me think about this. The major PITA was the async support which is now done.

Unfortunately, this time there is a bigger user base and we should be more careful when doing the transition.

[Alexander-Miller]

@yyoncho You also get to pick the error interface for async nodes. So far it's completely undefined, so if something does go wrong that "Loading" text is sticking around forever.

Treemacs should know there was an error so it can notify the user, so just returning nil is out. Other than that I see 2 options:

• Using a second callback to be called with an error message when something goes wrong.
• Calling the normal callback with a special value like (:async-error error-message-string)

I have no preference one way or the other, so you can pick (or propose) whatever would suit you best.

[yyoncho]

• Calling the normal callback with a special value like (:async-error error-message-string)

I don't have a preference either but let's pick this one since it is closer to what lsp-treemacs has now.

[Alexander-Miller]

Async error handling is up now. I've also rewritten the first part of the documentation.

[yyoncho]

Async error handling is up now. I've also rewritten the first part of the documentation.

Can you include in your testing refreshing of an async tree? This is one of the things that is still troublesome on lsp-treemacs side.

[Alexander-Miller]

Hmmmm. I don't think the plain treemacs-update-node is fit for that purpose. It would technically work, but the tree would visibly stutter because of the async delay, even if it was as good as instantaneous.

The decision of synchronization is in the hands of the caller, so I suggest the following mechanism: treemacs-update-node is expanded with a second optional parameter - the :children that would otherwise be queried when the node is expanded.

That way you'll be able to fire off an asynchronous lsp request, and synchronously update treemacs when you receive an answer.

WDYT?

[yyoncho]

The decision of synchronization is in the hands of the caller, so I suggest the following mechanism: treemacs-update-node is expanded with a second optional parameter - the :children that would otherwise be queried when the node is expanded.

I didn't quite get the proposal. Can you elaborate?

One way to do it, to avoid flickering is to refresh the model recursively and then just place the values in the tree.

What I do in lsp-treemacs right now is to trigger async loading for all nodes and if there is data for the node, it will be returned until the new data is loaded. This is to avoid flickering.

[Alexander-Miller]

Never mind, I had completely forgotten that a refresh would move down the hierarchy of expanded nodes, pre-calculating the content of just the top-level node you are refreshing will not work because its children are updated as well.

I think your caching idea is the way to go. I'll try to have something done next week.

[Alexander-Miller]

Update: I tried something, it didn't work, and now we have to try something
else.

I went by your description and tried to handle updates in such a way that actual
reloading would recursively work its way down the tree one generation after the
other instead of all at once as usual, with a cache being used for the nodes
that are not being updated yet.

However after a small trial I don't think that's the right approach. Not only
would a proper implementation of such a system be terribly complicated to do, I
think it'd be rife with difficult-to-reproduce edge cases and race conditions to
the point of not being worth the effort.

Instead I propose a different system that, while unfortunately putting more work
on the user of the api, has the great advantage of being deterministic, testable
and debuggable.

Basically we go back to my first proposal where you first gather all the
asynchronous data in advance, except we have to do it for an entire subtree. So
first you have to ask treemacs which nodes are currently expanded that need
updating. You get a nested list of the current setup, use that to asynchronously
re-compute the tree's new layout, and pass the info back to treemacs once you're
done so it can all be redisplayed in one step.

An example: say you want to update a tree displaying a file's symbols, you ask
treemacs for the current layout and get something like this:

(classes
 (class1
  (methods
   (method-1
    method-2))
  (members))
 (class2
  (methods
   (method-3
    method-4))
  (members
   (member-1
    member-2))))

You ask the language server for the new setup. Recursively going through that
tree to get all child and grandchild nodes' content is the unpleasant part that
gets shifted to your side.

When all is done you call treemacs again with something that looks like this and
it all gets rendered in one go.

(classes
 (class2
  (methods
   (method-3
    method-4
    method-X
    method-Y))
  (members
   (member-2
    member-Z))))

This is just a rough draft, we can decide on the details as necessary.

WDYT?

[yyoncho]

You get a nested list of the current setup, use that to asynchronously
re-compute the tree's new layout, and pass the info back to treemacs once you're
done so it can all be redisplayed in one step.

Why this should be in the extension code? The core code can perform that too. It has to call the function which returns the children for all open nodes and after it can construct the new tree.

[Alexander-Miller]

Why this should be in the extension code?

I'm not saying it should be. Async extensions is just where it would now be very convenient to have such a feature.

What difference would it make anyway?

[yyoncho]

I'm not saying it should be.

I am confused -

Recursively going through that tree to get all child and grandchild nodes' content is the unpleasant part that
gets shifted to your side.

What difference would it make anyway?

It makes difference because that part will be reimplemented over and over again and it seems to me that given the fact that the extension provides a method load-children(node-id, callback) then the core code can refresh the data without the need for the client to implement a more complex interface. Am I missing something?

[Alexander-Miller]

Ah, I got really confused what you meant by your original question and thought
you're talking about putting things into treemacs' core vs the treelib module.

Treemacs vs a client like lsp makes a lot more sense.

the extension provides a method load-children(node-id, callback) then the core
code can refresh the data without the need for the client to implement a more
complex interface. Am I missing something?

It does, but what happens there is that you are the one implementing
load-children. In the current examples a timer simulates asynchronicity. In a
real use-case you'd have some lsp code asking the language server for
information and calling the callback when you receive an answer.

So my train of thought was that since the client is in control for the fetching
of the children and passing them to treemacs that it'd be best if the client
also does that for updating an entire subtree.

However you are right that it would place an undue burden on your end. And it's
a good thing that you put things so plainly with load-children(node-id, callback),
I had forgotten that the callback is in my control and hadn't thought of using
it for my implementation.

So with this realization the original offer is back on the table: updating async
nodes without flicker and without extra effort for the client. I'll try to have
a working version within another week.