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

[Kaushal Modi]

Hi Stefan,

Thank you for the through review of the patch. My replies are inline below.

On Fri, Jul 15, 2016 at 10:15 PM Stefan Monnier <address@hidden> wrote:

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).

I believe that is in reference to the use of post-command-hook, correct? I need to yet go through that code.

 

> -  :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
";;".

OK. I have reverted that hunk in the patch.

 

> +    (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.

Thanks.

 

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`).

I see .. so you are suggesting to highlight the line at the window point, instead of the line where (point) is, correct? I have never needed to do that, but I get it. For now, "C-x 4 c" could be used as a work around.

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

Why (nlinum--after-change)?

You are correct, that wasn't needed.

 

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

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

From quick trial, that does not work. That function might need to be fixed for it work work in this use case where it is called in post-command-hook. From quick trial, I saw the line numbers change in the whole buffer even when I moved the cursor horizontally. I need to yet look into that function to understand why that's happening.

 

> +    ;; 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.

In the below updated patch, I attempt to do this (code commented out in the patch), but failed. I tried removing the text properties on the current and last lines, but it is not working. I'll give more time to understand tomorrow. But if you can point out the issue with those remove-text-properties, that will be great.

> +    (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).

Thanks. I learned that today. Fixed.

 

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))

Done. Thanks.

 

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.

I tried implementing that, but doesn't work as I expected. nlinum-current-line-face is already inheriting linum face. And I have set the linum face height to be 0.9 (ratio). From what I understand, "'(nlinum-current-line-face linum)" applies linum face properties which nlinum-current-line-face does not change. As nlinum-current-line-face inherits linum, that face height is already 0.9. But when linum applies on top of that, the current line face reduces further by 0.9 (or so it looks like). So I end up with the current line number face at 0.81 height and the rest of the line numbers at 0.9.

So I stuck with the applying just nlinum-current-line-face (it still inherits from linum).

I have also made one cosmetic change .. Instead of `t' as argument values, I have replaced them with `:local' or `:contextual' as appropriate. I like doing that so that I do not need to look up the documentation to learn what happens when an arg is set to `t'. Also `:contextual' and `:local' show up in a different face, which I like. Let me know if that doing that is alright..

===== Below is patch draft v2

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el

index 98c9cbc..4d1cf64 100644

--- a/packages/nlinum/nlinum.el

+++ b/packages/nlinum/nlinum.el

@@ -36,11 +36,22 @@

 

 ;;; Code:

 

-(require 'linum)                        ;For its face.

+(require 'linum) ; For its face.

 

 (defvar nlinum--width 2)

 (make-variable-buffer-local 'nlinum--width)

 

+(defface nlinum-current-line-face

+  '((t :inherit linum :weight bold))

+  "Face for displaying current line."

+  :group 'nlinum)

+

+(defvar-local nlinum--current-line 0

+  "Store current line number.")

+

+(defvar-local nlinum--last-line 0

+  "Store line number where the point was before it moved to the current line.")

+

 ;; (defvar nlinum--desc "")

 

 ;;;###autoload

@@ -53,9 +64,10 @@ if ARG is omitted or nil.

 Linum mode is a buffer-local minor mode."

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

   (jit-lock-unregister #'nlinum--region)

-  (remove-hook 'window-configuration-change-hook #'nlinum--setup-window t)

-  (remove-hook 'text-scale-mode-hook #'nlinum--setup-window t)

-  (remove-hook 'after-change-functions #'nlinum--after-change t)

+  (remove-hook 'window-configuration-change-hook #'nlinum--setup-window :local)

+  (remove-hook 'text-scale-mode-hook #'nlinum--setup-window :local)

+  (remove-hook 'after-change-functions #'nlinum--after-change :local)

+  (remove-hook 'post-command-hook #'nlinum--current-line-update :local)

   (kill-local-variable 'nlinum--line-number-cache)

   (remove-overlays (point-min) (point-max) 'nlinum t)

   ;; (kill-local-variable 'nlinum--ol-counter)

@@ -64,10 +76,11 @@ Linum mode is a buffer-local minor mode."

     ;; FIXME: Another approach would be to make the mode permanent-local,

     ;; which might indeed be preferable.

     (add-hook 'change-major-mode-hook (lambda () (nlinum-mode -1)))

-    (add-hook 'text-scale-mode-hook #'nlinum--setup-window nil t)

-    (add-hook 'window-configuration-change-hook #'nlinum--setup-window nil t)

-    (add-hook 'after-change-functions #'nlinum--after-change nil t)

-    (jit-lock-register #'nlinum--region t))

+    (add-hook 'text-scale-mode-hook #'nlinum--setup-window nil :local)

+    (add-hook 'window-configuration-change-hook #'nlinum--setup-window nil :local)

+    (add-hook 'after-change-functions #'nlinum--after-change nil :local)

+    (add-hook 'post-command-hook #'nlinum--current-line-update nil :local)

+    (jit-lock-register #'nlinum--region :contextual))

   (nlinum--setup-windows))

 

 (defun nlinum--face-height (face)

@@ -131,6 +144,36 @@ Linum mode is a buffer-local minor mode."

                          (point-min) (point-max) '(fontified)))))

                   (current-buffer)))

 

+(defun nlinum--current-line-update ()

+  "Update current line number, flush text properties for last and current line."

+  (setq nlinum--last-line nlinum--current-line)

+  ;; (setq nlinum--current-line (nlinum--line-number-at-pos)) ; does not work

+  (setq nlinum--current-line (line-number-at-pos)) ; works

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

+

+  ;; Flush the text properties only if the point has changed lines.

+  (when (not (eq nlinum--current-line nlinum--last-line))

+    (let* ((line-diff (- nlinum--last-line nlinum--current-line))

+           (last-line-beg (line-beginning-position line-diff))

+           (last-line-end (line-end-position line-diff))

+           ;; (curr-line-beg (line-beginning-position))

+           ;; (curr-line-end (line-end-position))

+           )

+      ;; (message "last:%d, curr:%d" nlinum--last-line nlinum--current-line)

+      (let* ((start (window-start)) ; works

+             (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))))

+      ;; (with-silent-modifications ; does not work

+      ;;   (remove-text-properties last-line-beg last-line-end '(fontified))

+      ;;   ;; (remove-text-properties curr-line-beg curr-line-end '(fontified))

+      ;;   )

+      )))

+

 ;; (defun nlinum--ol-count ()

 ;;   (let ((i 0))

 ;;     (dolist (ol (overlays-in (point-min) (point-max)))

@@ -215,11 +258,16 @@ Used by the default `nlinum-format-function'."

 

 (defvar nlinum-format-function

   (lambda (line width)

-    (let ((str (format nlinum-format line)))

+    (let* ((is-current-line (= line nlinum--current-line))

+           (str (format nlinum-format line)))

       (when (< (length str) width)

         ;; Left pad to try and right-align the line-numbers.

         (setq str (concat (make-string (- width (length str)) ?\ ) str)))

-      (put-text-property 0 width 'face 'linum str)

+      (put-text-property 0 width 'face

+                         (if is-current-line

+                             'nlinum-current-line-face

+                           'linum)

+                         str)

       str))

   "Function to build the string representing the line number.

 Takes 2 arguments LINE and WIDTH, both of them numbers, and should return

=====

Thanks.

--

Kaushal Modi