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 ctrlf-mode-map #64

Closed
wants to merge 4 commits into from
Closed

Conversation

rgrinberg
Copy link
Contributor

Attempt to fix #62.

I'm not sure I fully understand your
comment(my
elisp is weak), but I figure it's easier to continue the discussion
with some concrete code.


Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is exactly the check that I was suggesting. Thanks for taking it on! Just a few small changes:

  • I think we now need to rename the reference to ctrlf--keymap below to ctrlf-mode-map.
  • I think we probably want to keep the (setcdr ctrlf-mode-map nil) call (only in the case where your when clause passes, of course). It makes it so if you remove a binding from ctrlf-mode-map, it actually gets deactivated.
  • We should update the docstring of ctrlf-mode-bindings to say it's deprecated, and probably mark the variable as such (I think this is done with make-obsolete-variable, or perhaps defcustom has an option specially for this).
  • There should be an accompanying README update, and changelog entry.

@raxod502
Copy link
Member

Oh yes, also the CI is currently failing (due to the reference to ctrlf--keymap).

@rgrinberg
Copy link
Contributor Author

Thanks for the review. I think I addressed all your comments.

@rgrinberg
Copy link
Contributor Author

I can make an analogous change for ctrlf-minibuffer-bindings. Should it be called ctrlf-minibuffer-map?

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looks good to me. Yes, doing the same for ctrlf-minibuffer-bindings and naming it ctrlf-minibuffer-map sounds perfect.

CHANGELOG.md Show resolved Hide resolved
ctrlf.el Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Contributor Author

@raxod502 sorry for the delay. should be ready now

@rgrinberg rgrinberg force-pushed the binding-map branch 2 times, most recently from 1d62310 to 354f896 Compare February 27, 2021 22:24
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@raxod502
Copy link
Member

raxod502 commented Mar 7, 2021

Would you mind fixing the CI failures? This also may need a bit of additional consideration with the change ddfbc35 from February.

@raxod502
Copy link
Member

raxod502 commented Jun 6, 2021

This thread is being closed automatically by Tidier because it is labeled with "waiting on response" and has not seen any activity for 90 days. But don't worry—if you have any information that might advance the discussion, leave a comment and I will be happy to reopen the thread :)

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

Successfully merging this pull request may close these issues.

Make minibuffer keymap available
2 participants