bug#14170: 24.3; linum won't create all overlays after a folding

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

Steps to reproduce:
Open emacs -Q
M-: (progn (linum-mode) (hs-mode))
Insert a block of comments
M-x hs-hide-block

This might be happening because (window-end nil t) does not return an
updated value after the creation of a "hiding" overlay. Although I haven't
explored it deep enough, to be sure that it is indeed so.

Evgeni

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

bug#14170: 24.3; linum won't create all overlays after a folding

[Stefan Monnier]

> Steps to reproduce:
> Open emacs -Q
> M-: (progn (linum-mode) (hs-mode))
> Insert a block of comments
> M-x hs-hide-block

Could you test it with nlinum-mode (available from GNU ELPA)?


        Stefan

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

nlinum works correctly.

Evgeni


On Wed, Apr 10, 2013 at 1:38 AM, Stefan Monnier <monnier@iro.umontreal.ca>wrote:

> > Steps to reproduce:
> > Open emacs -Q
> > M-: (progn (linum-mode) (hs-mode))
> > Insert a block of comments
> > M-x hs-hide-block
>
> Could you test it with nlinum-mode (available from GNU ELPA)?
>
>
>         Stefan
>

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

bug#14170: 24.3; linum won't create all overlays after a folding

[Glenn Morris]

E Sabof wrote:

> nlinum works correctly.

This is the answer to most recent bug reports involving linum, as I
recall. Will nlinum replace linum at some point?

bug#14170: 24.3; linum won't create all overlays after a folding

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 09 Apr 2013 21:51:59 -0400
> 
> E Sabof wrote:
> 
> > nlinum works correctly.
> 
> This is the answer to most recent bug reports involving linum, as I
> recall. Will nlinum replace linum at some point?

Btw, if nlinum is our response to the problems in linum, then nlinum
should be a bit more friendly to bidirectional text, and determine
whether to put numbers on the left or right margin by looking at what
current-bidi-paragraph-direction returns.  (Yes, this means that if
some paragraphs are L2R and others R2L, some numbers will be on the
left, while others on the right.)

bug#14170: 24.3; linum won't create all overlays after a folding

[Stefan Monnier]

>> > nlinum works correctly.
>> This is the answer to most recent bug reports involving linum, as I
>> recall. Will nlinum replace linum at some point?
> Btw, if nlinum is our response to the problems in linum, then nlinum
> should be a bit more friendly to bidirectional text, and determine
> whether to put numbers on the left or right margin by looking at what
> current-bidi-paragraph-direction returns.  (Yes, this means that if
> some paragraphs are L2R and others R2L, some numbers will be on the
> left, while others on the right.)

Patches welcome, but I don't think we need to worry a bout this w.r.t
moving from linum.el to nlinum.el since linum.el doesn't support R2L
paragraphs either.


        Stefan

bug#14170: 24.3; linum won't create all overlays after a folding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Glenn Morris <rgm@gnu.org>,  14170@debbugs.gnu.org
> Date: Sat, 13 Apr 2013 21:21:22 -0400
> 
> >> > nlinum works correctly.
> >> This is the answer to most recent bug reports involving linum, as I
> >> recall. Will nlinum replace linum at some point?
> > Btw, if nlinum is our response to the problems in linum, then nlinum
> > should be a bit more friendly to bidirectional text, and determine
> > whether to put numbers on the left or right margin by looking at what
> > current-bidi-paragraph-direction returns.  (Yes, this means that if
> > some paragraphs are L2R and others R2L, some numbers will be on the
> > left, while others on the right.)
> 
> Patches welcome

Below.  (There's a missing feature in bidi display that causes the
numbers to be displayed backwards in R2L paragraphs; I will fix that
later.)

> but I don't think we need to worry a bout this w.r.t moving from
> linum.el to nlinum.el since linum.el doesn't support R2L paragraphs
> either.

Well, I hope you allow me to worry about that ;-)

=== modified file 'packages/nlinum/nlinum.el'
--- packages/nlinum/nlinum.el	2012-10-24 19:29:40 +0000
+++ packages/nlinum/nlinum.el	2013-04-14 06:09:13 +0000
@@ -35,7 +35,7 @@
 
 ;;;###autoload
 (define-minor-mode nlinum-mode
-  "Toggle display of line numbers in the left margin (Linum mode).
+  "Toggle display of line numbers in the margin (Linum mode).
 With a prefix argument ARG, enable Linum mode if ARG is positive,
 and disable it otherwise.  If called from Lisp, enable the mode
 if ARG is omitted or nil.
@@ -55,9 +55,21 @@ Linum mode is a buffer-local minor mode.
     (jit-lock-register #'nlinum--region t))
   (nlinum--setup-windows))
 
+(defvar margin-side nil)
 (defun nlinum--setup-window ()
-  (set-window-margins nil (if nlinum-mode nlinum--width)
-                      (cdr (window-margins))))
+  (cond ((eq bidi-paragraph-direction 'left-to-right)
+	 (set-window-margins nil (if nlinum-mode nlinum--width)
+			     (cdr (window-margins)))
+	 (setq-local margin-side 'left))
+	((eq bidi-paragraph-direction 'right-to-left)
+	 (set-window-margins nil (car (window-margins))
+			     (if nlinum-mode nlinum--width))
+	 (setq-local margin-side 'right))
+	(t
+	 (set-window-margins nil
+			     (if nlinum-mode nlinum--width)
+			     (if nlinum-mode nlinum--width))
+	 (setq-local margin-side nil))))
 
 (defun nlinum--setup-windows ()
   (dolist (win (get-buffer-window-list nil nil t))
@@ -157,7 +169,14 @@ Linum mode is a buffer-local minor mode.
             (and (not (eobp)) (< (point) limit)
                  (let* ((ol (make-overlay (point) (1+ (point))))
                         (str (format fmt line))
-                        (width (string-width str)))
+                        (width (string-width str))
+			(side
+			 (cond ((eq margin-side 'left) 'left-margin)
+			       ((eq margin-side 'right) 'right-margin)
+			       (t (if (eq (current-bidi-paragraph-direction)
+					  'right-to-left)
+				      'right-margin
+				    'left-margin)))))
                    (when (< nlinum--width width)
                      (setq nlinum--width width)
                      (nlinum--new-width))
@@ -165,7 +184,7 @@ Linum mode is a buffer-local minor mode.
                    (overlay-put ol 'evaporate t)
                    (overlay-put ol 'before-string
                                 (propertize " " 'display
-                                            `((margin left-margin)
+                                            `((margin ,side)
                                               ,(propertize str
                                                            'face 'linum))))
                    ;; (setq nlinum--ol-counter (1- nlinum--ol-counter))

bug#14170: 24.3; linum won't create all overlays after a folding

[Stefan Monnier]

> +(defvar margin-side nil)

I suspect you wanted to use nlinum--margin-side instead, right?

>  (defun nlinum--setup-window ()
> -  (set-window-margins nil (if nlinum-mode nlinum--width)
> -                      (cdr (window-margins))))
> +  (cond ((eq bidi-paragraph-direction 'left-to-right)
> +	 (set-window-margins nil (if nlinum-mode nlinum--width)
> +			     (cdr (window-margins)))
> +	 (setq-local margin-side 'left))
> +	((eq bidi-paragraph-direction 'right-to-left)
> +	 (set-window-margins nil (car (window-margins))
> +			     (if nlinum-mode nlinum--width))
> +	 (setq-local margin-side 'right))
> +	(t
> +	 (set-window-margins nil
> +			     (if nlinum-mode nlinum--width)
> +			     (if nlinum-mode nlinum--width))
> +	 (setq-local margin-side nil))))

Hmm... since the default value of bidi-paragraph-direction is nil, that
means that this will change the default behavior and put a margin in
each side.

Maybe we should decide nlinum--margin-side "lazily" based on the
buffer's content, so if bidi-paragraph-direction is nil we only set
a margin on the sides where we do display line numbers.


        Stefan

bug#14170: 24.3; linum won't create all overlays after a folding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: rgm@gnu.org, 14170@debbugs.gnu.org
> Date: Mon, 15 Apr 2013 19:00:46 -0400
> 
> > +(defvar margin-side nil)
> 
> I suspect you wanted to use nlinum--margin-side instead, right?

Yes, I suspect so, too.

> Hmm... since the default value of bidi-paragraph-direction is nil, that
> means that this will change the default behavior and put a margin in
> each side.

Not in the major modes where line numbers are normally wanted: every
mode that inherits from prog-mode has its bidi-paragraph-direction set
as left-to-right by default.

For other modes, yes.

> Maybe we should decide nlinum--margin-side "lazily" based on the
> buffer's content, so if bidi-paragraph-direction is nil we only set
> a margin on the sides where we do display line numbers.

Patches are welcome ;-)

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

The reason I've noticed/reported this bug was because I was working on a
mode with a similar implementation to linum's, and got the same behavior.
This is not a linum bug.

On a side note, I would prefer nlinum's approach for my package, but if I'm
correct it would become slower over time, as it doesn't "GC" overlays. And
I don't think jit-lock provides an easy method to do it, as it marks
regions "fontified" (as opposed to "fontified-by-X-function").

Evgeni


On Wed, Apr 10, 2013 at 2:02 AM, E Sabof <esabof@gmail.com> wrote:

> nlinum works correctly.
>
> Evgeni
>
>
>
> On Wed, Apr 10, 2013 at 1:38 AM, Stefan Monnier <monnier@iro.umontreal.ca>wrote:
>
>> > Steps to reproduce:
>> > Open emacs -Q
>> > M-: (progn (linum-mode) (hs-mode))
>> > Insert a block of comments
>> > M-x hs-hide-block
>>
>> Could you test it with nlinum-mode (available from GNU ELPA)?
>>
>>
>>         Stefan
>>
>
>

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

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

Although I should be able to remove all overlays between a modification and
the end of buffer. This however still leaves the case where I simply scroll
through a large buffer, without making changes, and overlays accumulate.

Evgeni


On Thu, Apr 11, 2013 at 2:37 AM, E Sabof <esabof@gmail.com> wrote:

> The reason I've noticed/reported this bug was because I was working on a
> mode with a similar implementation to linum's, and got the same behavior.
> This is not a linum bug.
>
> On a side note, I would prefer nlinum's approach for my package, but if
> I'm correct it would become slower over time, as it doesn't "GC" overlays.
> And I don't think jit-lock provides an easy method to do it, as it marks
> regions "fontified" (as opposed to "fontified-by-X-function").
>
> Evgeni
>
>
> On Wed, Apr 10, 2013 at 2:02 AM, E Sabof <esabof@gmail.com> wrote:
>
>> nlinum works correctly.
>>
>> Evgeni
>>
>>
>>
>> On Wed, Apr 10, 2013 at 1:38 AM, Stefan Monnier <monnier@iro.umontreal.ca
>> > wrote:
>>
>>> > Steps to reproduce:
>>> > Open emacs -Q
>>> > M-: (progn (linum-mode) (hs-mode))
>>> > Insert a block of comments
>>> > M-x hs-hide-block
>>>
>>> Could you test it with nlinum-mode (available from GNU ELPA)?
>>>
>>>
>>>         Stefan
>>>
>>
>>
>

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

bug#14170: 24.3; linum won't create all overlays after a folding

[Eli Zaretskii]

> Date: Thu, 11 Apr 2013 02:37:55 +0100
> From: E Sabof <esabof@gmail.com>
> Cc: 14170@debbugs.gnu.org
> 
> The reason I've noticed/reported this bug was because I was working on a
> mode with a similar implementation to linum's, and got the same behavior.
> This is not a linum bug.

Can you show a simple test case to reproduce this bug without using
linum (or any other mode that is too large)?

A simple reproduction recipe is 50% of a solution.

Thanks.

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

Here it is:

(defun 14170-mini ()
  (interactive)
  (remove-overlays)
  (let* ((win-end-initial (window-end nil t))
         (test-ov (make-overlay (point) (+ (point) 1000))))
    (overlay-put test-ov 'display "...")
    (cl-assert (not (= win-end-initial (window-end nil t))))
    ))

Will fail most of the time.

Evgeni


On Thu, Apr 11, 2013 at 3:50 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Thu, 11 Apr 2013 02:37:55 +0100
> > From: E Sabof <esabof@gmail.com>
> > Cc: 14170@debbugs.gnu.org
> >
> > The reason I've noticed/reported this bug was because I was working on a
> > mode with a similar implementation to linum's, and got the same behavior.
> > This is not a linum bug.
>
> Can you show a simple test case to reproduce this bug without using
> linum (or any other mode that is too large)?
>
> A simple reproduction recipe is 50% of a solution.
>
> Thanks.
>

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

bug#14170: 24.3; linum won't create all overlays after a folding

[Eli Zaretskii]

> Date: Thu, 11 Apr 2013 04:23:13 +0100
> From: E Sabof <esabof@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, rgm@gnu.org, 14170@debbugs.gnu.org
> Here it is:
> 
> (defun 14170-mini ()
>   (interactive)
>   (remove-overlays)
>   (let* ((win-end-initial (window-end nil t))
>          (test-ov (make-overlay (point) (+ (point) 1000))))
>     (overlay-put test-ov 'display "...")
>     (cl-assert (not (= win-end-initial (window-end nil t))))
>     ))
> 
> Will fail most of the time.

Thanks.

Actually, I had a hard time making it fail consistently (after I
overcame the initial failure due to cl-assert not being available in
"emacs -Q" ;-), until I found a simple way to make it 100% repeatable:

  (defun 14170-mini ()
    (interactive)
    (remove-overlays)
    (sit-for 0)  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    (let* ((win-end-initial (window-end nil t))
	   (test-ov (make-overlay (point) (+ (point) 1000))))
      (overlay-put test-ov 'display "...")
      (cl-assert (not (= win-end-initial (window-end nil t))))

And that immediately led to the root cause: window-end was thinking
that the display is up to date, while it really wasn't.

Turns out this is a regression introduced in v24.1, while solving bug
#12600.  I think I fixed this (trunk revision 112268) without
reintroducing that bug.

I don't know if this solves the original problem with linum, but if it
doesn't, that's a different problem.

Thanks.

P.S.  Note that the above recipe still predictably fails at EOB, but
this is expected and correct.

bug#14170: 24.3; linum won't create all overlays after a folding

[Stefan Monnier]

> On a side note, I would prefer nlinum's approach for my package, but
> if I'm correct it would become slower over time, as it doesn't "GC"
> overlays.  And I don't think jit-lock provides an easy method to do
> it, as it marks regions "fontified" (as opposed to
> "fontified-by-X-function").

You might like to look at nlinum.el (the source code) and specifically
at nlinum--flush-overlays (which is commented out).

If you wonder why I commented it out, it's not because I couldn't make
it work, but because the slowdown it tries to avoid didn't seem to be
a problem in practice.


        Stefan

bug#14170: 24.3; linum won't create all overlays after a folding

[E Sabof]

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

On Sun, Apr 14, 2013 at 2:20 AM, Stefan Monnier <monnier@iro.umontreal.ca>wrote:

> > It crossed my mind, but I didn't like the idea, as it also removes
> > "good" fortification.  I suppose this could be of little consequence.
>
> I don't know what you mean by "removes good fontification".
>
>
>         Stefan
>

Actually you've said it in a comment:

(remove-overlays start end 'nlinum t)
;; Warn jit-lock that this part of the buffer is not done any
;; more.  This has the downside that font-lock will be re-applied
;; as well.  But jit-lock doesn't know how to (and doesn't want
;; to) keep track of the status of its various
;; clients independently.
(put-text-property start end 'fontified nil)

Evgeni

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

bug#14170: 24.3; linum won't create all overlays after a folding

[Stefan Monnier]

>> > It crossed my mind, but I didn't like the idea, as it also removes
>> > "good" fortification.  I suppose this could be of little consequence.
>> I don't know what you mean by "removes good fontification".
> Actually you've said it in a comment:
> (remove-overlays start end 'nlinum t)
> ;; Warn jit-lock that this part of the buffer is not done any
> ;; more.  This has the downside that font-lock will be re-applied
> ;; as well.  But jit-lock doesn't know how to (and doesn't want
> ;; to) keep track of the status of its various
> ;; clients independently.
> (put-text-property start end 'fontified nil)

Oh, that's what you meant by "remove".  Right, it flushes fontification
which did not need to be flushed, but that can be recomputed.
Of course, the overall impact on performance could be a concern,
although it's unclear how important that would be.  Based on the
experience that having many overlays isn't nearly as bad as expected,
the nlinum--ol-count limit could be pushed much higher than 100, thus
reducing the potential performance impact of "removing good
fontification" correspondingly.


        Stefan

bug#14170: 24.3; linum won't create all overlays after a folding

[Noam Postavsky]

Eli Zaretskii <eliz <at> gnu.org> wrote:
> Turns out this is a regression introduced in v24.1, while solving
> bug #12600.  I think I fixed this (trunk revision 112268) without
> reintroducing that bug.
>
> I don't know if this solves the original problem with linum, but if
> it doesn't, that's a different problem.

I confirmed that 14170-mini fails in 24.3 and works in 24.5.

(couldn't really make sense of the original scenario: seems to be
missing a (require 'hideshow), `hs-mode' should be `hs-minor-mode',
and I didn't see anything obviously wrong...)