Re: Possible `point-entered' `point-left' Text Property Bug

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

> Eval in emacs -Q:
>
> (defun prop-test (old new) (message "XXX: %d %d" old new))
> (let ((buffer (generate-new-buffer "*prop tst*")))
>   (with-current-buffer buffer
>     (insert "1234567890\n1234567890\n")
>     (put-text-property (point-min) (point-max) 'point-entered 'prop-test)
>     (put-text-property (point-min) (point-max) 'point-left 'prop-test)
>     (pop-to-buffer buffer)))
>
> Now move up two lines.  You'll see the `message' for every
> line you move up.  This doesn't happen if you move backward
> character wise with C-b.
>
> As far as i understand the documentation this shouldn't
> happen for the line movement either as all chars have the
> same `point-left' and `point-entered' text-property.

As far as I can tell, the problem arises when there is no character
before point.  From the Elisp manual,

     The same comparison [between point-entered and point-left]
     is made for the characters before the old and
     new locations.  The result may be to execute two `point-left'
     functions (which may be the same function) and/or two
     `point-entered' functions (which may be the same function).  In
     any case, all the `point-left' functions are called first,
     followed by all the `point-entered' functions.

When executing previous-line and next-line, a situation can arise
where no "character before point" is found for the point left, whereas
one exists for the point entered (or vice versa).  That's why the
point-entered/left hooks are called.  I don't know why this happens
even when moving vertically with point in the middle of a line, but I
am not familiar with the point motion code.

One fix is to ignore the hook when either the point left or the point
entered does not contain a "point before".  This patch does that.
Maybe we should also do the same thing for "point after".  (The patch
is a little longer than it should be because the existing code
contains a confusion between leave_after and leave_before, which has
to be corrected.)

What do people think?

*** emacs/src/intervals.c.~1.132.~	2006-05-09 10:08:04.000000000 -0400
--- emacs/src/intervals.c	2006-05-11 12:16:46.000000000 -0400
***************
*** 2196,2231 ****
        Lisp_Object leave_after, leave_before, enter_after, enter_before;
  
        if (fromprev)
! 	leave_after = textget (fromprev->plist, Qpoint_left);
        else
! 	leave_after = Qnil;
        if (from)
! 	leave_before = textget (from->plist, Qpoint_left);
        else
! 	leave_before = Qnil;
  
        if (toprev)
! 	enter_after = textget (toprev->plist, Qpoint_entered);
        else
! 	enter_after = Qnil;
        if (to)
! 	enter_before = textget (to->plist, Qpoint_entered);
        else
! 	enter_before = Qnil;
  
!       if (! EQ (leave_before, enter_before) && !NILP (leave_before))
! 	call2 (leave_before, make_number (old_position),
! 	       make_number (charpos));
        if (! EQ (leave_after, enter_after) && !NILP (leave_after))
! 	call2 (leave_after, make_number (old_position),
! 	       make_number (charpos));
  
!       if (! EQ (enter_before, leave_before) && !NILP (enter_before))
! 	call2 (enter_before, make_number (old_position),
! 	       make_number (charpos));
        if (! EQ (enter_after, leave_after) && !NILP (enter_after))
! 	call2 (enter_after, make_number (old_position),
! 	       make_number (charpos));
      }
  }
  \f
--- 2196,2235 ----
        Lisp_Object leave_after, leave_before, enter_after, enter_before;
  
        if (fromprev)
! 	leave_before = textget (fromprev->plist, Qpoint_left);
        else
! 	leave_before = Qnil;
! 
        if (from)
! 	leave_after = textget (from->plist, Qpoint_left);
        else
! 	leave_after = Qnil;
  
        if (toprev)
! 	enter_before = textget (toprev->plist, Qpoint_entered);
        else
! 	enter_before = Qnil;
! 
        if (to)
! 	enter_after = textget (to->plist, Qpoint_entered);
        else
! 	enter_after = Qnil;
  
!       if (fromprev && toprev
! 	  && ! EQ (leave_before, enter_before) && !NILP (leave_before))
!       	call2 (leave_before, make_number (old_position),
!       	       make_number (charpos));
        if (! EQ (leave_after, enter_after) && !NILP (leave_after))
!       	call2 (leave_after, make_number (old_position),
!       	       make_number (charpos));
  
!       if (fromprev && toprev
! 	  && ! EQ (enter_before, leave_before) && !NILP (enter_before))
!       	call2 (enter_before, make_number (old_position),
!       	       make_number (charpos));
        if (! EQ (enter_after, leave_after) && !NILP (enter_after))
!       	call2 (enter_after, make_number (old_position),
!       	       make_number (charpos));
      }
  }
  \f

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

    When executing previous-line and next-line, a situation can arise
    where no "character before point" is found for the point left, whereas
    one exists for the point entered (or vice versa).  That's why the
    point-entered/left hooks are called.

That seems like correct behavior to me.  At least, more correct than
the alternative.  If the entered/left functions are called in this
case, they can detect this case and decide to do nothing.  But if we
change the code NOT to call them in this case, and some program wants
them to be called, there is nothing it can do.

Does this behavior really cause a problem that is hard to work around?
Or did it just strike people as incorrect?

					  I don't know why this happens
    even when moving vertically with point in the middle of a line, but I
    am not familiar with the point motion code.

I am really surprised about this because line-move-1 binds
inhibit-point-motion-hooks to t, which ought to prevent the
point-entered/left functions from even being checked for all the
intermediate positions.

The easy way to determine why they get called is to put breakpoints
on the call2 lines:

      if (! EQ (leave_before, enter_before) && !NILP (leave_before))
	call2 (leave_before, make_number (old_position),
	       make_number (charpos));
      if (! EQ (leave_after, enter_after) && !NILP (leave_after))
	call2 (leave_after, make_number (old_position),
	       make_number (charpos));

      if (! EQ (enter_before, leave_before) && !NILP (enter_before))
	call2 (enter_before, make_number (old_position),
	       make_number (charpos));
      if (! EQ (enter_after, leave_after) && !NILP (enter_after))
	call2 (enter_after, make_number (old_position),
	       make_number (charpos));

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

>     When executing previous-line and next-line, a situation can arise
>     where no "character before point" is found for the point left, whereas
>     one exists for the point entered (or vice versa).  That's why the
>     point-entered/left hooks are called.
>
> That seems like correct behavior to me.  At least, more correct than
> the alternative.  If the entered/left functions are called in this
> case, they can detect this case and decide to do nothing.  But if we
> change the code NOT to call them in this case, and some program wants
> them to be called, there is nothing it can do.
>
> Does this behavior really cause a problem that is hard to work around?
> Or did it just strike people as incorrect?

It struck the bug reporter as incorrect.  I don't know any real-life
problem that it causes.

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

> 					  I don't know why this happens
>     even when moving vertically with point in the middle of a line, but I
>     am not familiar with the point motion code.
>
> I am really surprised about this because line-move-1 binds
> inhibit-point-motion-hooks to t, which ought to prevent the
> point-entered/left functions from even being checked for all the
> intermediate positions.

I investigated some more, and found two separate bugs.  The first bug
is that inhibit-point-motion-hooks was not being set to t during
Fline_beginning_position.

The second bug is more subtle.  In simple.el line 3652, we have

	;; Process intangibility within a line.
	;; Move to the chosen destination position from above,
	;; with intangibility processing enabled.

	(goto-char (point-min))
	(let ((inhibit-point-motion-hooks nil))
	  (goto-char new)

Here, inhibit-point-motion-hooks is bound to nil to avoid processing
the intangible property.  But this also has the effect of spuriously
calling point-left and point-entered hooks if they are defined.  I
hacked around this by changing it to

	(goto-char new)
	(let ((inhibit-point-motion-hooks nil))
	  (goto-char new)

I'm not 100% certain this will work or how to test it, so someone
familiar with the tangibility/point motion code should take a look.

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

> I investigated some more, and found two separate bugs.  The first bug
> is that inhibit-point-motion-hooks was not being set to t during
> Fline_beginning_position.
>
> The second bug is more subtle.
> Here, inhibit-point-motion-hooks is bound to nil to avoid processing
> the intangible property.  But this also has the effect of spuriously
> calling point-left and point-entered hooks if they are defined.

Another possibility is to split inhibit-point-motion-hooks into two
separate variables that suppress point-entered/point-left and use of
the intangible property respectively.  In that case, both
Fline_beginning_position and the relevant part of line-move-1 would
inhibit use of point-entered/point-left while allowing use of the
tangible property.

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

    > That seems like correct behavior to me.  At least, more correct than
    > the alternative.  If the entered/left functions are called in this
    > case, they can detect this case and decide to do nothing.  But if we
    > change the code NOT to call them in this case, and some program wants
    > them to be called, there is nothing it can do.
    >
    > Does this behavior really cause a problem that is hard to work around?
    > Or did it just strike people as incorrect?

    It struck the bug reporter as incorrect.  I don't know any real-life
    problem that it causes.

Ok, let's not worry about it.

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

	    ;; Process intangibility within a line.
	    ;; Move to the chosen destination position from above,
	    ;; with intangibility processing enabled.

	    (goto-char (point-min))
	    (let ((inhibit-point-motion-hooks nil))
	      (goto-char new)

    Here, inhibit-point-motion-hooks is bound to nil to avoid processing
    the intangible property.  But this also has the effect of spuriously
    calling point-left and point-entered hooks if they are defined.  I
    hacked around this by changing it to

	    (goto-char new)
	    (let ((inhibit-point-motion-hooks nil))
	      (goto-char new)

I think that change is incorrect.  That code is supposed
to run the hooks in a controlled way.  Your change makes it
a no-op.

Why do you believe the old code is wrong?

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

    Another possibility is to split inhibit-point-motion-hooks into two
    separate variables that suppress point-entered/point-left and use of
    the intangible property respectively.

We can do that if necessary.  But is it necessary?

What exactly is the bug here?  What does the current code do that is
wrong?

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> 	    ;; Process intangibility within a line.
> 	    ;; Move to the chosen destination position from above,
> 	    ;; with intangibility processing enabled.
>
> 	    (goto-char (point-min))
> 	    (let ((inhibit-point-motion-hooks nil))
> 	      (goto-char new)
>
>     Here, inhibit-point-motion-hooks is bound to nil to avoid processing
>     the intangible property.  But this also has the effect of spuriously
>     calling point-left and point-entered hooks if they are defined.  I
>     hacked around this by changing it to
>
> 	    (goto-char new)
> 	    (let ((inhibit-point-motion-hooks nil))
> 	      (goto-char new)
>
> I think that change is incorrect.  That code is supposed
> to run the hooks in a controlled way.  Your change makes it
> a no-op.
>
> Why do you believe the old code is wrong?

Because of the bug report that started all this: suppose the
properties `point-entered' and `point-left' are defined over the
entire interval (point-min) to (point-max), and point is in the middle
of a line, as shown:

 1 2 3|4 5
 1 2 3 4 5

Now, suppose we call `next-line'.  The part of `line-move-finish' that
processes intangibility, shown above, first moves point from its
original position to point-min, rebinds inhibit-point-motion-hooks to
nil, then moves to the desired point:

|1 2 3 4 5
 1 2 3 4 5

 1 2 3 4 5
 1 2 3|4 5

Since there is no point before point-min, and
inhibit-point-motion-hooks is nil during the second point motion, the
point-entered/left hooks will be called.  This is wrong, since the
initial and final points of the `next-line' command have identical
`point-entered' and `point-left' properties.

See `line-move-finish' for details.

Of course, if you decide that it's OK for the point motion hooks to be
spuriously called in some situations, that's fine by me; I'll revert
the changes and add a note to the Elisp manual to that effect.

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

    Now, suppose we call `next-line'.  The part of `line-move-finish' that
    processes intangibility, shown above, first moves point from its
    original position to point-min, rebinds inhibit-point-motion-hooks to
    nil, then moves to the desired point:

Hmm, I agree that scanning from point-min is somewhat odd.
Maybe it would be better to use (line-beginning-position)
instead of (point-min) as the starting point.  What do you think?
Does that make it work?

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

>     Now, suppose we call `next-line'.  The part of `line-move-finish' that
>     processes intangibility, shown above, first moves point from its
>     original position to point-min, rebinds inhibit-point-motion-hooks to
>     nil, then moves to the desired point:
>
> Hmm, I agree that scanning from point-min is somewhat odd.
> Maybe it would be better to use (line-beginning-position)
> instead of (point-min) as the starting point.  What do you think?
> Does that make it work?

Not always.  For example, when point-left and point-entered are placed
over an interval that does not cover the line-beginning position, as
shown:

    | interval  |
 1 2 3 4_5 6 7 8 9
        ^ point is here

Re: Possible `point-entered' `point-left' Text Property Bug

[Richard Stallman]

    > Hmm, I agree that scanning from point-min is somewhat odd.
    > Maybe it would be better to use (line-beginning-position)
    > instead of (point-min) as the starting point.  What do you think?
    > Does that make it work?

    Not always.  For example, when point-left and point-entered are placed
    over an interval that does not cover the line-beginning position, as
    shown:

	| interval  |
     1 2 3 4_5 6 7 8 9
	    ^ point is here

The scenario is not fully specified, so I am not sure what the code
will do, nor can I start to think about whether it is right or wrong.

Could you try making a completely specified scenario, and show
what the code will do?

Re: Possible `point-entered' `point-left' Text Property Bug

[Chong Yidong]

>     > Hmm, I agree that scanning from point-min is somewhat odd.
>     > Maybe it would be better to use (line-beginning-position)
>     > instead of (point-min) as the starting point.  What do you think?
>     > Does that make it work?
>
>     Not always.  For example, when point-left and point-entered are placed
>     over an interval that does not cover the line-beginning position, as
>     shown:
>
> 	| interval  |
>      1 2 3 4_5 6 7 8 9
> 	    ^ point is here
>
> The scenario is not fully specified, so I am not sure what the code
> will do, nor can I start to think about whether it is right or wrong.
>
> Could you try making a completely specified scenario, and show
> what the code will do?

(defun prop-test (old new) (message "XXX: %d %d" old new))
(let ((buffer (generate-new-buffer "*prop tst*")))
  (with-current-buffer buffer
    (insert "12345\n12345\n12345")
    (goto-char 16)
    (put-text-property 8 (point-max) 'point-entered 'prop-test)
    (put-text-property 8 (point-max) 'point-left 'prop-test)
    (pop-to-buffer buffer)))

This brings up a buffer with contents (ignore the spaces, which are
there for illustrative purposes):

 1 2 3 4 5
 1*2 3 4 5
 1 2 3|4 5*

where the region between the asterisks has non-nil `point-entered' and
`point-left' properties, and point is placed at the position indicated
by `|'.

Now do C-p.  The `point-entered' hook, `prop-test', is called.  This
is wrong, since point is moving inside the region.

That's because in the middle of line-move-finish, when handling
intangibility, we bind inhibit-point-motion-hooks to nil and move
point to (point-min):

|1 2 3 4 5
 1*2 3 4 5
 1 2 3 4 5*

(or, as you suggested, line-beginning-position):

 1 2 3 4 5
|1*2 3 4 5
 1 2 3 4 5*

and then to the target position:

 1 2 3 4 5
 1*2 3|4 5
 1 2 3 4 5*

Either way, point leaves the region where point-entered/left is
non-nil during this operation, so the point motion hooks are called.

If you feel that fixing this is too much trouble before the release,
how about simply adding a note to the documentation saying that line
motion can call the point-entered/point-left hooks in unpredictable
ways?  I don't know any real-life code that breaks because of this, so
maybe we shouldn't risk introducing bugs with tangibility by changing
stuff now.