Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add command to "collect" nodes #163

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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.

None yet

2 participants