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 bqn-comint-bring #80

Closed
wants to merge 2 commits into from
Closed

Add bqn-comint-bring #80

wants to merge 2 commits into from

Conversation

slotThe
Copy link
Contributor

@slotThe slotThe commented Sep 8, 2024

Lots of other major modes with REPL-like functionality have some way to toggle between it and the buffer it's associated to. This is something I missed from bqn-mode, so here we are.

Additionally, there is a breaking change lurking here, technically: the BQN comint process is now associated to a particular buffer, instead of being global. In my opinion, this is much more natural anyways, but YMMV. Of course, this can always be made into a backwards-compatible toggle.

Commit Summary

Create a unique BQN comint process per buffer

Instead of one unique *BQN* buffer, create a new one for every buffer. This feels more intuitive than all sessions sharing the global instance, especially when using system functions that actually talk to the OS—which directory one is in matters.

Add bqn-comint-bring

Toggle between the comint buffer and its associated buffer. Bound to C-c C-z by default, as this seems to be quite common among other major modes.

Instead of one unique *BQN* buffer, create a new one for every buffer.
This feels more intuitive than all sessions sharing the global instance,
especially when using system functions that actually talk to the
OS—which directory one is in matters.
Toggle between the comint buffer and its associated buffer. Bound to
C-c C-z by default, as this seems to be quite common among other major
modes.
Comment on lines +319 to +323
(let* ((pref (bqn--comint-prefix))
(buf (or (buffer-file-name) (buffer-name))))
(if (string-prefix-p pref buf)
buf
(concat pref buf bqn--comint-suffix))))
Copy link
Contributor Author

@slotThe slotThe Sep 8, 2024

Choose a reason for hiding this comment

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

One could also use some internal state to keep track of comint buffer–buffer associations, but I figured since this is the only state that's currently needed this would be a bit overkill

@AndersonTorres
Copy link
Member

Hum...
There are some minor rewording and split I want to do in this patch. I put my modifications here. Can you please take a look?

https://github.com/museoa/bqn-mode/tree/patches

@@ -266,6 +266,7 @@ BQN buffers (or recreate them)."
(when bqn-glyph-map-modifier
(set-keymap-parent bqn-mode-map
(make-composed-keymap prog-mode-map bqn--glyph-map)))
(keymap-set bqn-mode-map "C-c C-z" #'bqn-comint-bring)
Copy link
Member

Choose a reason for hiding this comment

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

I split the keymap modifications because I think it is a little bit controversial to bind keys "by default".

https://github.com/positron-solutions/user-keys?tab=readme-ov-file#please-package-authors

Copy link
Contributor Author

@slotThe slotThe Sep 10, 2024

Choose a reason for hiding this comment

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

I thought a little bit about this, but since keybindings of the form C-c C-«letter» are reserved for major modes I figured it was fine. I think that it helps with discoverability, personally, and unbinding a keybinding from a keymap is not all that difficult. Though I don't feel too strongly either way and am fine with dropping this part if you'd prefer

EDIT: I now see that the link you posted talks about this in some length in another place. As I said, no strong opinions from me

@slotThe
Copy link
Contributor Author

slotThe commented Sep 10, 2024

Hum... There are some minor rewording and split I want to do in this patch. I put my modifications here. Can you please take a look?

https://github.com/museoa/bqn-mode/tree/patches

Looks fine to me, thanks! 👍

@slotThe
Copy link
Contributor Author

slotThe commented Sep 10, 2024

New melpazoid warnings from this PR seem to be the following:

2 issues found:
269:3: error: You should depend on (emacs "29.1") or the compat package if you need `keymap-set'.
471:3: error: You should depend on (emacs "29.1") or the compat package if you need `keymap-set'.

If we decide to keep the commit, one can also user the (now deprecated) define-key if keeping compatibility with older Emacs releases is desired

@AndersonTorres
Copy link
Member

If we decide to keep the commit, one can also user the (now deprecated) define-key if keeping compatibility with older Emacs releases is desired

Wouldn't compat be a better compromise?
I prefer to raise the base version, but living on the edge is not always sane.

@AndersonTorres
Copy link
Member

Looks fine to me, thanks! 👍

Rebased, thanks!

Solved at https://github.com/museoa/bqn-mode/releases/tag/2024-09-10

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