Patch to highlight current line number [nlinum.el]

Patch to highlight current line number [nlinum.el]

[Kaushal Modi]

[-- Attachment #1.1: Type: text/plain, Size: 4762 bytes --]

Hi Stefan and emacs-devel,

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.

By submitting this patch to the list, I am looking forward to how the
performance can be improved. Right now, this works as I want but I do this
by flushing the text properties in post-command-hook. I do not know enough
about jit-lock to use it or how to use it to update the face for the
current line number each time the cursor moves across lines. What's the
best way?

I am also looking forward for a review to find a bug because at times, very
rarely (once a month or so), I see about 15-20 consecutive lines set to the
same nlinum-current-line-face. As I cannot reproduce that issue, I don't
know where to start the debug from. Hopefully review of the below patch by
more experienced eyes can help catch this bug?

Thanks.

The result of this patch looks like this in my theme:

[image: pasted1]
The first draft of the patch follows:

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el
index 98c9cbc..02a7e0b 100644
--- a/packages/nlinum/nlinum.el
+++ b/packages/nlinum/nlinum.el
@@ -41,6 +41,14 @@
 (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 before jit-lock.")
+
 ;; (defvar nlinum--desc "")

 ;;;###autoload
@@ -51,7 +59,7 @@ and disable it otherwise.  If called from Lisp, enable
the mode
 if ARG is omitted or nil.

 Linum mode is a buffer-local minor mode."
-  :lighter nil ;; (" NLinum" nlinum--desc)
+  :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)
@@ -60,6 +68,7 @@ Linum mode is a buffer-local minor mode."
   (remove-overlays (point-min) (point-max) 'nlinum t)
   ;; (kill-local-variable 'nlinum--ol-counter)
   (kill-local-variable 'nlinum--width)
+  (remove-hook 'post-command-hook #'nlinum--current-line-update t)
   (when nlinum-mode
     ;; FIXME: Another approach would be to make the mode permanent-local,
     ;; which might indeed be preferable.
@@ -67,7 +76,8 @@ Linum mode is a buffer-local minor mode."
     (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))
+    (jit-lock-register #'nlinum--region t)
+    (add-hook 'post-command-hook #'nlinum--current-line-update nil t))
   (nlinum--setup-windows))

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

+(defun nlinum--current-line-update ()
+  "Reflush display on current window"
+  (when nlinum-mode
+    (nlinum--after-change)
+    (setq nlinum--current-line (string-to-number (format-mode-line "%l")))
+    ;; 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))))))
+
 ;; (defun nlinum--ol-count ()
 ;;   (let ((i 0))
 ;;     (dolist (ol (overlays-in (point-min) (point-max)))
@@ -215,11 +241,15 @@ Used by the default `nlinum-format-function'."

 (defvar nlinum-format-function
   (lambda (line width)
-    (let ((str (format nlinum-format line)))
+    (let* ((line-diff (abs (- line nlinum--current-line)))
+           (current-line-p (eq line-diff 0))
+           (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)
+      (if current-line-p
+          (put-text-property 0 width 'face 'nlinum-current-line-face str)
+        (put-text-property 0 width '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

[1]: https://github.com/CodeFalling/nlinum-relative
-- 

Kaushal Modi

[-- Attachment #1.2: Type: text/html, Size: 6541 bytes --]

[-- Attachment #2: pasted1 --]
[-- Type: image/png, Size: 46826 bytes --]

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

[Stefan Monnier]

> 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

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

[Kaushal Modi]

[-- 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 --]

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

[Stefan Monnier]

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

Not specifically.  Just that it does something similar.

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

Actually, after thinking about it, I got to the conclusion that as long
as you don't try to handle the multiple-window case correctly, you're
probably better off with post-command-hook.

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

More or less.  Try M-x nhexl-mode RET and the C-x 5 2 and then move
around the buffer and compare the two window's highlighting of the
"current line".  You'll see that only one of the two is correct.

> From quick trial, I saw the line numbers change in the whole buffer even
> when I moved the cursor horizontally.

You need to always call it with point at BOL.

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

Sorry, the problem didn't jump at me.  They look pretty good.

FWIW in hexl-mode, instead of removing the `fontified' property, I just
directly called the function that refreshes the overlays.  Not sure why
I did that (seems less efficient/elegant than your approach).

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

Clearly the above change should come with the corresponding change in
nlinum-current-line-face (i.e. it shouldn't inherit from `linum' face
any more).  But either way is OK.

Oh, and nowadays the convention is to use "-face" only for
names of variables whose value is a face, but not for the face names
themselves.  The font-lock-foo-face faces are the main exceptions
because they're already so omnipresent.

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

That's right.

> I have also made one cosmetic change .. Instead of `t' as argument values,
> I have replaced them with `:local' or `:contextual' as appropriate.

Fine.  I often use 'local (rather than :local) for that same purpose,
but I don't really care about the color of this shed.

> +(defvar-local nlinum--last-line 0
> +  "Store line number where the point was before it moved to the current
> line.")

No reason to keep this as a global var (but I'd rename
nlinum--current-line to nlinum--last-line).

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


        Stefan

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

[Kaushal Modi]

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

Thanks Stefan,

My comments are below.

On Mon, Jul 18, 2016 at 9:40 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Actually, after thinking about it, I got to the conclusion that as long
> as you don't try to handle the multiple-window case correctly, you're
> probably better off with post-command-hook.
>

Yes, post-command-hook works fine for my use case.

More or less.  Try M-x nhexl-mode RET and the C-x 5 2 and then move
> around the buffer and compare the two window's highlighting of the
> "current line".  You'll see that only one of the two is correct.
>

I see what you mean. I tried that. If I wanted to do something like that,
C-x 4 c (indirect buffer cloning) seems like a good solution.


> > From quick trial, I saw the line numbers change in the whole buffer even
> > when I moved the cursor horizontally.
>
> You need to always call it with point at BOL.
>

Got it. Below works

(setq nlinum--current-line (save-excursion                                ;
works
                               (unless (bolp)
                                 (forward-line 0))
                               (nlinum--line-number-at-pos)))

> 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.
>
> Sorry, the problem didn't jump at me.  They look pretty good.
>

Do you mean that the commented code to remove text properties from
last/current lines should have worked?

Here is a longish gif video (1 min 20 secs) demonstrating the issue I see
when removing text properties from the whole visible window vs just
curr+last lines: http://i.imgur.com/GN3zTlJ.gifv

Note that nlinum--current-line-update is called in post-command-hook.


> Clearly the above change should come with the corresponding change in
> nlinum-current-line-face (i.e. it shouldn't inherit from `linum' face
> any more).  But either way is OK.
>

OK, I will then stick with linum inheritance in the defface itself so that
people can change that in their themes if they wish.


> Oh, and nowadays the convention is to use "-face" only for
> names of variables whose value is a face, but not for the face names
> themselves.  The font-lock-foo-face faces are the main exceptions
> because they're already so omnipresent.
>

OK, I have made that change locally; will be seen in the final patch.


> > I have also made one cosmetic change .. Instead of `t' as argument
> values,
> > I have replaced them with `:local' or `:contextual' as appropriate.
>
> Fine.  I often use 'local (rather than :local) for that same purpose,
> but I don't really care about the color of this shed.
>

Thanks for confirming.


> > +(defvar-local nlinum--last-line 0
> > +  "Store line number where the point was before it moved to the current
> > line.")
>
> No reason to keep this as a global var (but I'd rename
> nlinum--current-line to nlinum--last-line).
>

Correct. nlinum--last-line does not need to be a defvar; I have now made it
a let-bound var. I did not understand why nlinum--current-line should be
renamed as nlinum--last-line; because that var is storing the current line
number and we are using that to highlight the current line number.

> +(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 function is updating the nlinum--current-line defvar
and removing text properties from the whole visible window (or not working
presently, current and last lines).
-- 

Kaushal Modi

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

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

[Stefan Monnier]

> I see what you mean.  I tried that.  If I wanted to do something like that,
> C-x 4 c (indirect buffer cloning) seems like a good solution.

Indirect buffers are very messy.  They work fine until they don't.

>> Sorry, the problem didn't jump at me.  They look pretty good.
> Do you mean that the commented code to remove text properties from
> last/current lines should have worked?

Yes.  Well not "should have worked" but more "can't see any obvious
reason why it wouldn't do the right thing".

> Here is a longish gif video (1 min 20 secs) demonstrating the issue I see
> when removing text properties from the whole visible window vs just
> curr+last lines: http://i.imgur.com/GN3zTlJ.gifv

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.

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?

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.

>> > +(defvar-local nlinum--last-line 0
>> > +  "Store line number where the point was before it moved to the current
>> > line.")
>> No reason to keep this as a global var (but I'd rename
>> nlinum--current-line to nlinum--last-line).
> Correct. nlinum--last-line does not need to be a defvar; I have now made it
> a let-bound var.

Thanks.

> I did not understand why nlinum--current-line should be renamed as
> nlinum--last-line; because that var is storing the current line number
> and we are using that to highlight the current line number.

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.

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


        Stefan

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

[Kaushal Modi]

[-- 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 --]

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

[Kaushal Modi]

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

On Mon, Jul 18, 2016 at 6:05 PM Kaushal Modi <kaushal.modi@gmail.com> wrote:

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

Here is a slightly optimized version of the same patch that results in
lesser save-excursion and remove-text-properties calls in
nlinum--current-line-update function:

I would like to optimize this even more.. I feel lag when I keep C-n/C-p
pressed with nlinum-highlight-current-line set to t vs nil. Any pointers?
You mentioned the use of markers instead of line numbers. How to use those
if that could improve the performance?

From 7eb48b322578cc199eaa45de6bfe778e2b5f63de 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 | 88
+++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el
index 98c9cbc..093d217 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,48 @@ 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)))
+
+    (let ((line-diff (- last-line nlinum--current-line))
+          beg end rm-text-prop)
+      (cond
+       ;; If point is moving upwards, set BEG to be the line-beginning of
+       ;; the current line, and END to be the line-beginning of the next
+       ;; line after the last line.
+       ((> line-diff 0)
+        (setq beg (line-beginning-position)) ; beginning of the current
line
+        ;; Handle the case of blank lines too.
+        ;; Using the beginning of the next line ensures that the
+        ;; BEG and END are never equal.
+        (setq end (save-excursion ; beginning of the line after the last
line
+                    (forward-line (1+ line-diff))
+                    (line-beginning-position)))
+        (setq rm-text-prop 1))
+
+       ;; If point is moving downwards, set BEG to be the line-beginning of
+       ;; the last line, and END to be the line-beginning of the next
+       ;; line after the current line.
+       ((< line-diff 0)
+        (setq beg (save-excursion ; beginning of the last line
+                    (forward-line line-diff)
+                    (line-beginning-position)))
+        (setq end (save-excursion ; beginning of the line after the
current line
+                    (forward-line 1)
+                    (line-beginning-position)))
+        (setq rm-text-prop 1)))
+
+      ;; Remove the text properties only if the current line has changed,
+      ;; i.e. line-diff != 0
+      (when rm-text-prop
+        (with-silent-modifications
+          (remove-text-properties beg end '(fontified)))))))
+
 ;; (defun nlinum--ol-count ()
 ;;   (let ((i 0))
 ;;     (dolist (ol (overlays-in (point-min) (point-max)))
@@ -215,11 +277,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: 9957 bytes --]

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

[Stefan Monnier]

> It works! I assumed incorrectly that line-beginning-position and
> line-end-position accept negative arguments too.

They do.  But (line-beginning-position 1) gives the line-beg of current
line, and (line-beginning-position 0) gives line-beg of previous line,
and (line-beginning-position -1) gives line-beg of the line before that.

> Using line numbers came just intuitively.  I will anyways need to get line
> numbers to calculate the line diffs.

Why do you need line-diff?

> So I cannot think of a way using just positions.

That's because you haven't spent enough time hacking on Elisp yet ;-)

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

I'm thinking more about a case like

   (progn
     (forward-line -2)
     (insert "a\nb\n")
     (forward-line -4))

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

Ideally, I'd keep a global var
nlinum--last-current-line-highlighted-overlay which (as its name
implies) keeps track of the overlay that holds the (one and only, tho we
could make it more robust and make it into a list, just in case) line
number highlighted as "current".  Then in nlinum--current-line-update
you just remove-text-properties at the position of this overlay (plus at
the position of point).

Now, there's a problem with this, because the code which creates the
overlay doesn't know whether that's the "current-line-highlighted"
overlay or not: this decision is made in nlinum-format-function which
just returns the string that should be put into the overlay.
And nlinum-format-function is meant to be somewhat customized (that's
why it's a var) so we really don't know what happens with the string
we return.  It could be put into an overlay as-is but it could just as
well be replaced with something else.

But it's probably sufficient to change nlinum-format-function so it just
sets a global nlinum--last-current-line-highlighted-marker to (point)
and then in the post-command-hook when that marker points somewhere,
just remove-text-properties between
nlinum--last-current-line-highlighted-marker and (1+
nlinum--last-current-line-highlighted-marker).

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

Don't worry about it.  The current name (pun intended) is fine.

> If we rename nlinum--current-line to nlinum--last-line, then all
> 'current' references I listed above will have to become 'last'.

No, they don't have to use the same name.

> And the patch now follows.

Looks good, feel free to install it.


        Stefan


> -(require 'linum)                        ;For its face.
> +(require 'linum) ; For its face.

Hmm... M-; doesn't agree with this comment indentation.
How did you get that?  If you want this indentation (I don't have an
opinion on that one), feel free to do it, but do it with ";;" so M-;
will place it as you want.

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

Hmm... we don't have an `nlinum' group yet, AFAIK.
Feel free to add one, and then we can drop the ":group 'nlinum"
args altogether.

Otherwise, we can simply keep using the `linum' group.

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

[Kaushal Modi]

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

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

> They do.  But (line-beginning-position 1) gives the line-beg of current
> line, and (line-beginning-position 0) gives line-beg of previous line,
> and (line-beginning-position -1) gives line-beg of the line before that.
>

Duh, yeah. I somehow missed that earlier in hasty re-evals. Thanks, I got
rid of those save-excursions in nlinum--current-line-update.


> > Using line numbers came just intuitively.  I will anyways need to get
> line
> > numbers to calculate the line diffs.
>
> Why do you need line-diff?
>

I need the line-diff to get the line begin/end positions for the last-line.
line-begining/end-position functions accept 'delta' as arguments.

=====
        (if (natnump line-diff)
            (progn
              (setq beg (line-beginning-position)) ; beginning of the
current line
              (setq end (line-end-position (1+ line-diff))) ; end of the
last line
              ;; Handle the case of blank lines too.
              (setq end (min (point-max) (1+ end))))
          ;; If point is moving downwards
          (setq beg (line-beginning-position (1+ line-diff))) ; beginning
of the last line
          (setq end (line-end-position)) ; end of the current line
          ;; Handle the case of blank lines too.
          (setq end (min (point-max) (1+ end))))
=====


> > So I cannot think of a way using just positions.
>
> That's because you haven't spent enough time hacking on Elisp yet ;-)
>

Yes, getting better. My future commit could be an improvement upon this.

   (progn
>      (forward-line -2)
>      (insert "a\nb\n")
>      (forward-line -4))
>

That case worked fine too.


> > Also, I do not know how to do it using just markers :)
>
> Ideally, I'd keep a global var
> nlinum--last-current-line-highlighted-overlay which (as its name
> implies) keeps track of the overlay that holds the (one and only, tho we
> could make it more robust and make it into a list, just in case) line
> number highlighted as "current".  Then in nlinum--current-line-update
> you just remove-text-properties at the position of this overlay (plus at
> the position of point).
>
> Now, there's a problem with this, because the code which creates the
> overlay doesn't know whether that's the "current-line-highlighted"
> overlay or not: this decision is made in nlinum-format-function which
> just returns the string that should be put into the overlay.
> And nlinum-format-function is meant to be somewhat customized (that's
> why it's a var) so we really don't know what happens with the string
> we return.  It could be put into an overlay as-is but it could just as
> well be replaced with something else.
>
> But it's probably sufficient to change nlinum-format-function so it just
> sets a global nlinum--last-current-line-highlighted-marker to (point)
> and then in the post-command-hook when that marker points somewhere,
> just remove-text-properties between
> nlinum--last-current-line-highlighted-marker and (1+
> nlinum--last-current-line-highlighted-marker).
>

I sort of understood the thought process. I will work on that in near
future.


> Looks good, feel free to install it.
>

Thanks. I have attached updated patch with the email with new defgroup
nlinum, changes in defcustom groups and nlinum--current-line-update update.

> -(require 'linum)                        ;For its face.
> > +(require 'linum) ; For its face.
>

I have fixed this.


> Hmm... M-; doesn't agree with this comment indentation.
> How did you get that?


I used to manually do C-e SPC ;. Now I am starting to do "M-;".


> If you want this indentation (I don't have an
> opinion on that one), feel free to do it, but do it with ";;" so M-;
> will place it as you want.
>
> > +(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)
>
> Hmm... we don't have an `nlinum' group yet, AFAIK.
> Feel free to add one, and then we can drop the ":group 'nlinum"
> args altogether.
>
> Otherwise, we can simply keep using the `linum' group.
>

OK, I have defined an nlinum group.

One reason was that it looked like the nlinum group was anyways
autogenerated when gloabl-nlinum-mode was created. It just wasn't formally
defined. So it was missing the description. On evaluating "(customize-group
'nlinum)", it gave:

-----
Nlinum group:  Group definition missing.
       State : visible group members are all at standard values.

Show Value Global Nlinum Mode
   Non-nil if Global Nlinum mode is enabled. More
-----

===== patch follows

From 06e4209cea3a860c3329d3f885605736fb414e48 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 | 87
+++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 11 deletions(-)

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el
index 98c9cbc..79f6891 100644
--- a/packages/nlinum/nlinum.el
+++ b/packages/nlinum/nlinum.el
@@ -4,7 +4,7 @@

 ;; Author: Stefan Monnier <monnier@iro.umontreal.ca>
 ;; Keywords: convenience
-;; Version: 1.6
+;; Version: 1.7

 ;; This program is free software; you can redistribute it and/or modify
 ;; it under the terms of the GNU General Public License as published by
@@ -26,6 +26,11 @@

 ;;; News:

+;; v1.7:
+;; - Add ability to highlight current line number.
+;; - New custom variable `nlinum-highlight-current-line' and
+;;   face `nlinum-current-line'.
+
 ;; v1.3:
 ;; - New custom variable `nlinum-format'.
 ;; - Change in calling convention of `nlinum-format-function'.
@@ -36,11 +41,32 @@

 ;;; Code:

-(require 'linum)                        ;For its face.
+(require 'linum)                        ; for its face
+
+(defgroup nlinum nil
+  "Show line numbers in the margin, (hopefully) more efficiently."
+  :group 'convenience
+  :prefix "nlinum")
+
+(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)
+
+(defface nlinum-current-line
+  '((t :inherit linum :weight bold))
+  "Face for displaying current line."
+  :group 'nlinum)

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

+(defvar nlinum--current-line 0
+  "Store current line number.")
+(make-variable-buffer-local 'nlinum--current-line)
+
 ;; (defvar nlinum--desc "")

 ;;;###autoload
@@ -53,9 +79,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 +91,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 +161,35 @@ 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)))
+
+    (let ((line-diff (- last-line nlinum--current-line))
+          beg end)
+      ;; Remove the text properties only if the current line has changed
+      (when (not (zerop line-diff))
+        ;; If point is moving upwards
+        (if (natnump line-diff)
+            (progn
+              (setq beg (line-beginning-position)) ; beginning of the
current line
+              (setq end (line-end-position (1+ line-diff))) ; end of the
last line
+              ;; Handle the case of blank lines too.
+              (setq end (min (point-max) (1+ end))))
+          ;; If point is moving downwards
+          (setq beg (line-beginning-position (1+ line-diff))) ; beginning
of the last line
+          (setq end (line-end-position)) ; end of the current line
+          ;; Handle the case of blank lines too.
+          (setq end (min (point-max) (1+ end))))
+        ;; (message "curr-line:%d [beg/end:%d/%d] -- last-line:%d"
+        ;;          nlinum--current-line beg end last-line)
+        (with-silent-modifications
+          (remove-text-properties beg end '(fontified)))))))
+
 ;; (defun nlinum--ol-count ()
 ;;   (let ((i 0))
 ;;     (dolist (ol (overlays-in (point-min) (point-max)))
@@ -215,11 +274,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: 16551 bytes --]

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

[Stefan Monnier]

> I need the line-diff to get the line begin/end positions for the last-line.

Exactly: if you have a marker showing you where is that "last-line", you
don't need the line-diff.  If you really want the beg/end of that line,
you `goto-char' to that marker and then use line-beginning-position and
line-end-position from there with no argument.

>> (progn
>>   (forward-line -2)
>>   (insert "a\nb\n")
>>   (forward-line -4))
> That case worked fine too.

Hmm... I wonder how, but in any case, that's good.

> One reason was that it looked like the nlinum group was anyways
> autogenerated when gloabl-nlinum-mode was created. It just wasn't formally
> defined. So it was missing the description. On evaluating "(customize-group
> 'nlinum)", it gave:

Oh, the gloabl-nlinum-mode was missing a ":group 'linum" argument!

> ===== patch follows

Please install it, and thank you very much.


        Stefan

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

[Kaushal Modi]

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

On Tue, Jul 19, 2016 at 8:28 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > I need the line-diff to get the line begin/end positions for the
> last-line.
>
> Exactly: if you have a marker showing you where is that "last-line", you
> don't need the line-diff.  If you really want the beg/end of that line,
> you `goto-char' to that marker and then use line-beginning-position and
> line-end-position from there with no argument.
>

Got it. I will work on that optimization as soon as I get chance.


> Oh, the gloabl-nlinum-mode was missing a ":group 'linum" argument!
>

OK, I added ":group 'nlinum" in the latest patch now.


> > ===== patch follows
>
> Please install it, and thank you very much.


The final (for now) patch is below. Thanks for all your help. Can you
please commit it? I do not have commit rights, but my copyright assignment
is on file.

=====
From c9c5e4a5a158cde292353ba8e0669e5877365a5a 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 | 90
++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/packages/nlinum/nlinum.el b/packages/nlinum/nlinum.el
index 98c9cbc..0e7cf06 100644
--- a/packages/nlinum/nlinum.el
+++ b/packages/nlinum/nlinum.el
@@ -4,7 +4,7 @@

 ;; Author: Stefan Monnier <monnier@iro.umontreal.ca>
 ;; Keywords: convenience
-;; Version: 1.6
+;; Version: 1.7

 ;; This program is free software; you can redistribute it and/or modify
 ;; it under the terms of the GNU General Public License as published by
@@ -26,6 +26,11 @@

 ;;; News:

+;; v1.7:
+;; - Add ability to highlight current line number.
+;; - New custom variable `nlinum-highlight-current-line' and
+;;   face `nlinum-current-line'.
+
 ;; v1.3:
 ;; - New custom variable `nlinum-format'.
 ;; - Change in calling convention of `nlinum-format-function'.
@@ -36,11 +41,32 @@

 ;;; Code:

-(require 'linum)                        ;For its face.
+(require 'linum)                        ; for its face
+
+(defgroup nlinum nil
+  "Show line numbers in the margin, (hopefully) more efficiently."
+  :group 'convenience
+  :prefix "nlinum")
+
+(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)
+
+(defface nlinum-current-line
+  '((t :inherit linum :weight bold))
+  "Face for displaying current line."
+  :group 'nlinum)

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

+(defvar nlinum--current-line 0
+  "Store current line number.")
+(make-variable-buffer-local 'nlinum--current-line)
+
 ;; (defvar nlinum--desc "")

 ;;;###autoload
@@ -53,9 +79,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 +91,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 +161,35 @@ 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)))
+
+    (let ((line-diff (- last-line nlinum--current-line))
+          beg end)
+      ;; Remove the text properties only if the current line has changed
+      (when (not (zerop line-diff))
+        ;; If point is moving upwards
+        (if (natnump line-diff)
+            (progn
+              (setq beg (line-beginning-position)) ; beginning of the
current line
+              (setq end (line-end-position (1+ line-diff))) ; end of the
last line
+              ;; Handle the case of blank lines too.
+              (setq end (min (point-max) (1+ end))))
+          ;; If point is moving downwards
+          (setq beg (line-beginning-position (1+ line-diff))) ; beginning
of the last line
+          (setq end (line-end-position)) ; end of the current line
+          ;; Handle the case of blank lines too.
+          (setq end (min (point-max) (1+ end))))
+        ;; (message "curr-line:%d [beg/end:%d/%d] -- last-line:%d"
+        ;;          nlinum--current-line beg end last-line)
+        (with-silent-modifications
+          (remove-text-properties beg end '(fontified)))))))
+
 ;; (defun nlinum--ol-count ()
 ;;   (let ((i 0))
 ;;     (dolist (ol (overlays-in (point-min) (point-max)))
@@ -215,11 +274,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
@@ -260,7 +325,8 @@ it may cause the margin to be resized and line numbers
to be recomputed.")

 ;;;###autoload
 (define-globalized-minor-mode global-nlinum-mode nlinum-mode
-  (lambda () (unless (minibufferp) (nlinum-mode))))
+  (lambda () (unless (minibufferp) (nlinum-mode)))
+  :group 'nlinum)

 (provide 'nlinum)
 ;;; nlinum.el ends here
-- 
2.6.0.rc0.24.gec371ff


-- 

Kaushal Modi

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

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

[Stefan Monnier]

> The final (for now) patch is below. Thanks for all your help. Can you
> please commit it? I do not have commit rights, but my copyright assignment
> is on file.

Installed,


        Stefan

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

[Kaushal Modi]

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

On Wed, Jul 20, 2016 at 3:33 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Installed,
>

Thanks for fine-tuning the patch futher.

BTW, I noticed that the :group 'nlinum tag for global-nlinum-mode is still
absent. But my patch had that. Was that intentional?
-- 

Kaushal Modi

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

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

[Stefan Monnier]

> BTW, I noticed that the :group 'nlinum tag for global-nlinum-mode is still
> absent. But my patch had that. Was that intentional?

Yes.  Its absence was a problem when there was no `nlinum' group
(i.e. we should have had ":group 'linum" before), but now that we do
have an `nlinum' group, the default behavior DTRT already.


        Stefan

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

[Kaushal Modi]

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

Got it. Thanks for helping with the patch and committing it!

On Wed, Jul 20, 2016 at 4:25 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Yes.  Its absence was a problem when there was no `nlinum' group
> (i.e. we should have had ":group 'linum" before), but now that we do
> have an `nlinum' group, the default behavior DTRT already.
>
-- 

Kaushal Modi

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