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

evil-collection-integration.el: advice show-paren-function doesn't highlight opening bracket #79

Closed
edkolev opened this issue Jan 19, 2018 · 9 comments

Comments

@edkolev
Copy link
Member

edkolev commented Jan 19, 2018

Hi, and again, thanks for this package!

I ran into the following issue - in normal state, show-paren-mode works when the point is on an opening bracket ( (the closing bracket is highlighted), but doesn't work when the point is on a closing bracket (the opening bracket is not highlighted).

To reproduce:

  • use this init.el
(require 'package)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/") t)
(add-to-list 'package-archives '("org" . "http://orgmode.org/elpa/") t)

(package-initialize)

;; use-package
(setq use-package-enable-imenu-support t)
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))
(eval-when-compile
  (require 'use-package))
(setq use-package-verbose nil)

(use-package evil
  :ensure t
  :init
  (setq evil-want-integration nil)
  :config
  (evil-mode)
  (use-package evil-collection
    :ensure t
    :init
    (evil-collection-init)))
  • start emacs
  • M-x show-paren-mode
  • run :new then enter (abc)
  • put cursor on (, works as expected:
    screen shot 2018-01-19 at 22 15 03
  • put cursor on ) - doesn't work as expected because the opening bracket is not highlighted
    screen shot 2018-01-19 at 22 15 12

Thanks!

@Ambrevar
Copy link
Collaborator

I haven't tried your recipe, but isn't it the expected behaviour?
What do you get with (setq show-paren-when-point-inside-paren t)?

I don't think evil-collection fiddles at all with show-paren-mode. I could be wrong though.

@jojojames
Copy link
Collaborator

@edkolev It does the same thing without evil-collection right?

I did a quick check with evil only and the same behavior exists (I believe... someone else please check. :))

I think you're right though, it should highlight the paren if you're on the ending paren and evil's eol defcustom is set (can't remember the name right now).

I think the problem is emacs by default (without evil/evil-collection) will only highlight the paren if the point is after the last paren (similar to how point needs to be after paren when eval-ing s-expressions).

I could be totally off base but I think taking a closer look at the show-paren advice we have and fixing it up will be the right fix.

Thanks for the report, that behavior was bugging me too but I couldn't put my finger on what it was.

@noctuid
Copy link
Contributor

noctuid commented Jan 20, 2018

I don't have this issue with regular evil.

@jojojames
Copy link
Collaborator

I see..

Looks like evil-mode activates/deactivtes those advices..

(defadvice evil-mode (after start-evil activate)
  "Enable Evil in Fundamental mode."
  (if evil-mode
      (progn
        (when (eq (default-value 'major-mode) 'fundamental-mode)
          ;; changed back by `evil-local-mode'
          (setq-default major-mode 'turn-on-evil-mode))
        (ad-enable-regexp "^evil")
        (ad-activate-regexp "^evil")
        (with-no-warnings (evil-esc-mode 1)))
    (when (eq (default-value 'major-mode) 'turn-on-evil-mode)
      (setq-default major-mode 'fundamental-mode))
    (ad-disable-regexp "^evil")
    (ad-update-regexp "^evil")
    (with-no-warnings (evil-esc-mode -1))))

I have a couple ideas for a quick fix but would be interested in hearing some other ideas.

A better solution might be to just rewrite the advice to use the new advice-add advice and enable it using advice-add and/or run ad-enable/activate-regexp ourselves (that seems wayy messy).

jojojames added a commit that referenced this issue Jan 20, 2018
Moved to advice-add as well as move integration into separate file.
@jojojames
Copy link
Collaborator

I just went with changing the advice + moving it to its own integration file. Lemme know how it goes for you @edkolev.

@edkolev
Copy link
Member Author

edkolev commented Jan 20, 2018

Works great, thanks for the quick response! It now works as regular evil.

@edkolev edkolev closed this as completed Jan 20, 2018
@noctuid
Copy link
Contributor

noctuid commented Jan 20, 2018

Wouldn't it make sense to automatically add the advice?

@jojojames
Copy link
Collaborator

@noctuid

I wanted to move that functionality out anyways so just went with the optional.

It's similar to #60 (comment).

There might be a set of functionality that should be set even if the user doesn't want the related keybindings. I'm still thinking about how to handle that.

@noctuid
Copy link
Contributor

noctuid commented Jan 20, 2018

My bad, I misunderstood the commit. Agreed that the separation makes sense.

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

No branches or pull requests

4 participants