jgru / consult-org-roam

A bunch of convenience functions for operating org-roam with the help of consult
GNU General Public License v3.0
121 stars 6 forks source link

Add consult-org-roam-buffer #13

Closed jgru closed 2 years ago

jgru commented 2 years ago

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.

apc commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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