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

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

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

Hi Stefan,

My comments are inline below, with a formatted patch at the end of this
email.

On Mon, Jul 18, 2016 at 1:06 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> It looks like the (remove-text-properties last-line-beg last-line-end
> '(fontified)) doesn't do its job, indeed.  AFAICT it seems to work
> correctly when moving down but not when moving up, so maybe it's just an
> off-by-one error somewhere.
>

It works! I assumed incorrectly that line-beginning-position and
line-end-position accept negative arguments too. Fixed by doing
(save-excursion (forward-line diff) ..) before calling those.


> BTW, Elisp generally works better with positions than with line numbers.
> Is there a particular reason why you keep the nlinum--current-*
> (which I think of nlinum--last-*) as a line-number rather than as
> a buffer-position (probably a marker), or is it just how it turned out?
>

Using line numbers came just intuitively. I will anyways need to get line
numbers to calculate the line diffs. So I cannot think of a way using just
positions.

I'm asking because I'm thinking that without using markers it could
> prove tricky to de-highlight the right line-number after buffer
> modifications (since it could still say "line 178" for a little while
> (e.g. jit-lock-context-time) while it's actually now the 200th line).
> So I think a marker might work better.
>

I cannot get to recreate a scenario where the said failure would happen. I
tried evaluating:

(save-excursion (goto-line (- (line-number-at-pos) 2)) (insert
"a\nb\nc\nd"))

But the line number update was fine. Adding the line number update to
post-command-hook is helping, is seems.

Also, I do not know how to do it using just markers :)

Its usefulness as a global variable runs from the end of
> a call to nlinum--current-line-update to the beginning of the next.
> During that time it holds the line-number that was current in the
> last call.
>

I have not yet made this change. This seems like chicken-egg scenario. We
have references to the current line in nlinum-highlight-current-line,
nlinum-current-line (face), is-current-line let-bound var in
nlinum-format-function, nlinum--curent-line-update function.

=====
(defvar nlinum-format-function
  (lambda (line width)
    (let* ((is-current-line (= line nlinum--current-line))
=====

Also on doing C-h v nlinum--current-line, it actually shows the current
line number.

In nlinum--current-line-update, I have a let-bound last-line to store the
previous value of nlinum--current-line. If we rename nlinum--current-line
to nlinum--last-line, then all 'current' references I listed above will
have to become 'last'. But then nlinum-highlight-last-line and
nlinum-last-line face seem non-intuitive. And then the let-bound last-line
will have to have a name like prev-to-last-line. WDYT?


>
> >> +(defun nlinum--current-line-update ()
> >> > +  "Update current line number, flush text properties for last and
> current
> >> > line."
> >> Actually, it shouldn't (and doesn't) flush text-properties.  It should
> >> only update the current-line highlighting or cause it to be
> >> updated later.
> > I did not understand this too. I refer to the remove-text-properties
> action
> > as "flushing".
>
> That's an internal detail of how it works and if we change it to work
> differently there's no reason to think that it would affect the callers,
> so it needn't be documented in the docstring.
>
> And of course it doesn't "flush text properties": it flushes one
> particular property (this nitpick is actually the detail that made me
> re-read the docstring and think harder about what it said ;-).
>

Agreed. Thanks.

And the patch now follows. I believe we need agreement only on the naming
of nlinum--current-line.

=====

From e423adb3cf91d8a7623922e2de88d678d814e370 Mon Sep 17 00:00:00 2001
From: Kaushal Modi <kaushal.modi@gmail.com>
Date: Mon, 18 Jul 2016 17:42:45 -0400
Subject: [PATCH] Add ability to highlight current line number

* nlinum.el (nlinum-highlight-current-line): New defcustom to enable
highlighting current line number when non-nil (default is nil).
        (nlinum-current-line): New face for highlighting the current
line number.
---
 packages/nlinum/nlinum.el | 79
+++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el
index 98c9cbc..6e7429c 100644
--- a/packages/nlinum/nlinum.el
+++ b/packages/nlinum/nlinum.el
@@ -36,11 +36,27 @@

 ;;; Code:

-(require 'linum)                        ;For its face.
+(require 'linum) ; For its face.
+
+(defcustom nlinum-highlight-current-line nil
+  "Whether the current line number should be highlighted.
+When non-nil, the current line number is highlighted in
`nlinum-current-line'
+face. "
+  :group 'nlinum
+  :type 'boolean)

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

+(defface nlinum-current-line
+  '((t :inherit linum :weight bold))
+  "Face for displaying current line."
+  :group 'nlinum)
+
+(defvar nlinum--current-line 0
+  "Store current line number.")
+(make-variable-buffer-local 'nlinum--current-line)
+
 ;; (defvar nlinum--desc "")

 ;;;###autoload
@@ -53,9 +69,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 +81,13 @@ 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)
+    (if nlinum-highlight-current-line
+        (add-hook 'post-command-hook #'nlinum--current-line-update nil
:local)
+      (remove-hook 'post-command-hook #'nlinum--current-line-update
:local))
+    (jit-lock-register #'nlinum--region :contextual))
   (nlinum--setup-windows))

 (defun nlinum--face-height (face)
@@ -131,6 +151,39 @@ Linum mode is a buffer-local minor mode."
                          (point-min) (point-max) '(fontified)))))
                   (current-buffer)))

+(defun nlinum--current-line-update ()
+  "Update current line number."
+  (let ((last-line nlinum--current-line))
+    (setq nlinum--current-line (save-excursion
+                                 (unless (bolp)
+                                   (forward-line 0))
+                                 (nlinum--line-number-at-pos)))
+
+    ;; Remove the text properties only if the current line has changed.
+    (when (not (eq nlinum--current-line last-line))
+      (let* ((line-diff (- last-line nlinum--current-line))
+             (last-line-beg (save-excursion
+                              (forward-line line-diff)
+                              (line-beginning-position)))
+             (last-line-end (save-excursion
+                              (forward-line (1+ line-diff))
+                              (line-beginning-position)))
+             (curr-line-beg (line-beginning-position))
+             ;; Handle the case of blank lines too.
+             ;; Using the beginning of the next line ensures that the
+             ;; curr-line-end is not equal to curr-line-beg.
+             (curr-line-end (save-excursion
+                              (forward-line 1)
+                              (line-beginning-position))))
+        ;; (message "last:%d [%d,%d], curr:%d [%d,%d], line diff:%d"
+        ;;          last-line last-line-beg last-line-end
+        ;;          nlinum--current-line curr-line-beg curr-line-end
+        ;;          line-diff)
+
+        (with-silent-modifications
+          (remove-text-properties curr-line-beg curr-line-end '(fontified))
+          (remove-text-properties last-line-beg last-line-end
'(fontified)))))))
+
 ;; (defun nlinum--ol-count ()
 ;;   (let ((i 0))
 ;;     (dolist (ol (overlays-in (point-min) (point-max)))
@@ -215,11 +268,17 @@ 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 (and nlinum-highlight-current-line
+                                  is-current-line)
+                             'nlinum-current-line
+                           '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
-- 
2.6.0.rc0.24.gec371ff

-- 

Kaushal Modi

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