syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.68k stars 4.89k forks source link

Consistency of bindings in transient states (SPC, q, C-g, ESC, etc.) #6992

Closed duianto closed 4 years ago

duianto commented 8 years ago

Which keys should be bound in the Transient States to quit/close.

Currently these Transient States have the [q] key bound to quit: Buffer Selection Code Fold Error Font Scaling Git Timemachine Transparent MacroStep Window Manipulation Zoom Frame

But these do not have [q] bound to quit: Evil Numbers Layout Move Text Scrolling Workspaces

The Code Folding TS also has C-g and SPC bound to nil, but C-g doesn't seem to be needed, since the other TS's close when C-g is pressed.

The Layouts TS has bound to nil. If the other TS's also should use to quit/close, then C-m (evil-ret) probably should be bound as well, i use it more often than reaching for with the right hands pinky.

I'm not sure if these are all the TS's, but the once above except MacroStep shows up as results when searching the HELM Descbinds SPC ? transient RET

System Info :computer:

(helm emacs-lisp git)
bmag commented 8 years ago

My opinion on the subject: (just an opinion)

q not bound in some TS's: did you execute each TS and confirm that q doesn't quit the TS? Is there a plausible use-case for q to not quit the TS? We should bind q for all TS's that don't have a good use-case for doing otherwise.

Code Folding TS: SPC should be removed. Did you confirm that when the C-g is commented out in the code, C-g still exits the TS? In this case we probably can remove it, though I don't think there's any harm in keeping it.

Layout TS: <return> is inconsistent and should be replaced by q. Anyway, did you confirm that C-m doesn't get automatically re-wired to <return>, thus exiting the TS?

TS's are defined by using the spacemacs|define-transient-state macro, so searching for it via SPC / in Spacemacs root should reveal all existing transient states.

nixmaniack commented 8 years ago

Code Folding TS: SPC should be removed

This was added to disable which-key pop-ups which are really not needed when TS is active. At least in code folding TS, it's normal (at least for me!) to keep the TS open as a reference and navigate around folds, so if you inadvertently hit SPC - which-key pop-up is a bit annoying. Hitting C-g in such scenario closes which-key as well as TS - which was more annoyance!

bmag commented 8 years ago

Hitting C-g in such scenario closes which-key as well as TS

Really? That's different than what I'm experiencing. In scratch buffer, evaluate this code:

(spacemacs|define-transient-state test
  :title "Test TS"
  :foreign-keys run
  :bindings
  ("q" nil :exit t))

Then:

  1. M-x spacemacs/test-transient-state/body: test TS pops up.
  2. SPC and wait: which-key to popup.
  3. C-g: which-key hides itself, TS stays alive.

This is with latest develop branch and updated packages.

System Info :computer:

(helm
 (auto-completion :variables auto-completion-enable-snippets-in-popup nil auto-completion-enable-help-tooltip 'manual company-tooltip-align-annotations t)
 command-log dash evil-cleverparens
 (git :variables git-magit-status-fullscreen t)
 ibuffer imenu-list nlinum org pdf-tools
 (shell :variables shell-default-shell 'shell shell-default-height 30 shell-default-position 'bottom)
 smex syntax-checking version-control yaml c-c++
 (clojure :variables clojure-enable-fancify-symbols nil)
 emacs-lisp
 (latex :variables latex-enable-auto-fill t latex-enable-folding t)
 markdown octave python ruby common-lisp org-tweaks spaceline-tweaks window-purpose window-tweaks themes-megapack
 (theming :variables theming-modifications
          '((spacemacs-dark
             (aw-leading-char-face :foreground "red" :height 3.0))
            (spacemacs-light
             (aw-leading-char-face :foreground "red" :height 3.0))
            (monokai
             (aw-leading-char-face :foreground "red" :height 3.0))
            (flatland
             (aw-leading-char-face :foreground "red" :height 3.0)))))
nixmaniack commented 8 years ago

Hmm. My bad. I had C-g mapped in TS to exit out. So that's what was causing issue when which-key and TS were active.

So if C-g mapping is not required in TS, we can definitely remove it.

Although, I would probably keep SPC to nil to not allow which-key pop-ups in code-folding TS.

TheBB commented 8 years ago

Closing #6991 in favour of this. I guess the topic is consistency of bindings in transient states.

bmag commented 8 years ago

So if C-g mapping is not required in TS, we can definitely remove it.

:+1:

keep SPC to nil to not allow which-key pop-ups in code-folding TS

That doesn't take care of other prefixes, e.g. C-x. Can't we use :on-enter and :on-exit to temporarily disable which-key? For example:

(spacemacs|define-transient-state test
  :title "Test TS"
  :foreign-keys run
  :on-enter (spacemacs/toggle-which-key-off)
  :on-exit (spacemacs/toggle-which-key-on)
  :bindings
  ("q" nil :exit t))

I wonder if this should the default TS behavior.

nixmaniack commented 8 years ago

@TheBB Rename the issue then to "Consistency of key bindings(SPC, q) in transient states".

Also, it'd be better if we can pull the information present in the other thread here to have it in one place. bmag and duianto have good points/opinions on usage of SPC.

nixmaniack commented 8 years ago

Can't we use :on-enter and :on-exit to temporarily disable which-key?

Nice. πŸ‘ Didn't think of it. I can make the PR if everyone agrees on disabling which-key when code-folding TS is active.

bmag commented 8 years ago

The original rational for disabling which-key was that quitting it with C-g also quitted the TS, right? But that's fixed by not binding C-g in the TS. So... I'm not sure I understand why we want to disable which-key at this point.

P.S. I should have included it in my previous comment, but didn't think of it then

nixmaniack commented 8 years ago

The original rational for disabling which-key was that quitting it with C-g also quitted the TS, right?

Actually, it's only for code-folding TS and the rationale behind this was (a) which-key isn't really needed when we have persistent TS and (b) it was annoyance to have it pop up on SPC where it's really not required.

For other transient states this might not be the case.

From consistency point of view, if we're keeping SPC in all TS, then it should be ok - not much would affect.

bmag commented 8 years ago

The thing is that which-key is still helpful for non-TS binding in this case, at least theoretically. But you're actually using this specific TS, while I'm just hypothesising. If using SPC prefixed keys is unlikely during the TS, then I agree we can disable which-key.

nixmaniack commented 8 years ago

I haven't found a use case(yet!) where I have used SPC/which-key when TS is active. I think TS serves the purpose of which-key by focusing on subset of useful keys. I will let others weigh in on this point.

duianto commented 8 years ago

did you execute each TS and confirm that q doesn't quit the TS?

When any key that isn't defined in a TS is pressed then the TS closes. But that key behaves as if the TS wasn't open. So when q is pressed in these TS's: Evil Numbers Layout Move Text Scrolling Workspaces

then the TS closes, but the next key that's pressed starts recording a keyboard macro to that key.

Did you confirm that when the C-g is commented out in the code, C-g still exits the TS? In this case we probably can remove it, though I don't think there's any harm in keeping it.

C-g still closes the Code Fold TS when it's commented out. It should most likely be removed, it's better to reduce code whenever possible, and only add code if/when it's needed.

There seems to be two places where Code Fold TS's are defined, the one that opens by default when SPC z . is pressed is defined here: https://github.com/syl20bnr/spacemacs/blob/develop/layers/%2Bdistributions/spacemacs-bootstrap/packages.el#L176

  ;; fold transient state
  (when (eq 'evil dotspacemacs-folding-method)
    (spacemacs|define-transient-state fold
      :title "Code Fold Transient State"
      :doc "
 Close^^          Open^^              Toggle^^             Other^^
 ───────^^──────  ─────^^───────────  ─────^^────────────  ─────^^───
 [_c_] at point   [_o_] at point      [_a_] around point   [_q_] quit
 ^^               [_O_] recursively   ^^
 [_m_] all        [_r_] all"
      :foreign-keys run
      :bindings
      ("a" evil-toggle-fold)
      ("c" evil-close-fold)
      ("o" evil-open-fold)
      ("O" evil-open-fold-rec)
      ("r" evil-open-folds)
      ("m" evil-close-folds)
      ("q" nil :exit t)
      ("C-g" nil :exit t)
      ("<SPC>" nil :exit t)))
  (spacemacs/set-leader-keys "z." 'spacemacs/fold-transient-state/body)

the other place where a Code Fold TS is define: https://github.com/syl20bnr/spacemacs/blob/develop/layers/%2Bspacemacs/spacemacs-editing/packages.el#L222

      (spacemacs|define-transient-state fold
        :title "Code Fold Transient State"
        :doc "
 Close^^            Open^^             Toggle^^         Goto^^         Other^^
 ───────^^───────── ─────^^─────────── ─────^^───────── ──────^^────── ─────^^─────────
 [_c_] at point     [_o_] at point     [_a_] at point   [_n_] next     [_s_] single out
 [_C_] recursively  [_O_] recursively  [_A_] all        [_p_] previous [_R_] reset
 [_m_] all          [_r_] all          [_TAB_] like org ^^             [_q_] quit"
        :foreign-keys run
        :on-enter (unless (bound-and-true-p origami-mode) (origami-mode 1))
        :bindings
        ("a" origami-forward-toggle-node)
        ("A" origami-toggle-all-nodes)
        ("c" origami-close-node)
        ("C" origami-close-node-recursively)
        ("o" origami-open-node)
        ("O" origami-open-node-recursively)
        ("r" origami-open-all-nodes)
        ("m" origami-close-all-nodes)
        ("n" origami-next-fold)
        ("p" origami-previous-fold)
        ("s" origami-show-only-node)
        ("R" origami-reset)
        ("TAB" origami-recursively-toggle-node)
        ("<tab>" origami-recursively-toggle-node)
        ("q" nil :exit t)
        ("C-g" nil :exit t)
        ("<SPC>" nil :exit t))
      ;; Note: The key binding for the fold transient state is defined in
      ;; evil config

i'm not sure where the "evil config" is?

Layout TS: <return> is inconsistent and should be replaced by q. Anyway, did you confirm that C-m doesn't get automatically re-wired to <return>, thus exiting the TS?

C-m does not get re-wired to the bound <return> key. When C-m is pressed then the cursor moves to the line below, just like C-m does when a TS isn't open. The TS does close, but for the same reason as in the first answer. Any key that isn't bound in a TS exits the TS.

Sorry for taking so long to answer, i wrote answers to these questions yesterday, but i had the comment panel in Preview mode and got sidetracked. When i got back to this page, then the preview mode looked like i already had pressed the Comment button. Well at least some more info got included the second time around.

bmag commented 8 years ago

So, from my understanding, these are the current decisions: (including discussion from #6991)

We're waiting a few days to let other people voice their suggestions (if any), then one of us can implement the changes. (I wanted to say I'll do it, but not sure I'll have the time)

Re <return> and C-m: If we bind RET instead of <return>, it should catch both the return button and C-m. However, the better solution here is to remove the binding from the TS entirely.

Re two Case Folding TS's: that's expected. The one that is really used depends on dotspacemacs-folding-method.

nixmaniack commented 8 years ago
  • <return> to be removed from Layouts TS
  • q to be added to Layouts TS as exit key (instead of <return>)

I didn't follow this before hence voicing my opinion now.

I find following behaviour more intuitive in Layouts TS(when pressed SPC l). You can think of it as preview mode for Layouts:

C-g as well can be used in place of q but I find single keystroke simpler.

Not sure if this feature request belongs here, but since we're talking of defining conventions, might help. Thoughts?

bmag commented 8 years ago

I see no harm in keeping <return>, so how about this: we don't remove <return>, but we replace it with RET (to catch also C-m) and add q (for consistency).

There was a discussion a month ago about refactoring the Layouts TS, but it kind of fizzled away: #6763.

duianto commented 8 years ago

If we bind RET instead of , it should catch both the return button and C-m.

Just tested it and replacing ("<return>" nil :exit t), with ("RET" nil :exit t), stops C-m from jumping to the next line, and both the C-m and the Return keys still close the Layouts TS.

bmag commented 8 years ago

@duianto yes, that was the intention. I can't figure out if you consider this a good thing or a bad thing.

duianto commented 8 years ago

Sorry for being unclear, it's good that C-m works the same as . As i stated in the initial post. I use C-m more often, because it's easier to reach than having to stretch the right hands pinky to reach the return key. On this keyboard there are two keys between the home row position of the right hands pinky and the return key.

If the RET key is going to be bound to exit a TS then it's also good that C-m doesn't move the cursor like it currently does.

I'm fine with both cases, if the return key does or does not close a TS. but it's probably a good idea to be consistent in most/all of the TS's.

braham-snyder commented 7 years ago

Should q for quitting be added directly within spacemacs-define-transient-state, or should it be added to each of the individual transient state definitions currently lacking bindings for q?

I think <escape> probably ought to be equivalent to q for transient states (at least for vim-style controls?) -- or at the very least, should not pass through many transient states. Spacemacs' current default causes <escape> to both pass through and exit, for example, the layout transient state, which will, e.g., bury a magit buffer if it's focused.

Regarding @nixmaniack's suggestions for the layouts-transient-state:

  • then hitting q is like cancel and quit but don't select the layout
  • and hitting selects layout
  • C-g as well can be used in place of q but I find single keystroke simpler.

Similarly, perhaps spacemacs/time-machine-transient-state ought to have a binding for q like this:

       ("q" (lambda ()
              (when (bound-and-true-p git-timemachine-mode)
                (git-timemachine-quit))) :exit t)

instead of the current, unconditional quit on-exit:

       :on-exit (when (bound-and-true-p git-timemachine-mode)
                  (git-timemachine-quit)))

enabling <return>1 to quit the transient state without closing the current time-machine buffer. This would allow, for example, searching that buffer without accidentally triggering the transient-state's unrelated bindings on n and N.

1: and/or C-g? it might be useful to distinguish it from q -- like emacs does when you attempt to quit with unsaved changes -- but perhaps not useful enough to warrant a breaking change?

edit: regarding <escape>, the following redefinition of spacemacs|define-transient-state appears to work as intended (edit: by rebinding it to nil :exit t), but I'm still not sure whether that's where it should really be left (may or may not at some point make it synonymous with q): https://github.com/braham-snyder/spacemacs/tree/fix.transient-states-escape

edit: spacemacs/macrostep-transient-state also maybe ought to use that <return> to retain or q to quit keybinding convention?

edit: FWIW, I'm happy thus far with my code regarding my prior two edits -- macrostep code is a bit odd but appears to work well: https://github.com/braham-snyder/spacemacs/tree/feat.macrostep-exit-hold-expansions

edit: Sorry about my rebasing/cherry-picking spam below -- I really shouldn't be modifying public git history in the first place... that said, still very happy with both of my modifications. I may or may not bump this issue at some point to gauge interest in a PR (or two).

duianto commented 7 years ago

The magit buffer closing, when one just want to close the layout transient state, is an unexpected behaviour, and that should be fixed. I don't know enough about the other suggestions to have an informed opinion.

duianto commented 6 years ago

@braham-snyder I do not consider the rebasing/cherry-picking to be spam, they are just progress reports πŸ˜„. It's great that your finding better (possibly more consistent) keybindings for exiting from the transient states πŸ‘

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!

dalanicolai commented 4 years ago

Should we consider to use (transient-bind-q-to-quit) by default on Spacemacs startup (see this comment)? I do not really understand all that is discussed here, but I found this function and I have placed it in my user-config.

I am posting it here because the comment seems to be newer than any comment here. So maybe this issue is worth reconsidering? (then please re-open the issue).

Personally, I have found no unwanted effects. Maybe you can try if this function causes any unwanted effects for you? (I do find it a little annoyance to, inconsistently, quit transients with C-g)