Skip to content

Conversation

@publicimageltd
Copy link

@publicimageltd publicimageltd commented Nov 20, 2021

This is the proposed change for adding a "collection" function, the elisp-side. It introduces one new command verb, "collect", which is handeld in org-roam-ui--on-msg-collect-node.

Sorry for also merging the latest commits from the main branch before doing the PR, my changes are in the first commit.

I did not add yet anything for gathering the possible list of collections. There's a decision to be made. Currently, I see two possibilities: (a) just offer a list of open buffers holding collections; that would be simply a call to delve-buffer-list. (b) More powerful, offer a list of open buffers and a list of files. If the user chooses a file, it will be loaded into a buffer on the fly. That's what `Delve' does. I would have to write a function which offers that functionality.

I think (b) is more powerful and simply better, but it could mean that the web menu will offer quite a bit of choices. So that's mainly what was holding me back. If there's an elegant solution for that problem, I would adopt for (b). Maybe we could first use (a) and then solve (b) by only offering the last 5 recently used collections, or something like that.

Another thing: Currently "collecting" nodes adds the nodes in the background, leaving the current window configuration in Emacs as it is. That might be unitintuitive and could be turned into an option.

This commit introduces a new command verb "collect", which basically
passes its arguments to a function which can be defined by the user.
Per default, the new command passes its arguments to
`delve-insert-nodes-by-id' if the package `Delve' is present. Also
rewrites the command dispatcher so that it uses the more idiomatic
pcase statement. Aside from being nicer to read, pcase allows us to
introduce more complex command patterns if simple one word strings
won't suffice.
@tefkah
Copy link
Contributor

tefkah commented Nov 26, 2021

Thank you for putting this together! I'm pretty busy with other things atm so I haven't been able to try and implement it yet, but I think I'll be able to next week or so!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants