emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patch to highlight current line number [nlinum.el]


From: Stefan Monnier
Subject: Re: Patch to highlight current line number [nlinum.el]
Date: Fri, 15 Jul 2016 22:15:42 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux)

> Inspired by the nlinum-relative package[1], I have come up with the below
> patch for nlinum that just sets the current line number in a different face
> `nlinum-current-line-face'. @SheJinxin, hope this is fine with you.

Sounds like a good feature, thanks.

> By submitting this patch to the list, I am looking forward to how the
> performance can be improved.

See my comments below.

You might like to look at nhexl-mode to see how I've done that in
a similar context (tho the nhexl-mode approach is not really right
either).

> -  :lighter nil ;; (" NLinum" nlinum--desc)
> +  :lighter nil ; (" NLinum" nlinum--desc)

Nitpick: this change results in code that mis-indented because a single
";" should be indented to comment-column or thereabout (use M-; to place
it right).  I wanted the comment close to the code which is why I used
";;".

> +    (add-hook 'post-command-hook #'nlinum--current-line-update nil t))

Using post-command-hook is OK (that's what I use in nhexl-mode as well).

I think it'd be better to use pre-redisplay-functions, but I haven't
played with that option yet, and post-command-hook is easier to work with.

In any case, the hard thing to get right (which you haven't tried to
solve and neither have I in nhexl-mode) is when the buffer is displayed
in several windows (in which case each window will want to highlight
potentially different line numbers since each window has a different
`point`).

> +(defun nlinum--current-line-update ()
> +  "Reflush display on current window"
> +  (when nlinum-mode
> +    (nlinum--after-change)

Why (nlinum--after-change)?

> +    (setq nlinum--current-line (string-to-number (format-mode-line "%l")))

I think this should use `nlinum--line-number-at-pos`?

> +    ;; Do reflush only in the visible portion in the window, not the whole
> +    ;; buffer, when possible.
> +    (let* ((start (window-start))
> +           (end (window-end))
> +           (out-of-range-p (< (point-max) end)))
> +      (when out-of-range-p
> +        (setq start (point-min))
> +        (setq end (point-max)))
> +      (with-silent-modifications
> +        (remove-text-properties start end '(fontified))))))

I think this is pessimistic.  There's no need to refresh the whole
window's line numbers.  The only line numbers that can/should change are
the line number of the old cursor position and that of the new
cursor position.  And if point is still in the same line, there's
nothing to do.

> +    (let* ((line-diff (abs (- line nlinum--current-line)))
> +           (current-line-p (eq line-diff 0))

"...-p" means "..-predicate", and a boolean variable is not a predicate
(a predicate in (E)Lisp is usually a function that returns a boolean).

And you can simplify this to

              (is-current-line (= line nlinum--current-line))

> +      (if current-line-p
> +          (put-text-property 0 width 'face 'nlinum-current-line-face str)
> +        (put-text-property 0 width 'face 'linum str))

Aka

         (put-text-property 0 width 'face
                            (if current-line-p
                                'nlinum-current-line-face
                              'linum)
                            str))

Also I think it'd just always use the `linum` face, as in

         (put-text-property 0 width 'face
                            (if current-line-p
                                '(nlinum-current-line-face linum)
                              'linum)
                            str))

Tho it's clearly a question of taste.


        Stefan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]