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