Skip to content

tweak(default): better vanilla multiple-cursor bindings#7342

Open
UndeadKernel wants to merge 1 commit into
doomemacs:masterfrom
UndeadKernel:vanilla-multiple-cursors
Open

tweak(default): better vanilla multiple-cursor bindings#7342
UndeadKernel wants to merge 1 commit into
doomemacs:masterfrom
UndeadKernel:vanilla-multiple-cursors

Conversation

@UndeadKernel

Copy link
Copy Markdown
Member

Better default bindings for multiple-cursors in vanilla emacs.

A transient map takes care of repeating useful commands without having to input entire shortcut sequences.
Added the binding "C-+" to create a new cursor like what is selected (following the recommendation of the package's author).


  • [x ] I searched the issue tracker and this hasn't been PRed before.
  • [ x] My changes are not on the do-not-PR list for this project.
  • [ x] My commits conform to the git conventions.
  • [ x] Any relevant issues or PRs have been linked to.

@UndeadKernel UndeadKernel requested a review from a team as a code owner August 8, 2023 13:39
@LemonBreezes

LemonBreezes commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

Hi. I'll look over this more later today but I disagree with binding C-- to contracting the expanded region because that overrides negative-argument. Maybe we can make a repeat map instead so that C-- is only bound transiently, since it's only ever called after first expanding the region.

@UndeadKernel

UndeadKernel commented Aug 8, 2023

Copy link
Copy Markdown
Member Author

The reason for changing "C--" to "contract region" is that there are other ways to set negative arguments and setting negative arguments is not as common as working with the expansion and contraction of regions. I feel that I only sporadically use negative arguments and, when I need to, there is always "C-u -".

@LemonBreezes

LemonBreezes commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

The reason for changing "C--" to "contract region" is that there are other ways to set negative arguments and setting negative arguments is not as common as working with the expansion and contraction of regions. I feel that I only sporadically use negative arguments and, when I need to, there is always "C-u -".

With respect to expand-region, doesn't this accomplish the same effect without binding that key globally?

(setq expand-region-fast-keys-enabled nil)
(add-hook 'repeat-mode-hook
          (lambda nil
            (defvar expand-region-repeat-map (make-sparse-keymap)
              "Defined by `define-repeat-map'.")
            (progn
              (define-key expand-region-repeat-map (kbd "=") #'er/expand-region)
              (define-key expand-region-repeat-map (kbd "-") #'er/contract-region)
              (define-key expand-region-repeat-map (kbd "C-=") #'er/expand-region)
              (define-key expand-region-repeat-map (kbd "C--") #'er/contract-region)
              (put 'er/expand-region 'repeat-map 'expand-region-repeat-map)
              (put 'er/contract-region 'repeat-map 'expand-region-repeat-map))))

Or if you use the define-repeat-map package, the above block is just

(setq expand-region-fast-keys-enabled nil)
(define-repeat-map expand-region
  ("=" er/expand-region
   "-" er/contract-region
   "C-=" er/expand-region
   "C--" er/contract-region))

Similarly, the transient map you wrote can be condensed to

(define-repeat-map multiple-cursors
    ("n" mc/mark-next-like-this
     "p" mc/mark-previous-like-this
     "N" mc/unmark-next-like-this
     "P" mc/unmark-previous-like-this))

So in my opinion, we should add

(use-package! repeat
  :init
  (add-hook 'doom-first-input-hook #'repeat-mode)
  :config
  (map! :map help-map "C-r" #'describe-repeat-maps)
  (setq repeat-exit-key "TAB"
        repeat-echo-mode-line-string nil))

and

(package! define-repeat-map :recipe (:repo "https://tildegit.org/acdw/define-repeat-map.el.git"))

and just condense that. Or we should use repeat-mode directly.

@UndeadKernel

Copy link
Copy Markdown
Member Author

I agree that indeed "C--" could be a transient binding. I'll look into this.

@UndeadKernel UndeadKernel force-pushed the vanilla-multiple-cursors branch from 64dc971 to 8063d36 Compare August 9, 2023 14:10
@UndeadKernel

Copy link
Copy Markdown
Member Author

I propose to not use other minor modes (e.g., repeat-mode) and external packages to create transients for multiple-cursors and expand-region. I don't like the idea of relying on yet more minor modes when it's possible to do it without any for just a little boiler plate.

Note that the "boiler plate" for the transients used in multiple-cursors (MC) is needed. Because of the way MC handles the creation of multiple cursors, all custom bindings for MC need to be named functions.

@LemonBreezes

LemonBreezes commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

I propose to not use other minor modes (e.g., repeat-mode) and external packages to create transients for multiple-cursors and expand-region. I don't like the idea of relying on yet more minor modes when it's possible to do it without any for just a little boiler plate.

Note that the "boiler plate" for the transients used in multiple-cursors (MC) is needed. Because of the way MC handles the creation of multiple cursors, all custom bindings for MC need to be named functions.

I can understand why you would not want to pull in the define-repeat-map macro but I still believe that repeat-mode is a superior solution to using transient maps as currently written because:

  1. repeat-mode provides a visual hint to the user indicating that a repeat map is currently active and shows which keys are in the transient map. With the lower level transient-map solution, a user does not have any indication that a transient map is active. The visual hint can also be integrated with which-key or embark using https://github.com/karthink/repeat-help if desired.
  2. repeat-mode also provides a describe-repeat-maps command and allows the user to toggle the use of repeat maps globally using (repeat-mode -1), etc., so it provides an overall more complete and unified user experience for transient maps.
  3. repeat-mode is in use by many packages such as tab-bar (built into Emacs) and diff-hl so simply enabling repeat-mode without further configuration will enable a lot of convenient repeat maps that package developers thought of when writing their packages.

@UndeadKernel

UndeadKernel commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

@hlissner, I'd like to have your opinion on this matter: As a summary, do you see any issue in using (and enabling) a new minor-mode to create transient bindings (for vanilla emacs only)? I originally created some transient bindings for the multiple-cursors minor mode using transient-mode-map. @LemonBreezes made a good point that we could also create those transient bindings configuring and enabling the minor mode repeat-mode. What's your take on this? If you don't have any preference, I suppose I'd go with @LemonBreezes's suggestion of configuring and enabling repeat-mode.

@LemonBreezes, is repeat-mode a global minor mode or does it have to be enabled when visiting new buffers?

@LemonBreezes

Copy link
Copy Markdown
Contributor

@hlissner, I'd like to have your opinion on this matter: As a summary, do you see any issue in using (and enabling) a new minor-mode to create transient bindings? I originally created some transient bindings for the multiple-cursors minor mode using transient-mode-map. @LemonBreezes made a good point that we could also create those transient bindings configuring and enabling the minor mode repeat-mode. What's your take on this? If you don't have any preference, I suppose I'd go with @LemonBreezes's suggestion of configuring and enabling repeat-mode.

@LemonBreezes, is repeat-mode a global minor mode or does it have to be enabled when visiting new buffers?

Yup, repeat-mode is a global minor mode.

Better default (transient) bindings for multiple-cursors (mc) and
expand-region (er) in vanilla emacs.

A transient map takes care of repeating useful commands without having
to input entire shortcut sequences.

Added the binding "C-+" to create a new cursor like what is
selected (following the recommendation of the package's author).
@UndeadKernel UndeadKernel force-pushed the vanilla-multiple-cursors branch from 8063d36 to acb2fc6 Compare March 27, 2024 11:45
@hlissner hlissner added re:keybinds Changes to or discussion about Doom's keybinds is:tweak Code changes that affects user-facing behavior re:defaults Regarding Doom's defaults labels Sep 7, 2024
@hlissner

hlissner commented Sep 7, 2024

Copy link
Copy Markdown
Member

Sorry for the tremendously late response, but I'd prefer we use repeat-mode. In fact, it's already on my TODO list to enable it by default for non-evil users, and perhaps even extend it for evil or leader keys. That said, I don't think it should be this module's responsibility to enable it, only exploit it if it happens to be enabled.

@hlissner hlissner added this to the modules backlog milestone Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

is:tweak Code changes that affects user-facing behavior re:defaults Regarding Doom's defaults re:keybinds Changes to or discussion about Doom's keybinds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants