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

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 <address@hidden> 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 <address@hidden>

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

From:	Kaushal Modi
Subject:	Re: Patch to highlight current line number [nlinum.el]
Date:	Mon, 18 Jul 2016 22:05:43 +0000