* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
@ 2013-05-14 2:49 Leo Liu
2013-05-14 16:02 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-14 2:49 UTC (permalink / raw)
To: 14395
[-- Attachment #1: Type: text/plain, Size: 3277 bytes --]
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
[-- Attachment #2: smie-highlight.png --]
[-- Type: image/png, Size: 13452 bytes --]
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-14 2:49 bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block Leo Liu
@ 2013-05-14 16:02 ` Stefan Monnier
2013-05-15 7:13 ` Leo Liu
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-05-14 16:02 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-14 16:02 ` Stefan Monnier
@ 2013-05-15 7:13 ` Leo Liu
2013-05-16 2:31 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-15 7:13 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-15 7:13 ` Leo Liu
@ 2013-05-16 2:31 ` Stefan Monnier
2013-05-16 3:27 ` Leo Liu
2013-05-16 4:45 ` Glenn Morris
0 siblings, 2 replies; 14+ messages in thread
From: Stefan Monnier @ 2013-05-16 2:31 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 2:31 ` Stefan Monnier
@ 2013-05-16 3:27 ` Leo Liu
2013-05-16 13:24 ` Stefan Monnier
2013-05-16 4:45 ` Glenn Morris
1 sibling, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-16 3:27 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 2:31 ` Stefan Monnier
2013-05-16 3:27 ` Leo Liu
@ 2013-05-16 4:45 ` Glenn Morris
2013-05-16 5:33 ` Leo Liu
1 sibling, 1 reply; 14+ messages in thread
From: Glenn Morris @ 2013-05-16 4:45 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395, Leo Liu
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 4:45 ` Glenn Morris
@ 2013-05-16 5:33 ` Leo Liu
0 siblings, 0 replies; 14+ messages in thread
From: Leo Liu @ 2013-05-16 5:33 UTC (permalink / raw)
To: Glenn Morris; +Cc: 14395
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 3:27 ` Leo Liu
@ 2013-05-16 13:24 ` Stefan Monnier
2013-05-16 16:06 ` Leo Liu
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-05-16 13:24 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395
>>> +(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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 13:24 ` Stefan Monnier
@ 2013-05-16 16:06 ` Leo Liu
2013-05-16 17:51 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-16 16:06 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 16:06 ` Leo Liu
@ 2013-05-16 17:51 ` Stefan Monnier
2013-05-16 23:09 ` Leo Liu
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-05-16 17:51 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395
>> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 17:51 ` Stefan Monnier
@ 2013-05-16 23:09 ` Leo Liu
2013-05-17 0:40 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-16 23:09 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395-done
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-16 23:09 ` Leo Liu
@ 2013-05-17 0:40 ` Stefan Monnier
2013-05-17 1:30 ` Leo Liu
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier @ 2013-05-17 0:40 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395-done
>> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-17 0:40 ` Stefan Monnier
@ 2013-05-17 1:30 ` Leo Liu
2013-05-22 18:50 ` Stefan Monnier
0 siblings, 1 reply; 14+ messages in thread
From: Leo Liu @ 2013-05-17 1:30 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 14395
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.
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block
2013-05-17 1:30 ` Leo Liu
@ 2013-05-22 18:50 ` Stefan Monnier
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Monnier @ 2013-05-22 18:50 UTC (permalink / raw)
To: Leo Liu; +Cc: 14395
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-05-22 18:50 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-14 2:49 bug#14395: 24.3; [PATCH] new feature smie-highlight-matching-block Leo Liu
2013-05-14 16:02 ` Stefan Monnier
2013-05-15 7:13 ` Leo Liu
2013-05-16 2:31 ` Stefan Monnier
2013-05-16 3:27 ` Leo Liu
2013-05-16 13:24 ` Stefan Monnier
2013-05-16 16:06 ` Leo Liu
2013-05-16 17:51 ` Stefan Monnier
2013-05-16 23:09 ` Leo Liu
2013-05-17 0:40 ` Stefan Monnier
2013-05-17 1:30 ` Leo Liu
2013-05-22 18:50 ` Stefan Monnier
2013-05-16 4:45 ` Glenn Morris
2013-05-16 5:33 ` Leo Liu
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).