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