bug#13446: 24.2; Fix loop test in linum.el

bug#13446: 24.2; Fix loop test in linum.el

[Nathan Trapuzzano]

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

There is an incorrect loop test in linum.el that potentially applies an
overlay to a line not visible in the window and thereby messes up the
width of the overlays in the lines that are visible. The patch/merge
directive is attached, and what follows is the commit message:

-----

Modify loop test in `linum-update-window'.

`limit' is set to the position returned by `window-end'; this position
is either on the last visible (logical) line in the buffer or is the
first position on the line following the last visible line. In the
former case, the loop variable never reaches the value of `limit', but
in the latter case, an overlay is applied to a line that is not
visible in the window. This can mess up the width of the overlay on
the visible lines, especially if the width of the line number (as a
string) of the line that's not visible is different from the width of
the visible lines' line numbers.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: linum.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: nbtrap@nbtrap.com-20130115010121-h7kwjyr5kimowgml
# target_branch: .
# testament_sha1: a9154d3ede2b389220646bb8e9e708117d876d01
# timestamp: 2013-01-14 20:03:26 -0500
# base_revision_id: nbtrap@nbtrap.com-20130111013646-pn4xh5r94x5asomb
# 
# Begin patch
=== modified file 'lisp/linum.el'
--- lisp/linum.el	2012-01-19 07:21:25 +0000
+++ lisp/linum.el	2013-01-15 00:45:27 +0000
@@ -151,7 +151,7 @@
     (run-hooks 'linum-before-numbering-hook)
     ;; Create an overlay (or reuse an existing one) for each
     ;; line visible in this window, if necessary.
-    (while (and (not (eobp)) (<= (point) limit))
+    (while (and (not (eobp)) (< (point) limit))
       (let* ((str (if fmt
                       (propertize (format fmt line) 'face 'linum)
                     (funcall linum-format line)))

# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIA
AACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS2
9yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA

bug#13446: 24.2; Fix loop test in linum.el

[Nathan Trapuzzano]

Any reason why this hasn't been accepted?

Nathan Trapuzzano <nbtrap@nbtrap.com> writes:

> There is an incorrect loop test in linum.el that potentially applies an
> overlay to a line not visible in the window and thereby messes up the
> width of the overlays in the lines that are visible. The patch/merge
> directive is attached, and what follows is the commit message:
>
> -----
>
> Modify loop test in `linum-update-window'.
>
> `limit' is set to the position returned by `window-end'; this position
> is either on the last visible (logical) line in the buffer or is the
> first position on the line following the last visible line. In the
> former case, the loop variable never reaches the value of `limit', but
> in the latter case, an overlay is applied to a line that is not
> visible in the window. This can mess up the width of the overlay on
> the visible lines, especially if the width of the line number (as a
> string) of the line that's not visible is different from the width of
> the visible lines' line numbers.
> # Bazaar merge directive format 2 (Bazaar 0.90)
> # revision_id: nbtrap@nbtrap.com-20130115010121-h7kwjyr5kimowgml
> # target_branch: .
> # testament_sha1: a9154d3ede2b389220646bb8e9e708117d876d01
> # timestamp: 2013-01-14 20:03:26 -0500
> # base_revision_id: nbtrap@nbtrap.com-20130111013646-pn4xh5r94x5asomb
> # 
> # Begin patch
> === modified file 'lisp/linum.el'
> --- lisp/linum.el	2012-01-19 07:21:25 +0000
> +++ lisp/linum.el	2013-01-15 00:45:27 +0000
> @@ -151,7 +151,7 @@
>      (run-hooks 'linum-before-numbering-hook)
>      ;; Create an overlay (or reuse an existing one) for each
>      ;; line visible in this window, if necessary.
> -    (while (and (not (eobp)) (<= (point) limit))
> +    (while (and (not (eobp)) (< (point) limit))
>        (let* ((str (if fmt
>                        (propertize (format fmt line) 'face 'linum)
>                      (funcall linum-format line)))
>
> # Begin bundle
> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIA
> AACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS2
> 9yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA

bug#13446: 24.2; Fix loop test in linum.el

[Nathan Trapuzzano]

Any reason why this hasn't been accepted?

bug#13446: 24.2; Fix loop test in linum.el

[Stefan Monnier]

> Any reason why this hasn't been accepted?

On my end, it's mostly for lack of time.  But having just looked at it,
I can't see what the problem is about really.  I mean, your patch looks
fine, but it's not clear what it's fixing.  I've read your explanation,
but I think it's too subtle to explain with only text.

I just installed your patch into trunk, since it looks sane.

But I'd be interested to hear a concrete description of the problematic
behavior this bug could introduce.


        Stefan


> Nathan Trapuzzano <nbtrap@nbtrap.com> writes:

>> There is an incorrect loop test in linum.el that potentially applies an
>> overlay to a line not visible in the window and thereby messes up the
>> width of the overlays in the lines that are visible. The patch/merge
>> directive is attached, and what follows is the commit message:
>> 
>> -----
>> 
>> Modify loop test in `linum-update-window'.
>> 
>> `limit' is set to the position returned by `window-end'; this position
>> is either on the last visible (logical) line in the buffer or is the
>> first position on the line following the last visible line. In the
>> former case, the loop variable never reaches the value of `limit', but
>> in the latter case, an overlay is applied to a line that is not
>> visible in the window. This can mess up the width of the overlay on
>> the visible lines, especially if the width of the line number (as a
>> string) of the line that's not visible is different from the width of
>> the visible lines' line numbers.
>> # Bazaar merge directive format 2 (Bazaar 0.90)
>> # revision_id: nbtrap@nbtrap.com-20130115010121-h7kwjyr5kimowgml
>> # target_branch: .
>> # testament_sha1: a9154d3ede2b389220646bb8e9e708117d876d01
>> # timestamp: 2013-01-14 20:03:26 -0500
>> # base_revision_id: nbtrap@nbtrap.com-20130111013646-pn4xh5r94x5asomb
>> # 
>> # Begin patch
>> === modified file 'lisp/linum.el'
>> --- lisp/linum.el	2012-01-19 07:21:25 +0000
>> +++ lisp/linum.el	2013-01-15 00:45:27 +0000
>> @@ -151,7 +151,7 @@
>> (run-hooks 'linum-before-numbering-hook)
>> ;; Create an overlay (or reuse an existing one) for each
>> ;; line visible in this window, if necessary.
>> -    (while (and (not (eobp)) (<= (point) limit))
>> +    (while (and (not (eobp)) (< (point) limit))
>> (let* ((str (if fmt
>> (propertize (format fmt line) 'face 'linum)
>> (funcall linum-format line)))
>> 
>> # Begin bundle
>> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIA
>> AACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS2
>> 9yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA

bug#13446: 24.2; Fix loop test in linum.el

[Nathan Trapuzzano]

The gist of it is that linum is in most cases applying an overlay to a
line not visible in the window, which can cause the width of the
overlays of the lines that _are_ in the window to get messed up.  Given
linum's default behavior, this is never actually a problem; it is only
potentially a problem for people who customize the linum formatting (via
the `linum-format' variable), as I found out.  Let me try to explain it
in more detail.

By default, linum will make a margin large enough to fit the line number
of the last line in the buffer.  (Technically, for the purpose of
determining the last line, linum correctly ignores the actual last line
if it is empty.  That's what the (not (eobp)) is doing.  But nevermind
that for now.)  So if the buffer is 2000 lines long (talking about
logical lines here), the margin in which the line numbers are displayed
is four characters wide, even if the window itself is only showing,
e.g., lines 200-250.

Now, suppose you want linum to display a margin only wide enough to hold
the highest line number of the lines currently displayed in the _window_
(as opposed to all the lines in the buffer).  You do this by setting
`linum-format' to "%d".  Now, if your window is showing lines 1-50, the
margin will only be two characters wide, even if there are 2000 lines in
the entire buffer.  However, this gets messes up on edge cases.  Try
this yourself: Set `linum-format' to "%d", switch to a buffer with 100
or more lines, and put point somewhere such that the highest line
displayed in the window is some line between 10 and 98 inclusive.
Notice how the margin on the left is two characters wide.  Now, put
point somewhere such that the highest line displayed is line 99.  (Also,
(1) make sure that the end of line 99 is itself visible in the window;
and (2) if 100 is the highest line in the entire buffer, make sure it is
not empty.)  Notice now that the margin for line numbers is three
characters wide--large enough to hold "100", even though line 100 is not
actually in the window.  This is caused by linum overlaying line 100,
which it shouldn't be doing.  My patch fixes this.

Nathan

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Any reason why this hasn't been accepted?
>
> On my end, it's mostly for lack of time.  But having just looked at it,
> I can't see what the problem is about really.  I mean, your patch looks
> fine, but it's not clear what it's fixing.  I've read your explanation,
> but I think it's too subtle to explain with only text.
>
> I just installed your patch into trunk, since it looks sane.
>
> But I'd be interested to hear a concrete description of the problematic
> behavior this bug could introduce.
>
>
>         Stefan
>
>
>> Nathan Trapuzzano <nbtrap@nbtrap.com> writes:
>
>>> There is an incorrect loop test in linum.el that potentially applies an
>>> overlay to a line not visible in the window and thereby messes up the
>>> width of the overlays in the lines that are visible. The patch/merge
>>> directive is attached, and what follows is the commit message:
>>> 
>>> -----
>>> 
>>> Modify loop test in `linum-update-window'.
>>> 
>>> `limit' is set to the position returned by `window-end'; this position
>>> is either on the last visible (logical) line in the buffer or is the
>>> first position on the line following the last visible line. In the
>>> former case, the loop variable never reaches the value of `limit', but
>>> in the latter case, an overlay is applied to a line that is not
>>> visible in the window. This can mess up the width of the overlay on
>>> the visible lines, especially if the width of the line number (as a
>>> string) of the line that's not visible is different from the width of
>>> the visible lines' line numbers.
>>> # Bazaar merge directive format 2 (Bazaar 0.90)
>>> # revision_id: nbtrap@nbtrap.com-20130115010121-h7kwjyr5kimowgml
>>> # target_branch: .
>>> # testament_sha1: a9154d3ede2b389220646bb8e9e708117d876d01
>>> # timestamp: 2013-01-14 20:03:26 -0500
>>> # base_revision_id: nbtrap@nbtrap.com-20130111013646-pn4xh5r94x5asomb
>>> # 
>>> # Begin patch
>>> === modified file 'lisp/linum.el'
>>> --- lisp/linum.el	2012-01-19 07:21:25 +0000
>>> +++ lisp/linum.el	2013-01-15 00:45:27 +0000
>>> @@ -151,7 +151,7 @@
>>> (run-hooks 'linum-before-numbering-hook)
>>> ;; Create an overlay (or reuse an existing one) for each
>>> ;; line visible in this window, if necessary.
>>> -    (while (and (not (eobp)) (<= (point) limit))
>>> +    (while (and (not (eobp)) (< (point) limit))
>>> (let* ((str (if fmt
>>> (propertize (format fmt line) 'face 'linum)
>>> (funcall linum-format line)))
>>> 
>>> # Begin bundle
>>> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIA
>>> AACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS2
>>> 9yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA

bug#13446: 24.2; Fix loop test in linum.el

[Stefan Monnier]

> not empty.)  Notice now that the margin for line numbers is three
> characters wide--large enough to hold "100", even though line 100 is not
> actually in the window.  This is caused by linum overlaying line 100,
> which it shouldn't be doing.  My patch fixes this.

Ah, got it!
Thank you, it makes perfect sense now,

BTW, in nlinum-mode, the default behavior is half-way between the two:
we don't try to accommodate the largest line number there can be
(partly to avoid scanning the whole buffer, which can be a performance
problem in itself), so we grow the margin only when we display a larger
number, but we don't shrink it back when moving back to the beginning of
the file with shorter line numbers.  The main reason was that I found it
annoying to have the margin grow&shrink all the time in those cases
where you're working right around line 100 or 1000.


        Stefan

bug#13446: 24.2; Fix loop test in linum.el

[Nathan Trapuzzano]

The behavior you describe is actually doable using a combination of
`linum-before-numbering-hook' and `linum-format'.  (In particular, I'm
talking about setting `linum-format' to a symbol, which will cause linum
to call the function with that symbol's name to retrieve the line number
as a string.)  I know because I've implemented it, or something very
similar to it, myself.

Indeed, given these two variables, linum is actually very capable.  On
the other hand, one seeminly irreparable problem with linum is _when_ it
applies the overlays--or when it doesn't.  I'm not sure if there's a bug
report for this, but faily often linum won't display the overlays at
all, or will only display the line numbers on some of the lines in the
window.  This happens especially frequently when bouncing on balanced
parens/brackets via `forward-' and `backward-sexp'.  If something is
going to replace linum, I'd be very interested to know whether it fixes
this problem.  Do you happen to know?

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> BTW, in nlinum-mode, the default behavior is half-way between the two:
> we don't try to accommodate the largest line number there can be
> (partly to avoid scanning the whole buffer, which can be a performance
> problem in itself), so we grow the margin only when we display a larger
> number, but we don't shrink it back when moving back to the beginning of
> the file with shorter line numbers.

bug#13446: 24.2; Fix loop test in linum.el

[Stefan Monnier]

> parens/brackets via `forward-' and `backward-sexp'.  If something is
> going to replace linum, I'd be very interested to know whether it fixes
> this problem.  Do you happen to know?

The intended replacement is nlinum.el (you can install it via M-x
list-packages).  But it is not as hackable as linum.el (nothing
equivalent to linum-before-numbering-hook).


        Stefan

> Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> BTW, in nlinum-mode, the default behavior is half-way between the two:
>> we don't try to accommodate the largest line number there can be
>> (partly to avoid scanning the whole buffer, which can be a performance
>> problem in itself), so we grow the margin only when we display a larger
>> number, but we don't shrink it back when moving back to the beginning of
>> the file with shorter line numbers.