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

[Kaushal Modi <kaushal.modi@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10840 bytes --]

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 <monnier@iro.umontreal.ca>
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

[-- Attachment #2: Type: text/html, Size: 15576 bytes --]