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

Handle dired bookmarks from Bookmarks+ #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

legendre6891
Copy link

Hi! Fantastic package -- what do you think about adding support for bmkp's dired directories? They have a non-nil handler.

Bookmark+ dired bookmarks have their handlers set to `bmkp-dired-jump`; this modifies the bookmark filtering to support that usecase.
@legendre6891
Copy link
Author

@karthink Wondering if you’ve had time to take a look? Thanks!

@karthink
Copy link
Owner

Sorry for the delay. Integrating bmkp bookmarks is a good idea. But there are two alternatives I'd like to think about:

  1. Make a separate bmkp source. This is the intended way to customize consult-dir to your needs. This is straightforward to do, there's an example in the README.
  2. Make a defcustom consult-dir-bookmark-handlers for bookmark handlers to consider (beside the nil value). Then you can add bmkp-dired-jump to that list.

I would prefer not to hardcode tests for features provided by libraries not included in Emacs. (I know that I made an exception for Projectile but I'm rethinking that too.)

What do you think of options 1 and 2?

@legendre6891
Copy link
Author

Not a problem at all, @karthink, and thanks for your thoughts. Here are my opinions regarding your two points:

  1. I would prefer not to make bmkp a separate source. The reason is that someone (like me :-)) who is using bmkp is unlikely to be using the vanilla bookmarks, and vice-versa. Moreover, in so far as consult-dir is concerned, there is not much semantic difference about whether a dired bookmark came from vanilla bookmarks or bmkp.

  2. I think this is a great idea, I guess it will suffice for consult-dir-bookmark-handlers to be a list of symbols for which to filter against (by eq, say).

If you are happy with going with option 2, I'll take a stab at implementing it this weekend. What do you think?

Cheers

@karthink
Copy link
Owner

karthink commented Oct 29, 2021 via email

@legendre6891
Copy link
Author

That's a good point! I'm not aware of others at the moment (and TIL that vc-dir could be used as a bookmark!).

Bookmarks are now filtered by `consult-dir-bookmark-handlers`
@legendre6891
Copy link
Author

legendre6891 commented Oct 30, 2021

Hi @karthink, I made a small patch regarding consult-dir-bookmark-handlers above. Would you like to give it a try?

I also added a fbound, since I noticed that project--read-project-list is not defined in OOTB Emacs 27.2 (need to update package.el, which requires an Internet connection).

@legendre6891
Copy link
Author

@karthink bump on this to see if you've had any time to look at it.

Appreciate that pre-Thankgiving November is the busiest part of the year :-) Cheers

@karthink
Copy link
Owner

karthink commented Nov 8, 2021

Looks good! Thank you for the effort. I added a couple of comments.

@legendre6891
Copy link
Author

Thanks @karthink! Looking forward to the addition, if there are no fixes requested (I couldn’t see your comments, so I guess they are in a local/offline branch.) cheers

@karthink
Copy link
Owner

Looks good! Thank you for the effort. I added a couple of comments.

@legendre6891 The comments are part of a review, it should be visible directly above this comment.

@legendre6891
Copy link
Author

Sorry @karthink, I could not see any comments? This is what I see on my screen -- not very familiar with Github, unfortunately

image

@karthink
Copy link
Owner

not very familiar with Github, unfortunately

Neither am I! Here's what I see:

2021-11-10-213801_1068x1154_scrot

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