bug in forward-visible-line: Patch

bug in forward-visible-line: Patch

[Luc Teirlinck]

Start out with the following buffer:

line 1;line 2
line 3;line 4
line 5

Where, after each ";" there is an invisible newline.  With point at
the beginning of the buffer, do M-: (forward-visible-line arg) for
various args. 1 carries us to the beginning of line 3 and 2 to the
beginning of line 5 (as I expected).  Now put point at the end of line
4.  There 0 carries us to the beginning of line 4, -1 to the beginning
of line 3, -2 to the beginning of line 2 and -3 carries us also to the
beginning of line 2.

Makes no sense.  The following patch seems to fix the problem:

Change log:

2003-05-21  Luc Teirlinck  <teirllm@mail.auburn.edu>

        * simple.el (forward-visible-line): Fix negative arguments.

Patch:

===File ~/simplediff========================================
cd /usr/local/share/emacs/21.3.50/lisp/
diff -c /usr/local/share/emacs/21.3.50/lisp/simple.old.el /usr/local/share/emacs/21.3.50/lisp/simple.el
*** /usr/local/share/emacs/21.3.50/lisp/simple.old.el	Wed May 21 18:33:45 2003
--- /usr/local/share/emacs/21.3.50/lisp/simple.el	Wed May 21 23:13:01 2003
***************
*** 2265,2272 ****
  	      (unless (bolp)
  		(goto-char opoint))))
  	(let ((first t))
! 	  (while (or first (< arg 0))
! 	    (if (zerop arg)
  		(beginning-of-line)
  	      (or (zerop (forward-line -1))
  		  (signal 'beginning-of-buffer nil)))
--- 2265,2272 ----
  	      (unless (bolp)
  		(goto-char opoint))))
  	(let ((first t))
! 	  (while (or first (<= arg 0))
! 	    (if first
  		(beginning-of-line)
  	      (or (zerop (forward-line -1))
  		  (signal 'beginning-of-buffer nil)))
***************
*** 2275,2287 ****
  	    (unless (bobp)
  	      (let ((prop
  		     (get-char-property (1- (point)) 'invisible)))
! 		(if (if (eq buffer-invisibility-spec t)
! 			prop
! 		      (or (memq prop buffer-invisibility-spec)
! 			  (assq prop buffer-invisibility-spec)))
! 		    (setq arg (1+ arg)))))
! 	    (setq first nil)
! 	    (setq arg (1+ arg)))
  	  ;; If invisible text follows, and it is a number of complete lines,
  	  ;; skip it.
  	  (let ((opoint (point)))
--- 2275,2286 ----
  	    (unless (bobp)
  	      (let ((prop
  		     (get-char-property (1- (point)) 'invisible)))
! 		(unless (if (eq buffer-invisibility-spec t)
! 			    prop
! 			  (or (memq prop buffer-invisibility-spec)
! 			      (assq prop buffer-invisibility-spec)))
! 		  (setq arg (1+ arg)))))
! 	    (setq first nil))
  	  ;; If invisible text follows, and it is a number of complete lines,
  	  ;; skip it.
  	  (let ((opoint (point)))

Diff finished at Wed May 21 23:13:27
============================================================

Re: bug in forward-visible-line: Patch

[Stefan Monnier]

> ! 		(if (if (eq buffer-invisibility-spec t)
> ! 			prop
> ! 		      (or (memq prop buffer-invisibility-spec)
> ! 			  (assq prop buffer-invisibility-spec)))

BTW, how about a `invisible-p' function that does above ?
Especially since the above is wrong (the assq might match a cons
cell like (foo . nil) which says that it's *not* invisible).

Also while I'm talking about the invisible property.
A lot of code uses t as an invisible value that is assumed to
always be invisible and nil for the opposite.  A recent change
makes buffer-invisibility-spec always contain t so that t should
indeed always be invisible, but I think a better change is to make
the invisibility check something like

	(cond
	 ((eq buffer-invisibility-spec t) val)
	 ((null val) 'visible)
	 ((eq val t) 'invisible)
	 (t (or (memq prop buffer-invisibility-spec)
	        (cdr (assq prop buffer-invisibility-spec)))))

where the change is that nil and t are special values that do
not depend on buffer-invisibility-spec.


	Stefan

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

Stefan Monnier wrote:

   BTW, how about a `invisible-p' function that does above ?

Might be useful.  A correct version of the above, of course.  I
personally also see something wrong with it, but not the same thing
you seem to see.

   Especially since the above is wrong (the assq might match a cons
   cell like (foo . nil) which says that it's *not* invisible).

I do not believe that is what is wrong with it.

Documentation string of buffer-invisibility-spec:

    buffer-invisibility-spec's value is 
    ((silly)
     nonsense)

    Local in buffer killerstuff; global value is t
    Automatically becomes buffer-local when set in any fashion.

    Invisibility spec of this buffer.  The default is t, which means
    that text is invisible if it has a non-nil `invisible' property.
    If the value is a list, a text character is invisible if its
    `invisible' property is an element in that list.  If an element is
    a cons cell of the form (PROP . ELLIPSIS), then characters with
    property value PROP are invisible, and they have an ellipsis as
    well if ELLIPSIS is non-nil.


Note that the documentation string says that (foo . nil) means
that the character will get no ellipsis but will still be invisible.
I tried it out, it is true. (silly) above is equivalent with (silly
. nil) and silly newlines were invisible.

The documentation string above is wrong about something else, however.
Newlines with invisibility property '(aha ihi oho silly mia) are
invisible too, even though according to the documentation string they
should not be.  The documentation string should be corrected.

The (in as far as I know) correct description of buffer-invisibility-spec
can be found in C-h i g (elisp)Invisible Text.  forward-visible-line
fails to recognize newlines with invisible property '(aha ihi oho
silly mia) as invisible, even though they are.  That is what I believe
is wrong with the above code.

   A lot of code uses t as an invisible value that is assumed to
   always be invisible and nil for the opposite.  A recent change
   makes buffer-invisibility-spec always contain t so that t should
   indeed always be invisible,

What exactly do you mean?  After I reset buffer-invisibility-spec
without any problems to the above value, newlines with invisibility
property t became visible.  As long as that is the actual behavior,
forward-visible-line should treat them as such.  

   where the change is that nil and t are special values that do
   not depend on buffer-invisibility-spec.

In as far as t is concerned, only if the actual behavior would change,
not with the present behavior.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

Stefan Monnier wrote:

   BTW, how about a `invisible-p' function that does above ?

I believe visiblep would be slightly more useful (does not make any
real difference though).  I believe the usual convention is to only
use -p if the function name is already multi-word, although this
convention is not universally followed.

What about the following function?  I should still double-check it
more carefully, but on first testing in a reasonably complex
situation, it seems to work OK.  I believe that I could use the
function not only to make forward-visible-line behave correctly (even
after my patch, a problem remains, as I pointed out earlier), but
probably in other situations as well.

===File ~/invp.el===========================================
(defun visiblep (&optional pos)
  "Return t if character at POS is currently visible.
POS defaults to point."
  (unless pos (setq pos (point)))
  (let ((prop (get-text-property pos 'invisible)))
    (cond
     ((null prop))
     ((eq buffer-invisibility-spec t) nil)
     ((memq prop buffer-invisibility-spec) nil)
     ((assq prop buffer-invisibility-spec) nil)
     ((listp prop)
      (catch 'found
	(dolist (var prop)
	  (if (or (memq var buffer-invisibility-spec)
		  (assq var buffer-invisibility-spec))
	      (throw 'found nil)))))
     (t))))
============================================================

Re: bug in forward-visible-line: Patch

[Stefan Monnier]

>    BTW, how about a `invisible-p' function that does above ?
> 
> I believe visiblep would be slightly more useful (does not make any
> real difference though).  I believe the usual convention is to only
> use -p if the function name is already multi-word, although this
> convention is not universally followed.
> 
> What about the following function?  I should still double-check it
> more carefully, but on first testing in a reasonably complex
> situation, it seems to work OK.  I believe that I could use the
> function not only to make forward-visible-line behave correctly (even
> after my patch, a problem remains, as I pointed out earlier), but
> probably in other situations as well.
> 
> ===File ~/invp.el===========================================
> (defun visiblep (&optional pos)
>   "Return t if character at POS is currently visible.
> POS defaults to point."
>   (unless pos (setq pos (point)))
>   (let ((prop (get-text-property pos 'invisible)))
>     (cond
>      ((null prop))
>      ((eq buffer-invisibility-spec t) nil)
>      ((memq prop buffer-invisibility-spec) nil)
>      ((assq prop buffer-invisibility-spec) nil)
>      ((listp prop)
>       (catch 'found
> 	(dolist (var prop)
> 	  (if (or (memq var buffer-invisibility-spec)
> 		  (assq var buffer-invisibility-spec))
> 	      (throw 'found nil)))))
>      (t))))

Don't reinvent the wheel: implement it in C where it's
already available (and so you're sure it really works the
same).  The C version return 0 1 or 2 so you can also tell
whether it's got an ellipsis or not.


	Stefan

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

Stefan Monnier wrote:

   Don't reinvent the wheel: implement it in C where it's
   already available (and so you're sure it really works the
   same).  The C version return 0 1 or 2 so you can also tell
   whether it's got an ellipsis or not.

This is somewhat ambiguous.  I believe you do not mean "implement it in
C" but "it is already implemented in C".  In the latter case I guess it
would be easy to make it callable from Lisp, which would be useful.

I might have misunderstood your previous mail.  Was your cond form
intended as a blueprint for a Lisp function (which would also have
reinvented the wheel) or as a suggested change in actual behavior?

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

>From my previous message:

   In the latter case I guess it would be easy to make it callable
   from Lisp, which would be useful.

In particular, it should be used in forward-visible-line, which uses
four times an erroneous form for determining visibility and in
end-of-visible-line which does so once.  (In particular, the current
version of end-of-visible-line has a bug too, which should be fixed
and would be trivial to fix with the new function.)  There could be
other places, but these are the ones I know of.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

I did not notice this before, but there is already a Lisp function that
tries to implement the functionality we have been discussing.  It is
incorrect, however:

(defun line-move-invisible (pos)
  "Return non-nil if the character after POS is currently invisible."
  (let ((prop
   (get-char-property pos 'invisible)))
    (if (eq buffer-invisibility-spec t)
    prop
      (or (memq prop buffer-invisibility-spec)
        (assq prop buffer-invisibility-spec)))))

One needs a dolist of the type I used if one wants to implement this
correctly in Lisp.  But as you suggested, it might be better to use
the C implementation.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

This would be a corrected version of line-move-invisible:

(defun line-move-invisible (pos)
  "Return non-nil if the character after POS is currently invisible."
  (let ((prop (get-char-property pos 'invisible)))
    (cond
     ((null prop) nil)
     ((eq buffer-invisibility-spec t))
     ((memq prop buffer-invisibility-spec))
     ((assq prop buffer-invisibility-spec))
     ((listp prop)
      (catch 'found
        (dolist (var prop)
          (if (or (memq var buffer-invisibility-spec)
                  (assq var buffer-invisibility-spec))
              (throw 'found t)))))
     (t nil))))

I could double-check this carefully (I did not) and send a diff, but
as Stefan pointed out, it might be better to implement this function
in C.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[David Kastrup]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> What about the following function?  I should still double-check it
> more carefully, but on first testing in a reasonably complex
> situation, it seems to work OK.

If (listp prop) evaluates to t, nil is always returned.

> ===File ~/invp.el===========================================

invp.el for a function visiblep?

> (defun visiblep (&optional pos)
>   "Return t if character at POS is currently visible.
> POS defaults to point."
>   (unless pos (setq pos (point)))
>   (let ((prop (get-text-property pos 'invisible)))
>     (cond
>      ((null prop))
>      ((eq buffer-invisibility-spec t) nil)
>      ((memq prop buffer-invisibility-spec) nil)
>      ((assq prop buffer-invisibility-spec) nil)
>      ((listp prop)
>       (catch 'found
> 	(dolist (var prop)
> 	  (if (or (memq var buffer-invisibility-spec)
> 		  (assq var buffer-invisibility-spec))
> 	      (throw 'found nil)))))
>      (t))))

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: bug in forward-visible-line: Patch

[Miles Bader]

BTW, one problem with the name `visiblep' is that it could be confused
to mean the same thing as `pos-visible-in-window-p'.  OTOH, I don't
think I don't think `invisiblep' has this problem, since the term of
`invisible' in emacs is strongly associated with the invisible property.

-miles
-- 
Run away!  Run away!

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

Miles Bader wrote:

   BTW, one problem with the name `visiblep' is that it could be confused
   to mean the same thing as `pos-visible-in-window-p'.  OTOH, I don't
   think I don't think `invisiblep' has this problem, since the term of
   `invisible' in emacs is strongly associated with the invisible property.

As I already pointed out in my reply to David Kastrup, I failed to
notice at the time when I proposed this function that there already
was a function, `line-move-invisible', with essentially the same
functionality as invisiblep.  This function currently works fine in
the absence of invisibility properties that are lists with two or more
elements, but malfunctions in the presence of such invisibility
properties.  This is a bug that should be fixed and can easily be
fixed by either using the new version of `line-move-invisible' which I
proposed in a later posting or by using the C functionality and making
`line-move-invisible' a primitive, as Stefan suggested.  The advantage
of the latter solution would be that if `line-move-invisible' used the
same function as display to determine visibility (Stefan seems to
suggest that this would be easy to implement), then it would actually
be "theoretically impossible" for `line-move-invisible' to get it
wrong.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Richard Stallman]

    As I already pointed out in my reply to David Kastrup, I failed to
    notice at the time when I proposed this function that there already
    was a function, `line-move-invisible', with essentially the same
    functionality as invisiblep.  This function currently works fine in
    the absence of invisibility properties that are lists with two or more
    elements, but malfunctions in the presence of such invisibility
    properties.

I still think we should make this a C function.  It may need to be
called often.

Re: bug in forward-visible-line: Patch

[Richard Stallman]

    BTW, one problem with the name `visiblep' is that it could be confused
    to mean the same thing as `pos-visible-in-window-p'.  OTOH, I don't
    think I don't think `invisiblep' has this problem, since the term of
    `invisible' in emacs is strongly associated with the invisible property.

I agree.  In addition, text is normally visible, and most text is
visible, so invisibility is an unusual situation.  That is another reason
why invisiblep is a better function to install.

I think this function is worth writing in C for speed.
Also, parts of it already exist in C, ISTR.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

David Kastrup wrote:

   If (listp prop) evaluates to t, nil is always returned.

Yes, this was a typo which I fixed in a later posting. It should have
said: (dolist (var prop t)

Also, the use of get-text-property was a bug.  I should have used
get-char-property or overlays would not be handled correctly.
All of these are moot points now, since my main oversight at the time
was not to notice that there already was a function with a similar
functionality: line-move-invisible.  (I noticed it later.)

That function has problems that should be corrected as I pointed out,
but I believe that doing so would eliminate the need for a new
function.

Sincerely,

Luc.

Re: bug in forward-visible-line: Patch

[Luc Teirlinck]

There was a typo in my function. New version:

(defun visiblep (&optional pos)
  "Return t if character at POS is currently visible.
POS defaults to point."
  (unless pos (setq pos (point)))
  (let ((prop (get-text-property pos 'invisible)))
    (cond
     ((null prop))
     ((eq buffer-invisibility-spec t) nil)
     ((memq prop buffer-invisibility-spec) nil)
     ((assq prop buffer-invisibility-spec) nil)
     ((listp prop)
      (catch 'found
        (dolist (var prop t)
          (if (or (memq var buffer-invisibility-spec)
                  (assq var buffer-invisibility-spec))
              (throw 'found nil)))))
     (t))))