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 consult-org-roam-buffer #13

Merged
merged 18 commits into from
Oct 16, 2022
Merged

Add consult-org-roam-buffer #13

merged 18 commits into from
Oct 16, 2022

Conversation

jgru
Copy link
Owner

@jgru jgru commented Oct 7, 2022

Add consult-org-roam-buffer.el to address #12 and provide the ability to use a custom source for org-roam-notes with consult-buffer.

This is related to #12 and to #660 opened by @apc at the consult repo.

Add consult-org-roam-buffer to fix #12
@jgru jgru self-assigned this Oct 7, 2022
@jgru jgru added the enhancement New feature or request label Oct 7, 2022
@jgru jgru linked an issue Oct 7, 2022 that may be closed by this pull request
@apc
Copy link

apc commented Oct 7, 2022

Starting to look into this. Thanks!

Quick first question: what do you think about the issue of having org-roam buffers buried all the way after consult--source-recent-files? Maybe just adding an option to move it to the top?

Here's a rough proposal adapting your own suggestion in the original discussion:

(defcustom consult-org-roam-buffer-after-buffers nil
  "If non-nil, display org-roam buffers right after non-org-roam buffers.
  Otherwise, display org-roam buffers after any other visible default
  source")

(defun consult-org-roam-buffer-setup ()
  (if consult-org-roam-buffer-after-buffers
    (let* ((idx (cl-position 'consult--source-buffer consult-buffer-sources :test 'equal))
           (tail (nthcdr idx consult-buffer-sources)))
      (setcdr
       (nthcdr (1- idx) consult-buffer-sources)
       (append (list 'org-roam-buffer-source) tail)))
    (add-to-list 'consult-buffer-sources 'org-roam-buffer-source 'append)))

Final question (for now!): what do you think about propertizing the output of consult-org-roam-buffer--get-title so that the hash is less prominent?

@jgru
Copy link
Owner Author

jgru commented Oct 7, 2022

Starting to look into this. Thanks!

Glad to hear.

One initial question: did you mean to leave out a narrowing key for org-roam-buffer-source?

I created a defcustom named consult-org-roam-buffer-narrow-key. I used ?n for notes. I still need some time to add documentation to the readme describing how to set this.

A second question: what do you think about the issue of having org-roam buffers buried all the way after consult--source-recent-files? Maybe just adding an option to move it to the top?

This is very sensible I think.

Here's a rough proposal adapting your own suggestion in the original discussion:

Great, I added this in 401f68e.

Final question (for now!): what do you think about propertizing the output of consult-org-roam-buffer--get-title so that the hash is less prominent?

Good idea. Should we make it completely invisible?

Best regards,
jgru

@apc
Copy link

apc commented Oct 7, 2022

Starting to look into this. Thanks!

Glad to hear.

One initial question: did you mean to leave out a narrowing key for org-roam-buffer-source?

I created a defcustom named consult-org-roam-buffer-narrow-key. I used ?n for notes. I still need some time to add documentation to the readme describing how to set this.

Yes, sorry, I realized this after posting (not sure how I didn't see it when I checked the value of org-roam-buffer-source!), tried to edit to save you the time responding, but you were too quick!

A second question: what do you think about the issue of having org-roam buffers buried all the way after consult--source-recent-files? Maybe just adding an option to move it to the top?

This is very sensible I think.

Here's a rough proposal adapting your own suggestion in the original discussion:

Great, I added this in 401f68e.

Great!

Final question (for now!): what do you think about propertizing the output of consult-org-roam-buffer--get-title so that the hash is less prominent?

Good idea. Should we make it completely invisible?

I think that would be great. It's a bit of an edge case anyway, having same title in different buffers, and the noise from the hashes is a bit much (I think).

Best,

A.

Best regards, jgru

@jgru
Copy link
Owner Author

jgru commented Oct 7, 2022

Yes, sorry, I realized this after posting (not sure how I didn't see it when I checked the value of org-roam-buffer-source!), tried to edit to save you the time responding, but you were too quick!

Nevermind, it was not that obvious anyways.

I think that would be great. It's a bit of an edge case anyway, having same title in different buffers, and the noise from the hashes is a bit much (I think).

I completely agree and added this in commit 2803df5.

After extending the documentation and checking that it is working out of the box when installed via straight or melpa we are good to go I think.

Thanks again for kicking this off...I think it is a really helpful feature. Sure, one could just use org-roam-node-find but having a defined scope of open org-roam-buffers is helpful.

Best regards,
jgru

@apc
Copy link

apc commented Oct 7, 2022

Nice! Something is a bit off now, though: after opening a single org-roam-buffer, the source lists it twice. Let me see if I can figure out what's going on. Will write back.

@apc
Copy link

apc commented Oct 7, 2022

Just reporting, in the meantime, that there's something weird going on with my config at the moment, so that may be part of the problem.

@jgru
Copy link
Owner Author

jgru commented Oct 7, 2022

Nice! Something is a bit off now, though: after opening a single org-roam-buffer, the source lists it twice. Let me see if I can figure out what's going on. Will write back.

Okay, I can't reproduce that.

Just reporting, in the meantime, that there's something weird going on with my config at the moment, so that may be part of the problem.

This could be an issue, take your time to inspect that and please tell me if you find any issue.

@apc
Copy link

apc commented Oct 7, 2022

I'm baffled here. I now have set org-roam-directory to a test dir containing just two files. I've run org-roam-db-clear-all etc. Once I load consult-org-roam-buffer, the following happens after just visiting one buffer:

image

I was having some issues earlier with org-roam-buffer-list acting up (actually, buffer-list was acting up, not sure why). But now if I call org-roam-buffer-list I just get:

(#<buffer 20221007103219.org>)

So I do not understand how it could be showing up twice on the list of buffers. I'll keep digging but if you have any clues, let me know.

@jgru
Copy link
Owner Author

jgru commented Oct 7, 2022

I'm baffled here. I now have set org-roam-directory to a test dir containing just two files. I've run org-roam-db-clear-all etc. Once I load consult-org-roam-buffer, the following happens after just visiting one buffer:

That's really weird. Again, I cannot reproduce the issue.

So I do not understand how it could be showing up twice on the list of buffers. I'll keep digging but if you have any clues, let me know.

Unfortunately, I have no clue what might be going on here.
Generally, I think it is always good to test weird issues with emacs -Q and, e.g., this minimal config.

@apc
Copy link

apc commented Oct 7, 2022

OK, I think I figured out what's going on. The problem is that if you set consult-org-roam-buffer-after-buffers to t and then run consult-org-roam-buffer-setup, you end up with org-roam-buffer-source showing up twice in consult-buffer-sources. Simply running (delete-dups consult-buffer-sources) fixed the problem.

I guess one way to avoid this would be to change the definition of consult-org-roam-buffer-setup to first remove org-roam-buffer-source from consult-buffer-sources, e.g. by first running

(setq consult-buffer-sources (delete org-roam-buffer-source consult-buffer-sources))

Not sure this is the best or the canonical way to do this sort of thing though.


Unrelated: the minimal config you linked to is quite helpful, but it sets the value of org-roam-directory to some random path which is probably non-existent in most people's machines. I gather the idea was to provide something like a test directory of org-roam notes to work with?

@jgru
Copy link
Owner Author

jgru commented Oct 8, 2022

OK, I think I figured out what's going on. The problem is that if you
set consult-org-roam-buffer-after-buffers to t and then run
consult-org-roam-buffer-setup, you end up with
org-roam-buffer-source showing up twice in consult-buffer-sources.
Simply running (delete-dups consult-buffer-sources) fixed the
problem.

Thanks for investigating this. I could reproduce it by changing the defcustom and manually re-evaluation org-roam-buffer-setup.

I guess one way to avoid this would be to change the definition of
consult-org-roam-buffer-setup to first remove
org-roam-buffer-source from consult-buffer-sources, e.g. by first
running

(setq consult-buffer-sources (delete org-roam-buffer-source consult-buffer-sources))

Not sure this is the best or the canonical way to do this sort of
thing though.

I think this is viable to do. I just hat to quote the symbol org-roam-buffer-source like so:

(setq consult-buffer-sources
    (delete 'org-roam-buffer-source consult-buffer-sources))

Commit 3eee411 should fix the problem, can you confirm that?

Unrelated: the minimal config you linked to is quite helpful, but it
sets the value of org-roam-directory to some random path which is
probably non-existent in most people's machines. I gather the idea was
to provide something like a test directory of org-roam notes to work
with?

Thanks for the notice. I changed that path to /tmp/roam and added a ;; FIXME comment to it (see here). It would be probably nice to have an elisp function that populates a test directory with some notes during :init.

Best regards,
jgru

@apc
Copy link

apc commented Oct 11, 2022

OK, so far so good! My only question is whether there's a clearer way of indicating that a file has been deleted than "not persisted". I for one wouldn't have known what to make of this had I encountered it. What about simply "file deleted"?

About the test config, have you considered just having some test files that can be downloaded as part of the repo, which could be what org-roam-dir is set to according to your minimal config? I think citar has something like that setup and I think it works fine.

@jgru
Copy link
Owner Author

jgru commented Oct 11, 2022

OK, so far so good! My only question is whether there's a clearer way
of indicating that a file has been deleted than "not persisted". I for
one wouldn't have known what to make of this had I encountered it.
What about simply "file deleted"?

Thanks for looking at it again.
"File deleted" is certainly more expressive. I changed the term in commit ffd6fbd.

About the test config, have you considered just having some test files
that can be downloaded as part of the repo, which could be what
org-roam-dir is set to according to your minimal config? I think
citar has something like that
setup and I think it works fine.

I think populating a org-roam-directory with test notes should be done in a programmatic way. To track this, I created an issue #14. If you are interested to implement this, please feel free to do so.

Best regards,
jgru

@jgru jgru merged commit 6f64973 into main Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rough idea: add source for consult-buffer
2 participants