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

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

Closed
duianto opened this issue Sep 2, 2016 · 24 comments
Closed

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

duianto opened this issue Sep 2, 2016 · 24 comments
Labels
- Mailling list - Discussion stale marked as a stale issue/pr (usually by a bot) Transient-state

Comments

@duianto
Copy link
Collaborator

duianto commented Sep 2, 2016

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 💻

  • OS: windows-nt
  • Emacs: 24.5.1
  • Spacemacs: 0.105.22
  • Spacemacs branch: develop (rev. 3290c8a)
  • Graphic display: t
  • Distribution: spacemacs
  • Editing style: vim
  • Completion: helm
  • Layers:
(helm emacs-lisp git)
@bmag
Copy link
Collaborator

bmag commented Sep 2, 2016

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
Copy link
Contributor

nixmaniack commented Sep 2, 2016

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
Copy link
Collaborator

bmag commented Sep 2, 2016

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 💻

  • OS: gnu/linux
  • Emacs: 25.1.1
  • Spacemacs: 0.105.22
  • Spacemacs branch: develop (rev. feb4197)
  • Graphic display: t
  • Distribution: spacemacs
  • Editing style: hybrid
  • Completion: helm
  • Layers:
(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
Copy link
Contributor

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
Copy link
Collaborator

TheBB commented Sep 2, 2016

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

@bmag
Copy link
Collaborator

bmag commented Sep 2, 2016

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

👍

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
Copy link
Contributor

@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
Copy link
Contributor

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.

@TheBB TheBB changed the title Transient State, quit/close key(s) Consistency of bindings in transient states (SPC, q, C-g) Sep 2, 2016
@bmag
Copy link
Collaborator

bmag commented Sep 2, 2016

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

@TheBB TheBB changed the title Consistency of bindings in transient states (SPC, q, C-g) Consistency of bindings in transient states (SPC, q, C-g, ESC, etc.) Sep 2, 2016
@nixmaniack
Copy link
Contributor

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
Copy link
Collaborator

bmag commented Sep 2, 2016

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
Copy link
Contributor

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
Copy link
Collaborator Author

duianto commented Sep 3, 2016

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
Copy link
Collaborator

bmag commented Sep 5, 2016

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

  • C-g to be removed from Code Folding TS
  • <SPC> to be removed from Code Folding TS
  • which-key to be disabled while Code Folding TS is active
  • <return> to be removed from Layouts TS
  • q to be added to Layouts TS as exit key (instead of <return>)
  • <escape> in web-mode, ivy and ido TS's: no decision

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
Copy link
Contributor

  • <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:

  • you navigate using n/p
  • then hitting q is like cancel and quit but don't select the layout
  • and hitting <return> selects layout

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
Copy link
Collaborator

bmag commented Sep 6, 2016

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
Copy link
Collaborator Author

duianto commented Sep 6, 2016

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
Copy link
Collaborator

bmag commented Sep 7, 2016

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

@duianto
Copy link
Collaborator Author

duianto commented Sep 7, 2016

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
Copy link
Contributor

braham-snyder commented May 29, 2017

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
Copy link
Collaborator Author

duianto commented May 30, 2017

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.

braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Aug 8, 2017
* if `<escape>` is unbound in the definition of the
  transient-state, then default it to just exit the hydra
  instead of exiting the hydra and additionally sending <escape> to
  the most recently selected buffer

* ref: syl20bnr#6992

(cherry picked from commit 4f1d87d55949cefd2bf46de42f6c2fac43b77626)
braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Aug 8, 2017
* analogous to `RET` in `spacemacs/layouts-transient-state`

* ref: syl20bnr#6992

(cherry picked from commit 8b12944)
braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Aug 8, 2017
* analogous to `RET` in `spacemacs/layouts-transient-state`

* ref: syl20bnr#6992
braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Aug 8, 2017
* if `<escape>` is unbound in the definition of the
  transient-state, then default it to just exit the hydra
  instead of exiting the hydra and additionally sending <escape> to
  the most recently selected buffer

* ref: syl20bnr#6992
braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Oct 7, 2017
* if `<escape>` is unbound in the definition of the
  transient-state, then default it to just exit the hydra
  instead of exiting the hydra and additionally sending <escape> to
  the most recently selected buffer

* ref: syl20bnr#6992

(cherry picked from commit 4f1d87d55949cefd2bf46de42f6c2fac43b77626)
braham-snyder added a commit to braham-snyder/spacemacs that referenced this issue Oct 7, 2017
* analogous to `RET` in `spacemacs/layouts-transient-state`

* ref: syl20bnr#6992

(cherry picked from commit 8b12944)
@duianto
Copy link
Collaborator Author

duianto commented Nov 16, 2017

@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
Copy link

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!

@github-actions github-actions bot added the stale marked as a stale issue/pr (usually by a bot) label Feb 29, 2020
@dalanicolai
Copy link
Contributor

dalanicolai commented Sep 1, 2020

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- Mailling list - Discussion stale marked as a stale issue/pr (usually by a bot) Transient-state
Projects
None yet
Development

No branches or pull requests

6 participants