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

fix(tuareg.el): Fix and improve Emacs bindings #309

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

erikmd
Copy link
Contributor

@erikmd erikmd commented Aug 31, 2023

(following the discussion in #273)

Cc @Chris00 @monnier

For a quick recap of the changes and their motivation, see also the table from comment:

ocaml/merlin#1386 (comment)

@monnier monnier self-requested a review August 31, 2023 22:00
Copy link
Contributor

@monnier monnier left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.
BTW, the C-c ` binding should be made unnecessary by making C-x ` work (via next-error-function).

@erikmd
Copy link
Contributor Author

erikmd commented Aug 31, 2023

Hi @monnier!

Actually I tried to be as conservative as possible, and I'm not sure it is needed to shrink even further the keymap!

Indeed for example, I checked that C-x ‘ and C-c ‘ are both bound in latex-mode from AUCTeX:

(let ((test (get-buffer-create "test")))
  (set-buffer test)
  (latex-mode)
  (concat
   (describe-key-briefly (kbd "C-c `"))
   "\n"
   (describe-key-briefly (kbd "C-x `"))))

"C-c ` runs the command TeX-next-error  
C-x ` runs the command TeX-next-error"

@monnier
Copy link
Contributor

monnier commented Sep 1, 2023 via email

@erikmd
Copy link
Contributor Author

erikmd commented Sep 6, 2023

@Chris00 @monnier are you happy with the current version of this PR?

— I agree with your earlier comment about the fact we could even unbind "C-c `" but IMHO it is not critical to keep it (all the more as it might surprise some users that use "C-c `" instead of the generic "C-x `")…

@monnier
Copy link
Contributor

monnier commented Sep 6, 2023

Yes, I already approved the change :-)

@erikmd
Copy link
Contributor Author

erikmd commented Sep 18, 2023

Hi, it seems the merlin counterpart of this PR will be merged soon; what about this one?

Anyway, I'd be happy to know @Chris00's opinion, given the three PRs I opened (summarized in this comment) are basically a follow-up of the wrap-up issue he had opened (ocaml/merlin#1386)

@monnier monnier merged commit f03eb1c into ocaml:master Oct 2, 2023
@erikmd erikmd deleted the fix-bindings branch October 2, 2023 08:19
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