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 emacs-tree-sitter/elisp-tree-sitter#23 😬 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.

[Alexander-Miller]

@yyoncho We still doing this?

I can help you with the migration if you're busy elsewhere.

[yyoncho]

Sorry, I am very busy (changed jobs, having a second child). I guess we can go ahead, merge it and start fixing the regressions... Most of them will be localized in lsp-treemacs

[stale]

This issue has been automatically marked as stale because it has not had recent activity.

[Alexander-Miller]

@yyoncho I have some extra time now, I'll make the push within the next couple of days.

[Alexander-Miller]

New version is up. I'll close this issue now, new feedback can be collected in #975.

I've reworked the documentation, I think it should give a good idea of the new options you have.

There's also a not quite finished mu4e addon that shows off a serious implementation: https://github.com/Alexander-Miller/treemacs/blob/master/src/extra/treemacs-mu4e.el

[yyoncho]

can you tag a release just before that commit?

[Alexander-Miller]

Sure thing. 2.11 is the last tag before the rewrite. The current version is 3.0.

[yyoncho]

can you make :face optional in treemacs-define-expandable-node-type and allow the clients to specify the face in the label itself?

[yyoncho]

also is there a variable to make it possible to expand N levels of the generated tree?

[yyoncho]

Can you provide a recipe to refresh a buffer without losing the nodes open/closed state?

[yyoncho]

I am getting also this when I am opening/closing async nodes:

Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
  treemacs--do-collapse-extension-node(#<marker (moves after insertion) at 62 in *dap-ui-sessions*> #s(treemacs-extension :name lsp-treemacs-generic-node :closed-state (closure (t) nil "" 'treemacs-lsp-treemacs-generic-node-closed) :open-state (closure (t) nil "" 'treemacs-lsp-treemacs-generic-node-open) :closed-icon (closure (t) (&optional btn item) "" (ignore item) (ignore btn) (lsp-treemacs--generic-icon item nil)) :open-icon (closure (t) (&optional btn item) "" (ignore item) (ignore btn) (lsp-treemacs--generic-icon item t)) :children (closure (t) (&optional btn item callback) (ignore btn item callback) (let ((input0 item)) (let* ((item input0) (children (plist-get item :children)) (children-async (plist-get item :children-async))) (cond ((functionp children) (funcall callback ...)) (children-async (let* ... ...)) (t (funcall callback ...)))))) :key (closure (t) (&optional btn item) "" (ignore item) (ignore btn) item) :face (closure (t) (&optional btn item) "" (ignore item) (ignore btn) (let* ((label (and t (plist-get item :label)))) (if label (get-text-property 0 'face label) 'default))) :label (closure (t) (&optional btn item) "" (ignore item) (ignore btn) (plist-get item :label)) :more-properties (closure (t) (&optional btn item) "" (ignore item) (ignore btn) nil) :child-type (closure (t) nil "" (symbol-value 'treemacs-lsp-treemacs-generic-node-extension-instance)) :variadic? nil :async? t :entry-point? nil) nil)
  treemacs-collapse-extension-node(nil)
  treemacs-TAB-action(nil)
  funcall-interactively(treemacs-TAB-action nil)
  command-execute(treemacs-TAB-action)

[Alexander-Miller]

can you make :face optional in treemacs-define-expandable-node-type and allow the clients to specify the face in the label itself?

Done.

[Alexander-Miller]

also is there a variable to make it possible to expand N levels of the generated tree?

When the prefix arg is an integer N you will open N extra levels, at least for normal nodes. I'll implement it for extensions next.

[Alexander-Miller]

Can you provide a recipe to refresh a buffer without losing the nodes open/closed state?

If you mean what I think you mean then it should work already. Can you provide an example?

I am getting also this when I am opening/closing async nodes:

I've seen the same errors when I didn't have lexical binding enabled. Make sure that it's on.

[Alexander-Miller]

can you make :face optional in treemacs-define-expandable-node-type and allow the clients to specify the face in the label itself?

Done.

Now that I think about it, do we even need an independent :face parameter? It's strictly tied to the :label anyway, and less flexible to boot, since a label could want to use multiple faces. If you don't see a use for it either, then I'll go ahead and delete it.

BTW I have also added a treemacs-treelib-version constant so you can check for a minimal required version.

[yyoncho]

Now that I think about it, do we even need an independent :face parameter?

I don't need it.

[yyoncho]

I am getting also this when I am opening/closing async nodes:

I've seen the same errors when I didn't have lexical binding enabled. Make sure that it's on.

I have it on. Note that it won't work at all with lexical binding because it won't be able to call the callback. The issue happens most likely due to opening/closing/opening and then receiving the async data for the first open.

Can you provide a recipe to refresh a buffer without losing the nodes open/closed state?

If you mean what I think you mean then it should work already. Can you provide an example?

I have new data and I want to refresh the tree without losing the context. The previous recipe seems to be broken.

[yyoncho]

Can you provide a recipe to refresh a buffer without losing the nodes open/closed state?

If you mean what I think you mean then it should work already. Can you provide an example?

I have new data and I want to refresh the tree without losing the context. The previous recipe seems to be broken.

The following code used to work:

(let ((inhibit-read-only t))
        (treemacs-update-node '(lsp-treemacs-generic-root) t))

[Alexander-Miller]

I don't need it.

It's removed now.

When the prefix arg is an integer N you will open N extra levels, at least for normal nodes. I'll implement it for extensions next.

That's done now.

The following code used to work:

That is also fixed.

Note that I haven't extensively tested the new changes with an asynchronous implementation. I'll do that next, but I figure it is better to give you the changes you request sooner rather than later.

It'd be useful if you could upload your working changes so I can look at issues based on the actual implementation instead of relying on my simple timed buffers example. I have a python and a rust language server installed that I can test against.

I have it on. Note that it won't work at all with lexical binding because it won't be able to call the callback. The issue happens most likely due to opening/closing/opening and then receiving the async data for the first open.

Whatever it is, it's not as simple as repeatedly opening and closing an async node, at least in my example.
I'm not sure why you say it won't work with lexical binding. The code from your error message looks like this:

#(treemacs-extension
 :children
 (closure
  (t) (&optional btn item callback)
  (ignore btn item callback)
  (let ((input0 item))
    (let* ((item input0)
           (children (plist-get item :children))
           (children-async (plist-get item :children-async)))
      (cond ((functionp children)
             (funcall callback ...))
            (children-async
             (let* ... ...))
            (t (funcall callback ...)))))))

Why should the callback not be usable here?

[yyoncho]

When the prefix arg is an integer N you will open N extra levels, at least for normal nodes. I'll implement it for extensions next.

That's done now.

I want that to be controlled by the extension API. E. g. I want to be able to pass argument and expand the tree 2 levels or at all.

[Alexander-Miller]

Got it, I'll add these changes next.

[Alexander-Miller]

Expansion's done now. I have changed the signature of treemacs-initialize so you can do something like this:

(treemacs-initialize showcase-buffers-variadic
    :with-expand-depth 'all
    :and-do (setf treemacs-space-between-root-nodes t))