Alexander-Miller
Towards a polymorphic extensions implementation
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.
ping @yyoncho As a heavy user of extensions I am sure you have plenty of ideas how things can be improved.
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.
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.
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])))
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.
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.
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)))
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.
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.
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?
@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.
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))
(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.
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.
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.
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.
@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 As promised I've pushed a prototype to the
treelibbranch 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.
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.
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
@yyoncho Looks like like you're active again. Shall we give this another go?
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:
- Will this be a breaking change?
- 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.
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.
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.
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.
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.
@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.
- 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.