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

Previous Next

Full log

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

Previous Next

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