evil-unimpaired: Switch from normal state to motion state

[sorawee]

This change will make buffers with the motion state
(e.g., spacemacs homepage) be able to use the
keybindings. Note that the normal state will inherit
the motion state map anyway, so there's no need to set
the normal state map separately.

[duianto]

I'm not seeing any change when I apply this PR as a patch.

In the Spacemacs home buffer SPC b h.
[ still only shows five bindings:
( -> evil-previous-open-paren
[ -> evil-backward-section-begin
] -> evil-backward-section-end
s -> evil-prev-flyspell-error
{ -> evil-previous-open-brace

] calls the equivalent bindings with close instead of open, forward instead of backward, next instead of prev/previous.

The help buffer also calls the previous commands:
[ help-go-back
] help-go-forward

I use the current help bindings to navigate between help pages, but I see that gb and gf also are bound to the same commands, So I'll adapt.

It's better to have consistent key bindings where ever possible.

System Info 💻

• OS: windows-nt
• Emacs: 26.2
• Spacemacs: 0.300.0
• Spacemacs branch: develop (rev. 9a2699571)
• Graphic display: t
• Distribution: spacemacs
• Editing style: vim
• Completion: helm
• Layers:

(autohotkey helm auto-completion emacs-lisp git github multiple-cursors org spell-checking themes-megapack treemacs version-control)

• System configuration features: XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS THREADS LCMS2

[sorawee]

@duianto are you sure you applied the patch correctly? Here's a GIF showing before and after I apply the patch. It seems to work as I expect.

[duianto]

The bindings are not being applied automatically on startup for me.

When I select them and evaluate region , e r, then they work in the spacemacs buffer, but in the Help buffer [ and ] still call help-go-back and help-go-forward.

Both buffers are in motion state.

evil-state is a variable defined in ‘evil-vars.el’.
Its value is ‘motion’
Local in buffer *spacemacs*; global value is nil

  Automatically becomes permanently buffer-local when set.

evil-state is a variable defined in ‘evil-vars.el’.
Its value is ‘motion’
Local in buffer *Help*; global value is nil

  Automatically becomes permanently buffer-local when set.

Applying a PR as a patch

This is how I'm applying all patches before testing and pushing them to the develop branch.

I have these functions/bindings for downloading a PR as a .patch:

    (defun open-changelog-develop ()
      (interactive)
      (find-file "~/.emacs.d/CHANGELOG.develop"))

    (defvar github-pr-patch-dir nil)

    ;; Ubuntu directory
    ;; (setq github-pr-patch-dir "~/projects/spacemacs/prs/")
    ;; Windows 10 directory
    (setq github-pr-patch-dir "C:\\Users\\username\\projects\\spacemacs\\prs\\")

    (defun open-github-pr-patch-dir ()
      (interactive)
      (let ((dir (or github-pr-patch-dir
                     (read-file-name "Open dir: "))))
        (dired dir)))

    ;; Ask for a Spacemacs PR number, and download the PR as a .patch
    ;; TODO add an option to apply the PR to either:
    ;; the current branch, ask if the branch isn't develop,
    ;; or apply it to a new branch with the name of the PR number
    (defun save-github-pr-to-patch-file ()
      "Ask for the PR number of a Spacemacs PR on github,
either ask for a location where the PRs patch file should be stored,
or save directly to the `github-pr-patch-dir' location if it's set."
      (interactive)
      (let* (;; (url (read-string "Github PR URL: "))
             ;; (pr-nr (substring url (1+ (string-match "/[[:digit:]]*$" url))))
             (pr-url-prefix "https://github.com/syl20bnr/spacemacs/pull/")
             (pr-nr (read-string "Github Spacemacs PR nr: "))
             (patch-suffix ".patch")
             ;; (filename (concat pr-nr patch-suffix))
             (dir (or github-pr-patch-dir
                      (read-file-name "Save PR patch file in: ")))
             ;; (full-file-path (concat dir filename)))
             (filename (concat pr-url-prefix pr-nr patch-suffix))
             (full-file-path (concat dir pr-nr patch-suffix)))
        ;; (url-copy-file (concat url patch-suffix) full-file-path)))
        ;; (message "%s %s" filename full-file-path)
        (url-copy-file filename full-file-path 'replace)))

    (spacemacs/declare-prefix " op" "PRs")
    (spacemacs/set-leader-keys
      "opc" 'open-changelog-develop
      "opd" 'open-github-pr-patch-dir
      "ops" 'save-github-pr-to-patch-file)

When the patch has been downloaded, then I open the spacemacs .emacs.d project in magit:
SPC p p (with .emacs.d selected) M-g

And apply the patch with w w, and restart.

[sorawee]

Oh, the help doesn't work for me as well because [ and ] are overwritten. It also doesn't work in a lot of other places like magit-status-mode-map, magit-diff-mode-map, message-buffer-mode-map, etc due to the same reason. I personally have these code to fix these maps for me:

  (defmacro fix-bracket (state map)
    `(progn
       (evil-define-key ,state ,map (kbd "[") nil)
       (evil-define-key ,state ,map (kbd "]") nil)
       (evil-define-key ,state ,map (kbd "[ b") 'previous-buffer)
       (evil-define-key ,state ,map (kbd "] b") 'next-buffer)
       (evil-define-key ,state ,map (kbd "[ f") 'evil-unimpaired/previous-filmode)
       (evil-define-key ,state ,map (kbd "] f") 'evil-unimpaired/next-file)
       (evil-define-key ,state ,map (kbd "[ w") 'previous-multiframe-window)
       (evil-define-key ,state ,map (kbd "] w") 'next-multiframe-window)))

  (with-eval-after-load 'message-buffer-mode
    (fix-bracket 'normal message-buffer-mode-map))

  (with-eval-after-load 'magit
    (fix-bracket 'normal magit-status-mode-map)
    (fix-bracket 'normal magit-diff-mode-map))

I guess one way to put it is that this PR focuses on only mode maps that don't override [ and ].

[duianto]

The evil-unimpaired bindings don't seem to be loaded on startup.

I added:

(message "Evil Unimpaired Bindings")

before the comment:

spacemacs/layers/+spacemacs/spacemacs-evil/local/evil-unimpaired/evil-unimpaired.el

Line 83 in 6f34185

;; from tpope's unimpaired

But the message doesn't appear in the messages buffer on startup.

This seems to be where the package is called:

spacemacs/layers/+spacemacs/spacemacs-evil/packages.el

Lines 337 to 339 in 6f34185

	(defun spacemacs-evil/init-evil-unimpaired ()
	;; No laziness here, unimpaired bindings should be available right away.
	(use-package evil-unimpaired))

The comment says that they should be available right away, but it doesn't seem to happen.

If I add:

    :init
    (message "init-evil-unimpaired: :init Evil Unimpaired Bindings")
    :config
    (message "init-evil-unimpaired: :config Evil Unimpaired Bindings")

in (use-package evil-unimpaired then both messages appear in the messages buffer.

[duianto]

I think I found the issue, there's also an evil-unimpaired.el file in the elpa dir:
c:/Users/username/.emacs.d/elpa/26.2/develop/evil-unimpaired-20190514.162633/evil-unimpaired.el

But the PR changes the local version:
c:/Users/username/.emacs.d/layers/+spacemacs/spacemacs-evil/local/evil-unimpaired/evil-unimpaired.el

Adding a message in the elpa dir version shows it in the messages buffer, so that's where the bindings are loaded from.

[duianto]

Ok now I think I understand. The elpa dir package is generated from the local version.
When I removed evil-unimpaired-* from the elpa dir and restarted. then the elpa dir version is installed.

And now this PRs changes are working in the spacemacs buffer:

[duianto]

Do you want to update the PR to support the other modes? Or should we apply it as is for now?

[sorawee]

I'd rather be conservative, so I think we should apply as it is.

[duianto]

Since they keys don't work in the help buffer, the commit message need to be updated.

And it might be helpful to mention evil-unimpaired in the commit subject, then it's easy to see from the commit messages what the state change is referring to.

I made a possible rewrite below, with these changes:

• Added evil-unimpaired to the subject.
• Removed help from the buffers with motion state examples (are there other buffers than the Spacemacs home buffer where they will work?)
• Changed spacemacs homepage to spacemacs home buffer
• Mentioned that the change applies to the evil-unimpaired navigation key bindings.

Before

Switch from normal state to motion state

This change will make buffers with the motion state
(e.g., help and spacemacs homepage) be able to use the
keybindings. Note that the normal state will inherit
the motion state map anyway, so there's no need to set
the normal state map separately.

After

evil-unimpaired switch normal to motion state

This change will make buffers with the motion state
(e.g., spacemacs home buffer) be able to use the
evil-unimpaired navigation key bindings.

Note that the normal state will inherit the motion 
state map anyway, so there's no need to set the 
normal state map separately.

Changelog.develop

We have also started suggesting that PR authors add an entry in the changelog.develop file, it's optional, but it helps if the PR author writes it since they are most familiar with what the PR changes.

A possible entry for this PR could be:

  - Added =evil-unimpaired= navigation keys prefixed by ~[~ and ~]~ to the
    Spacemacs home buffer (thanks to Sorawee Porncharoenwase)

in the section:

*** Distribution changes
- Key bindings:

Would you like to make any changes or are the suggestions acceptable?

[sorawee]

Is it possible to change a commit message? (I guess I can force push, but I'm not sure if that's a good idea).

But yes, all of these look good to me.

[duianto]

I can make the changes before pushing.

[sorawee]

Please make the changes then! Thanks again :)

[duianto]

Thank you for contributing to Spacemacs!
The PR has been cherry-picked into the develop branch, you can safely delete your branch.

I guess I can force push, but I'm not sure if that's a good idea

It's fine to force push to a PR, as long as you don't change your forks branch name.
If you need to change the forks branch name, then you might have to open a new PR.

A PR can be changed by a collaborator like this:
When testing PRs, they can be downloaded as a .patch file, by adding .patch to the end of the PRs url.
In this case: https://github.com/syl20bnr/spacemacs/pull/11787.patch

When it's downloaded it can be applied it to the current Spacemacs branch in Magit with w w.

Then any necessary modifications can be made and added to the commit with:
commit, extend c e.