GNU bug report logs - #14395
24.3; [PATCH] new feature smie-highlight-matching-block

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14395 in the body.
You can then email your comments to 14395 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Tue, 14 May 2013 10:49:20 +0800

[Message part 1 (text/plain, inline)]

Hi Stefan,

I want something for octave mode that looks like something in the
attached screenshot. But since this is generic I would like to put it in
smie.el. Do you have any objections or comments?

It doesn't make sense for this feature and smie-blink-matching-open to
be on at the same time. So in the patch nothing is enabled.


diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index bbdd9f83..ad23f78c 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1021,6 +1021,61 @@ (defun smie-blink-matching-open ()
             (let ((blink-matching-check-function #'smie-blink-matching-check))
               (blink-matching-open))))))))
 
+(defface smie-matching-block-highlight (:inherit highlight)
+  "Face used to highlight matching block."
+  :group 'smie)
+
+(defvar-local smie-highlight-matching-block-overlay nil)
+(defvar-local smie-highlight-matching-block-lastpos -1)
+
+(defun smie-highlight-matching-block ()
+  (when (and (bound-and-true-p smie-closer-alist)
+             (/= (point) smie-highlight-matching-block-lastpos))
+    (unless (overlayp smie-highlight-matching-block-overlay)
+      (setq smie-highlight-matching-block-overlay
+            (make-overlay (point) (point))))
+    (setq smie-highlight-matching-block-lastpos (point))
+    (let ((open-re (concat "\\_<"
+                           (regexp-opt (mapcar 'car smie-closer-alist))
+                           "\\_>"))
+          (close-re (concat "\\_<"
+                            (regexp-opt (mapcar 'cdr smie-closer-alist))
+                            "\\_>"))
+          (beg-of-tok
+           (lambda (re)
+             "Move to the beginning of current token if matching RE."
+             (or (looking-at-p re)
+                 (let* ((start (point))
+                        (beg (progn
+                               (funcall smie-backward-token-function)
+                               (and (looking-at-p re) (point))))
+                        (end (and beg
+                                  (progn
+                                    (funcall smie-forward-token-function)
+                                    (point)))))
+                   (if (and beg (<= beg start) (<= start end))
+                       (goto-char beg)
+                     (goto-char start)
+                     nil)))))
+          (highlight (lambda (beg end)
+                       (move-overlay smie-highlight-matching-block-overlay
+                                     beg end)
+                       (overlay-put smie-highlight-matching-block-overlay
+                                    'face 'smie-matching-block-highlight))))
+      (save-excursion
+        (cond
+         ((funcall beg-of-tok open-re)
+          (with-demoted-errors
+            (forward-sexp 1)
+            (when (looking-back close-re)
+              (funcall highlight (match-beginning 0) (match-end 0)))))
+         ((funcall beg-of-tok close-re)
+          (funcall smie-forward-token-function)
+          (forward-sexp -1)
+          (when (looking-at open-re)
+            (funcall highlight (match-beginning 0) (match-end 0))))
+         (t (overlay-put smie-highlight-matching-block-overlay 'face nil)))))))
+
 ;;; The indentation engine.
 
 (defcustom smie-indent-basic 4

[smie-highlight.png (image/png, attachment)]

Message #8 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Tue, 14 May 2013 12:02:38 -0400

> I want something for octave mode that looks like something in the
> attached screenshot. But since this is generic I would like to put it in
> smie.el. Do you have any objections or comments?

Looks like a good feature, thank you.

> It doesn't make sense for this feature and smie-blink-matching-open to
> be on at the same time. So in the patch nothing is enabled.

I don't think enabling it in octave-mode makes sense: this is more like
"blink-paren vs show-paren-mode", i.e. a personal preference.  So the
enabling/disabling should be done via code in smie.el.

> +  (when (and (bound-and-true-p smie-closer-alist)

It's defvarred to nil, so don't test if it's boundp.

> +    (let ((open-re (concat "\\_<"
> +                           (regexp-opt (mapcar 'car smie-closer-alist))
> +                           "\\_>"))
> +          (close-re (concat "\\_<"
> +                            (regexp-opt (mapcar 'cdr smie-closer-alist))
> +                            "\\_>"))

The string returned by smie-forward-token-function is usually the same
as the representation of the token in the buffer, but not always.
So the above is not strictly correct.

Instead you want to call smie-for/backward-token-function and then
compare the result via (r?assoc tok smie-closer-alist).

> +         ((funcall beg-of-tok open-re)
> +          (with-demoted-errors
> +            (forward-sexp 1)
> +            (when (looking-back close-re)
> +              (funcall highlight (match-beginning 0) (match-end 0)))))

I think this should not use with-demoted-errors but instead should
explicitly catch the scan-error and turn it into a message.
After all, the user doesn't want to be thrown in the debugger just
because his sexp is not properly closed yet.  And also this way you can
provide a much nicer error message.


        Stefan

Message #11 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Wed, 15 May 2013 15:13:49 +0800

On 2013-05-15 00:02 +0800, Stefan Monnier wrote:
> I don't think enabling it in octave-mode makes sense: this is more like
> "blink-paren vs show-paren-mode", i.e. a personal preference.  So the
> enabling/disabling should be done via code in smie.el.
>
>> +  (when (and (bound-and-true-p smie-closer-alist)
>
> It's defvarred to nil, so don't test if it's boundp.
>
>> +    (let ((open-re (concat "\\_<"
>> +                           (regexp-opt (mapcar 'car smie-closer-alist))
>> +                           "\\_>"))
>> +          (close-re (concat "\\_<"
>> +                            (regexp-opt (mapcar 'cdr smie-closer-alist))
>> +                            "\\_>"))
>
> The string returned by smie-forward-token-function is usually the same
> as the representation of the token in the buffer, but not always.
> So the above is not strictly correct.
>
> Instead you want to call smie-for/backward-token-function and then
> compare the result via (r?assoc tok smie-closer-alist).
>
>> +         ((funcall beg-of-tok open-re)
>> +          (with-demoted-errors
>> +            (forward-sexp 1)
>> +            (when (looking-back close-re)
>> +              (funcall highlight (match-beginning 0) (match-end 0)))))
>
> I think this should not use with-demoted-errors but instead should
> explicitly catch the scan-error and turn it into a message.
> After all, the user doesn't want to be thrown in the debugger just
> because his sexp is not properly closed yet.  And also this way you can
> provide a much nicer error message.

Thank you for your comments, Stefan. I have taken these into account and
new patch attached.

One thing in the patch that I dislike is having to forward-declare
smie-highlight-matching-block-mode. Do you have a cleaner way?

Leo


=== modified file 'lisp/emacs-lisp/smie.el'
--- lisp/emacs-lisp/smie.el	2013-04-25 03:25:34 +0000
+++ lisp/emacs-lisp/smie.el	2013-05-15 07:03:02 +0000
@@ -966,12 +966,15 @@
         (let ((starter (funcall smie-forward-token-function)))
           (not (member (cons starter ender) smie-closer-alist))))))))
 
+(defvar smie-highlight-matching-block-mode nil) ; Silence compiler warning
+
 (defun smie-blink-matching-open ()
   "Blink the matching opener when applicable.
 This uses SMIE's tables and is expected to be placed on `post-self-insert-hook'."
   (let ((pos (point))                   ;Position after the close token.
         token)
     (when (and blink-matching-paren
+               (not smie-highlight-matching-block-mode)
                smie-closer-alist                     ; Optimization.
                (or (eq (char-before) last-command-event) ;; Sanity check.
                    (save-excursion
@@ -1021,6 +1024,80 @@
             (let ((blink-matching-check-function #'smie-blink-matching-check))
               (blink-matching-open))))))))
 
+(defface smie-matching-block-highlight '((t (:inherit highlight)))
+  "Face used to highlight matching block."
+  :group 'smie)
+
+(defvar smie-highlight-matching-block-timer nil)
+(defvar-local smie-highlight-matching-block-overlay nil)
+(defvar-local smie-highlight-matching-block-lastpos -1)
+
+(defun smie-highlight-matching-block ()
+  (when (and smie-closer-alist
+             (/= (point) smie-highlight-matching-block-lastpos))
+    (unless (overlayp smie-highlight-matching-block-overlay)
+      (setq smie-highlight-matching-block-overlay
+            (make-overlay (point) (point))))
+    (setq smie-highlight-matching-block-lastpos (point))
+    (let ((beg-of-tok
+           (lambda (&optional start)
+             "Move to the beginning of current token."
+             (let* ((token)
+                    (start (or start (point)))
+                    (beg (progn
+                           (funcall smie-backward-token-function)
+                           (point)))
+                    (end (progn
+                           (setq token (funcall smie-forward-token-function))
+                           (point))))
+               (if (and (<= beg start) (<= start end)
+                        (or (assoc token smie-closer-alist)
+                            (rassoc token smie-closer-alist)))
+                   (progn (goto-char beg) token)
+                 (goto-char start)
+                 nil))))
+          (highlight (lambda (beg end)
+                       (move-overlay smie-highlight-matching-block-overlay
+                                     beg end)
+                       (overlay-put smie-highlight-matching-block-overlay
+                                    'face 'smie-matching-block-highlight))))
+      (save-excursion
+        (condition-case nil
+            (if (nth 8 (syntax-ppss))
+                (overlay-put smie-highlight-matching-block-overlay 'face nil)
+              (let ((token
+                     (or (funcall beg-of-tok)
+                         (funcall beg-of-tok
+                                  (prog1 (point)
+                                    (funcall smie-forward-token-function))))))
+                (cond
+                 ((assoc token smie-closer-alist) ; opener
+                  (forward-sexp 1)
+                  (let ((end (point))
+                        (closer (funcall smie-backward-token-function)))
+                    (when (rassoc closer smie-closer-alist)
+                      (funcall highlight (point) end))))
+                 ((rassoc token smie-closer-alist) ; closer
+                  (funcall smie-forward-token-function)
+                  (forward-sexp -1)
+                  (let ((beg (point))
+                        (opener (funcall smie-forward-token-function)))
+                    (when (assoc opener smie-closer-alist)
+                      (funcall highlight beg (point)))))
+                 (t (overlay-put smie-highlight-matching-block-overlay
+                                 'face nil)))))
+          (scan-error
+           (overlay-put smie-highlight-matching-block-overlay 'face nil)))))))
+
+;;;###autoload
+(define-minor-mode smie-highlight-matching-block-mode nil
+  :global t :group 'smie
+  (if smie-highlight-matching-block-mode
+      (setq smie-highlight-matching-block-timer
+            (run-with-idle-timer 0.2 t #'smie-highlight-matching-block))
+    (when (timerp smie-highlight-matching-block-timer)
+      (cancel-timer smie-highlight-matching-block-timer))))
+
 ;;; The indentation engine.
 
 (defcustom smie-indent-basic 4

Message #14 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Wed, 15 May 2013 22:31:47 -0400

> Thank you for your comments, Stefan. I have taken these into account and
> new patch attached.

See below.

> One thing in the patch that I dislike is having to forward-declare
> smie-highlight-matching-block-mode. Do you have a cleaner way?

Move the define-minor-mode before the first use of the variable?

> +(defvar smie-highlight-matching-block-mode nil) ; Silence compiler warning

A forward declaration takes the form (defvar foo), not (defvar foo nil).

>  (defun smie-blink-matching-open ()
>    "Blink the matching opener when applicable.
>  This uses SMIE's tables and is expected to be placed on `post-self-insert-hook'."
>    (let ((pos (point))                   ;Position after the close token.
>          token)
>      (when (and blink-matching-paren
> +               (not smie-highlight-matching-block-mode)

I suggest you instead remove smie-blink-matching-open from
post-self-insert-hook (and refrain from adding it back) when enabling
smie-highlight-matching-block-mode.

> +(defvar smie-highlight-matching-block-timer nil)
> +(defvar-local smie-highlight-matching-block-overlay nil)
> +(defvar-local smie-highlight-matching-block-lastpos -1)

Please use the "smie--" prefix for such internal variables.

> +(define-minor-mode smie-highlight-matching-block-mode nil

Please provide a docstring.

> +  :global t :group 'smie

Don't bother with the ":group 'smie", it'll be added automatically anyway.

> +  (if smie-highlight-matching-block-mode
> +      (setq smie-highlight-matching-block-timer
> +            (run-with-idle-timer 0.2 t #'smie-highlight-matching-block))
> +    (when (timerp smie-highlight-matching-block-timer)
> +      (cancel-timer smie-highlight-matching-block-timer))))

   (progn
     (smie-highlight-matching-block-mode 1)
     (smie-highlight-matching-block-mode 1))

Will start 2 timers (and lose the reference to the first one, making
it harder to cancel).  Better structure minor mode bodies as "first
unconditionally turn it off, then turn it on is needed" to avoid
such problems.

Similarly

   (progn
     (smie-highlight-matching-block-mode -1)
     (smie-highlight-matching-block-mode -1))

will try to cancel an already canceled timer, so better reset the timer
var to nil after canceling.


        Stefan

Message #17 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 11:27:31 +0800

On 2013-05-16 10:31 +0800, Stefan Monnier wrote:
>> +(define-minor-mode smie-highlight-matching-block-mode nil
>
> Please provide a docstring.

Is the automatically-provided docstring good enough?

New patch as follows.


diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index bbdd9f83..de91c21f 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1021,6 +1021,85 @@ (defun smie-blink-matching-open ()
             (let ((blink-matching-check-function #'smie-blink-matching-check))
               (blink-matching-open))))))))
 
+(defface smie-matching-block-highlight '((t (:inherit highlight)))
+  "Face used to highlight matching block."
+  :group 'smie)
+
+(defvar-local smie--highlight-matching-block-overlay nil)
+(defvar-local smie--highlight-matching-block-lastpos -1)
+
+(defun smie-highlight-matching-block ()
+  (when (and smie-closer-alist
+             (/= (point) smie--highlight-matching-block-lastpos))
+    (unless (overlayp smie--highlight-matching-block-overlay)
+      (setq smie--highlight-matching-block-overlay
+            (make-overlay (point) (point))))
+    (setq smie--highlight-matching-block-lastpos (point))
+    (let ((beg-of-tok
+           (lambda (&optional start)
+             "Move to the beginning of current token at START."
+             (let* ((token)
+                    (start (or start (point)))
+                    (beg (progn
+                           (funcall smie-backward-token-function)
+                           (forward-comment (point-max))
+                           (point)))
+                    (end (progn
+                           (setq token (funcall smie-forward-token-function))
+                           (forward-comment (- (point)))
+                           (point))))
+               (if (and (<= beg start) (<= start end)
+                        (or (assoc token smie-closer-alist)
+                            (rassoc token smie-closer-alist)))
+                   (progn (goto-char beg) token)
+                 (goto-char start)
+                 nil))))
+          (highlight
+           (lambda (beg end)
+             (move-overlay smie--highlight-matching-block-overlay beg end)
+             (overlay-put smie--highlight-matching-block-overlay
+                          'face 'smie-matching-block-highlight))))
+      (save-excursion
+        (condition-case nil
+            (if (nth 8 (syntax-ppss))
+                (overlay-put smie--highlight-matching-block-overlay 'face nil)
+              (let ((token
+                     (or (funcall beg-of-tok)
+                         (funcall beg-of-tok
+                                  (prog1 (point)
+                                    (funcall smie-forward-token-function))))))
+                (cond
+                 ((assoc token smie-closer-alist) ; opener
+                  (forward-sexp 1)
+                  (let ((end (point))
+                        (closer (funcall smie-backward-token-function)))
+                    (when (rassoc closer smie-closer-alist)
+                      (funcall highlight (point) end))))
+                 ((rassoc token smie-closer-alist) ; closer
+                  (funcall smie-forward-token-function)
+                  (forward-sexp -1)
+                  (let ((beg (point))
+                        (opener (funcall smie-forward-token-function)))
+                    (when (assoc opener smie-closer-alist)
+                      (funcall highlight beg (point)))))
+                 (t (overlay-put smie--highlight-matching-block-overlay
+                                 'face nil)))))
+          (scan-error
+           (overlay-put smie--highlight-matching-block-overlay 'face nil)))))))
+
+(defvar smie--highlight-matching-block-timer nil)
+
+;;;###autoload
+(define-minor-mode smie-highlight-matching-block-mode nil :global t
+  (when (timerp smie--highlight-matching-block-timer)
+    (cancel-timer smie--highlight-matching-block-timer))
+  (if smie-highlight-matching-block-mode
+      (setq smie--highlight-matching-block-timer
+            (run-with-idle-timer 0.2 t #'smie-highlight-matching-block))
+    (when (timerp smie--highlight-matching-block-timer)
+      (cancel-timer smie--highlight-matching-block-timer))
+    (setq smie--highlight-matching-block-timer nil)))
+
 ;;; The indentation engine.
 
 (defcustom smie-indent-basic 4
@@ -1698,8 +1777,11 @@ (defun smie-setup (grammar rules-function &rest keywords)
       ;; Only needed for interactive calls to blink-matching-open.
       (set (make-local-variable 'blink-matching-check-function)
            #'smie-blink-matching-check)
-      (add-hook 'post-self-insert-hook
-                #'smie-blink-matching-open 'append 'local)
+      (if smie-highlight-matching-block-mode
+          (remove-hook 'post-self-insert-hook
+                       #'smie-blink-matching-open 'local)
+        (add-hook 'post-self-insert-hook
+                  #'smie-blink-matching-open 'append 'local))
       (set (make-local-variable 'smie-blink-matching-triggers)
            (append smie-blink-matching-triggers
                    ;; Rather than wait for SPC to blink, try to blink as

Message #20 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14395 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 00:45:40 -0400

Stefan Monnier wrote:

>> +(define-minor-mode smie-highlight-matching-block-mode nil
>> +  :global t :group 'smie
>
> Don't bother with the ":group 'smie", it'll be added automatically anyway.

That would be preferable, but it doesn't work like that.
Instead it will automatically use a totally-bogus
"smie-highlight-matching-block" group. I just fixed a bunch of such
problems.

Message #23 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 13:33:45 +0800

On 2013-05-16 12:45 +0800, Glenn Morris wrote:
> That would be preferable, but it doesn't work like that.
> Instead it will automatically use a totally-bogus
> "smie-highlight-matching-block" group.

Good catch. I have fixed it locally.

Leo

Message #26 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 09:24:08 -0400

>>> +(define-minor-mode smie-highlight-matching-block-mode nil
>> Please provide a docstring.
> Is the automatically-provided docstring good enough?

Oh, right, I guess it is, sorry I forgot that I tried to make
define-minor-mode DTRT.

> +(define-minor-mode smie-highlight-matching-block-mode nil :global t
> +  (when (timerp smie--highlight-matching-block-timer)
> +    (cancel-timer smie--highlight-matching-block-timer))
> +  (if smie-highlight-matching-block-mode
> +      (setq smie--highlight-matching-block-timer
> +            (run-with-idle-timer 0.2 t #'smie-highlight-matching-block))
> +    (when (timerp smie--highlight-matching-block-timer)
> +      (cancel-timer smie--highlight-matching-block-timer))
> +    (setq smie--highlight-matching-block-timer nil)))

No, need to "cancel" twice when disabling the mode.

> -      (add-hook 'post-self-insert-hook
> -                #'smie-blink-matching-open 'append 'local)
> +      (if smie-highlight-matching-block-mode
> +          (remove-hook 'post-self-insert-hook
> +                       #'smie-blink-matching-open 'local)
> +        (add-hook 'post-self-insert-hook
> +                  #'smie-blink-matching-open 'append 'local))

I think the `remove-hook' should be done within the body of the
smie-highlight-matching-block-mode minor mode rather than here.
In here, you just need to wrap the add-hook within a test of
smie-highlight-matching-block-mode.

BTW.  Is there a non-SMIE version of "highlight-matching-block-mode",
which does it for parentheses?  If yes, maybe 
smie-highlight-matching-block-mode should integrate into it.

One more thought, maybe you were right that futzing around with
add/remove-hook is too complicated and it's easier to check a variable.
But then maybe smie-highlight-matching-block-mode should set
blink-matching-paren to nil (which brings us back to whether there's
a global highlight-matching-block-mode working not just for modes using
SMIE).


        Stefan

Message #29 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Fri, 17 May 2013 00:06:23 +0800

On 2013-05-16 21:24 +0800, Stefan Monnier wrote:
> I think the `remove-hook' should be done within the body of the
> smie-highlight-matching-block-mode minor mode rather than here.
> In here, you just need to wrap the add-hook within a test of
> smie-highlight-matching-block-mode.

But the post insert hook is buffer-local. Seems too much trouble to find
all of them and remove-hook.

> BTW.  Is there a non-SMIE version of "highlight-matching-block-mode",
> which does it for parentheses?  If yes, maybe 
> smie-highlight-matching-block-mode should integrate into it.

I am not sure there is.

> One more thought, maybe you were right that futzing around with
> add/remove-hook is too complicated and it's easier to check a variable.
> But then maybe smie-highlight-matching-block-mode should set
> blink-matching-paren to nil (which brings us back to whether there's
> a global highlight-matching-block-mode working not just for modes using
> SMIE).

Maybe checking smie-highlight-matching-block-mode in
smie-blink-matching-open is the better solution because
post-self-insert-hook is buffer-local.

Leo

Message #32 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 13:51:12 -0400

>> I think the `remove-hook' should be done within the body of the
>> smie-highlight-matching-block-mode minor mode rather than here.
>> In here, you just need to wrap the add-hook within a test of
>> smie-highlight-matching-block-mode.
> But the post insert hook is buffer-local.

I know, but removing it where you remove it has mostly no effect (it's
normally run from a major-mode body, so all the vars have been set
back to their global value).

> Seems too much trouble to find all of them and remove-hook.

That's fine.  But doing it where you currently do it is a waste.

>> BTW.  Is there a non-SMIE version of "highlight-matching-block-mode",
>> which does it for parentheses?  If yes, maybe
>> smie-highlight-matching-block-mode should integrate into it.
> I am not sure there is.

Doesn't show-paren-mode do that?

>> One more thought, maybe you were right that futzing around with
>> add/remove-hook is too complicated and it's easier to check a variable.
>> But then maybe smie-highlight-matching-block-mode should set
>> blink-matching-paren to nil (which brings us back to whether there's
>> a global highlight-matching-block-mode working not just for modes using
>> SMIE).
> Maybe checking smie-highlight-matching-block-mode in
> smie-blink-matching-open is the better solution because
> post-self-insert-hook is buffer-local.

OK.
BTW, feel free to commit your current code in the mean time.


        Stefan

Message #37 received at 14395-done <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14395-done <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Fri, 17 May 2013 07:09:48 +0800

Fixed in trunk.

On 2013-05-17 01:51 +0800, Stefan Monnier wrote:
> I know, but removing it where you remove it has mostly no effect (it's
> normally run from a major-mode body, so all the vars have been set
> back to their global value).

OK, corrected as suggested.

>> I am not sure there is.
>
> Doesn't show-paren-mode do that?

I didn't do anything to integrate with show-paren-mode. Not sure what to
do. There are packages outside emacs that replace show-paren-mode.

> OK.
> BTW, feel free to commit your current code in the mean time.

OK, committed and thanks for all the help.

Leo

Message #40 received at 14395-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395-done <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Thu, 16 May 2013 20:40:39 -0400

>> Doesn't show-paren-mode do that?
> I didn't do anything to integrate with show-paren-mode.

But in terms of functionality, your minor mode provides a behavior
comparable to show-paren-mode, I think.

So maybe the better thing to do now is to try and see how to integrate
so that it "just works" for people who use show-paren-mode.  Just like
smie-blink-matching-open makes blink-matching-paren "just work".


        Stefan

Message #43 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Fri, 17 May 2013 09:30:34 +0800

On 2013-05-17 08:40 +0800, Stefan Monnier wrote:
> But in terms of functionality, your minor mode provides a behavior
> comparable to show-paren-mode, I think.

I agree.

> So maybe the better thing to do now is to try and see how to integrate
> so that it "just works" for people who use show-paren-mode.  Just like
> smie-blink-matching-open makes blink-matching-paren "just work".

Would it be OK if we let show-paren-mode turn on
smie-highlight-matching-block-mode?

I just realised that it doesn't clean up the overlays when
smie-highlight-matching-block-mode is turned off.

Should I install something like this? An alternative fix might be not to
make smie--highlight-matching-block-overlay buffer-local which is easier
to clean up.

diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index 21134578..3e78b76b 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1095,10 +1095,17 @@ (define-minor-mode smie-highlight-matching-block-mode nil
   (when (timerp smie--highlight-matching-block-timer)
     (cancel-timer smie--highlight-matching-block-timer))
   (setq smie--highlight-matching-block-timer nil)
-  (when smie-highlight-matching-block-mode
-    (remove-hook 'post-self-insert-hook #'smie-blink-matching-open 'local)
-    (setq smie--highlight-matching-block-timer
-          (run-with-idle-timer 0.2 t #'smie-highlight-matching-block))))
+  (if smie-highlight-matching-block-mode
+      (progn
+        (remove-hook 'post-self-insert-hook #'smie-blink-matching-open 'local)
+        (setq smie--highlight-matching-block-timer
+              (run-with-idle-timer 0.2 t #'smie-highlight-matching-block)))
+    ;; Clean up.
+    (dolist (b (buffer-list))
+      (with-current-buffer b
+        (when (overlayp smie--highlight-matching-block-overlay)
+          (delete-overlay smie--highlight-matching-block-overlay)
+          (kill-local-variable smie--highlight-matching-block-overlay))))))
 
 ;;; The indentation engine.

Message #46 received at 14395 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 14395 <at> debbugs.gnu.org
Subject: Re: bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
Date: Wed, 22 May 2013 14:50:06 -0400

> Would it be OK if we let show-paren-mode turn on
> smie-highlight-matching-block-mode?

I think so, yes.  Even better would be to add some hook(s) to
show-paren-mode, which smie-setup can use so that smie's highlighting
(re)uses show-paren's code (e.g. so smie-highlight doesn't have its own
timer nor its own overlay).

That will also save us the trouble of disabling blink-paren, since
blink-matching-open already checks show-paren-mode (which is slightly
more refined than to just disable blink-matching-open when
show-paren-mode is enabled).


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.