bug#11094: Wrong cursor positioning with display+invisible

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

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

Package: Emacs
Version: 24.0.94

Extract the test.cpio and cpio-mode.el files attached, and then try:

   src/emacs -Q -l .../cpio-mode.el .../test.cpio

First, you'll note that the cursor is displayed at end-of-line (whereas
in reality point as it BOB), then move forward with C-f and you'll see
the cursor jumping in odd ways (often being pushed to end-of-line even
when we're still in the middle of the line).

In Emacs-23, the cursor jumps correctly over the various display and
invisible fields, so this is a regression.


        Stefan

   


In GNU Emacs 24.0.94.1 (i386-unknown-linux-gnu, GTK+ Version 2.24.10)
 of 2012-03-21 on pastel
Windowing system distributor `The X.Org Foundation', version 11.0.11104000
Configured using:
 `configure
 'CFLAGS=-Wall -Wno-pointer-sign -DUSE_LISP_UNION_TYPE -DSYNC_INPUT -DENABLE_CHECKING -DXASSERTS -DFONTSET_DEBUG -g -O0'
 '--with-tiff=no''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: fr_CH.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t


[-- Attachment #2: test.cpio --]
[-- Type: application/x-cpio, Size: 232 bytes --]

[-- Attachment #3: cpio-mode.el --]
[-- Type: application/emacs-lisp, Size: 6336 bytes --]

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

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

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 26 Mar 2012 00:07:32 -0400
> 
> Extract the test.cpio and cpio-mode.el files attached, and then try:
> 
>    src/emacs -Q -l .../cpio-mode.el .../test.cpio

Ouch!

(Btw, test.cpio is an invalid archive, according to 2 different
implementations of the cpio command.  The one from GNU/Linux says
"cpio: premature end of file".  Not that it matters for the purposes
of this discussion.)

> First, you'll note that the cursor is displayed at end-of-line (whereas
> in reality point as it BOB), then move forward with C-f and you'll see
> the cursor jumping in odd ways (often being pushed to end-of-line even
> when we're still in the middle of the line).

Right.  The end-of-line placement of the cursor usually means that the
display engine knows the cursor should be on this line, but it didn't
find a proper place to put it.  And in this case, for a good reason,
see below.

> In Emacs-23, the cursor jumps correctly over the various display and
> invisible fields

For some value of "correctly".  E.g., position the cursor over the
".", which is the first file name in the archive, and type "C-x =".
You will see "63", which is a lie: the actual value of point is 111.
This lie becomes more evident if you position the cursor on the first
's' of "scripts", the file name on the second line: "C-x =" says "175"
(whereas the truth is 223), but if you now type "C-f C-x =", you will
see "224".  Huh?

Anyway, that this "works" in Emacs 23 and before is more by accident
than by anything else.  Your cpio-mode.el almost completely conceals
the buffer contents from the display engine, either by 'invisible'
property or behind display strings.  The cursor-positioning code is
therefore completely helpless when it comes to decide where to put the
cursor, because all it has is (a) the value of point, and (b) the
glyphs generated for each display line (a.k.a. "glyph row").  With
most of the buffer positions covered by 'invisible' and 'display'
properties, no glyphs to place the cursor on can be found for most
values of point, since there are no glyphs which "tell" the display
engine that they came from those buffer positions.

The old, unidirectional display engine could get away (albeit by the
skin of its teeth) with such code because it relied on buffer
positions to increase monotonically with screen positions.  So once it
found a glyph from buffer position N that is greater or equal to the
value of point, it could place the cursor there, because it "knew" all
the later glyphs _must_ correspond to positions beyond N.  That is why
it "works" in Emacs 23.

A case in point is the position of cursor at BOB.  The value of point
is 1, whereas the first glyph on the first screen lines comes from
buffer position 15(!).  The old display code could put the cursor
there _only_ because that is the first glyph whose buffer position is
greater or equal to 1.

However, the bidirectional display engine can no longer make such
assumptions, so it is helpless without at least _some_ clue about the
buffer positions corresponding to the glyphs.  And your code gives
almost no such clues.

I think we should either (1) from now on discourage (i.e., maintain
that we don't support) code that covers too much of buffer text by
display strings, or (2) completely redesign 'struct glyph', so that it
keeps more information about the buffer positions "associated" with
the glyph, and then refactor the code which needs that information.
(The second alternative is, of course, not for Emacs 24.)

Alternatively, may I suggest that you use the 'cursor' property to
provide some rope for the display engine to do its job even in this
situation?  The attached variant of your code demonstrates how to do
that; it works equally well with Emacs 23 and with the current trunk.

Bottom line, to resolve this bug for Emacs 24.1, I suggest a judicial
use of the 'cursor' property as the only safe (and IMO the only sane)
alternative.  If you still think we should support your original code,
we should schedule some post-24.1 redesign and refactoring.  Let me
know what you think.

Here's a modified version of cpio-mode.el.  Note that it removes most
of the 'invisible' properties, because they are not really needed, and
because they have an unpleasant side effect of reporting incorrect
value of point due to "point adjustments".  I suspect that all the
other 'invisible' properties should be removed as well, but I didn't
test the situations where they would come into play.


[-- Attachment #2: cpio-mode1.el --]
[-- Type: application/emacs-lisp, Size: 6672 bytes --]

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

>> Extract the test.cpio and cpio-mode.el files attached, and then try:
>> src/emacs -Q -l .../cpio-mode.el .../test.cpio
> Ouch!

;-)

> (Btw, test.cpio is an invalid archive, according to 2 different
> implementations of the cpio command.  The one from GNU/Linux says
> "cpio: premature end of file".  Not that it matters for the purposes
> of this discussion.)

I just truncated it, since the original (an initrd) is 10MB long.

>> In Emacs-23, the cursor jumps correctly over the various display and
>> invisible fields
> For some value of "correctly".  E.g., position the cursor over the
> ".", which is the first file name in the archive, and type "C-x =".
> You will see "63", which is a lie: the actual value of point is 111.

I don't think it's a lie because that text is preceded by invisible
text, so while "." is at 111, point is indeed at 63 (and that's just
because of adjust_point_for_property, so we can get point to stay at
111 by being more careful with text property's stickiness).

> The old, unidirectional display engine could get away (albeit by the
> skin of its teeth) with such code because it relied on buffer
> positions to increase monotonically with screen positions.  So once it
> found a glyph from buffer position N that is greater or equal to the
> value of point, it could place the cursor there, because it "knew" all
> the later glyphs _must_ correspond to positions beyond N.  That is why
> it "works" in Emacs 23.

I see that the problem is not so much that all the text is covered by
those properties, but rather that there is are contiguous texts with
`invisible' and `display' properties.  I get Emacs-24 to behave
correctly by turning all "invisible span followed by display span" into
just a single display span.

E.g. if you do

  emacs -Q
  (put-text-property (point-min) (+ 2 (point-min)) 'invisible t)
  (put-text-property (+ 2 (point-min)) (+ 4 (point-min)) 'display "<>")
  (goto-char (point-min))

you'll see that the cursor is drawn at the wrong place (i.e. after the
<>), whereas if you do
  
  emacs -Q
  (put-text-property (point-min) (+ 4 (point-min)) 'display "<>")
  (goto-char (point-min))

the cursor is drawn at the right place.  I think this is the core of the
problem that's handled differently from Emacs-23.
[ IIUC you've gotten to the same conclusion.  ]

> If you still think we should support your original code, we should
> schedule some post-24.1 redesign and refactoring.  Let me know what
> you think.

I don't think it's a very high priority problem, but it would be good to
try and tackle it, yes (post-24.1).


        Stefan

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 11094@debbugs.gnu.org
> Date: Sun, 01 Apr 2012 21:06:59 -0400
> 
> > For some value of "correctly".  E.g., position the cursor over the
> > ".", which is the first file name in the archive, and type "C-x =".
> > You will see "63", which is a lie: the actual value of point is 111.
> 
> I don't think it's a lie because that text is preceded by invisible
> text, so while "." is at 111, point is indeed at 63 (and that's just
> because of adjust_point_for_property, so we can get point to stay at
> 111 by being more careful with text property's stickiness).

Having point report X when it is position on a character whose
position is Y, or jump when you move it from one character of a file
name to the next violates the principle of least astonishment, IMO.

> I see that the problem is not so much that all the text is covered by
> those properties, but rather that there is are contiguous texts with
> `invisible' and `display' properties.

Yes, that doesn't help, as I mentioned.

>   emacs -Q
>   (put-text-property (point-min) (+ 4 (point-min)) 'display "<>")
>   (goto-char (point-min))
> 
> the cursor is drawn at the right place.  I think this is the core of the
> problem that's handled differently from Emacs-23.
> [ IIUC you've gotten to the same conclusion.  ]

While there could be more than one way to cut this cake, I still think
we should encourage Lips programmers to use the `cursor' property in
these situations.

> > If you still think we should support your original code, we should
> > schedule some post-24.1 redesign and refactoring.  Let me know what
> > you think.
> 
> I don't think it's a very high priority problem, but it would be good to
> try and tackle it, yes (post-24.1).

OK.

Should we close this bug, then?

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

>> > For some value of "correctly".  E.g., position the cursor over the
>> > ".", which is the first file name in the archive, and type "C-x =".
>> > You will see "63", which is a lie: the actual value of point is 111.
>> I don't think it's a lie because that text is preceded by invisible
>> text, so while "." is at 111, point is indeed at 63 (and that's just
>> because of adjust_point_for_property, so we can get point to stay at
>> 111 by being more careful with text property's stickiness).
> Having point report X when it is position on a character whose
> position is Y, or jump when you move it from one character of a file
> name to the next violates the principle of least astonishment, IMO.

That's an inevitable consequence of using `invisible'.  So yes, I agree
that `invisible' should be used with great care, but there is no bug in
this particular part (the underlying problem is that the block cursor is
drawn *on* characters whereas point is *between* characters).

>> the cursor is drawn at the right place.  I think this is the core of the
>> problem that's handled differently from Emacs-23.
>> [ IIUC you've gotten to the same conclusion.  ]
> While there could be more than one way to cut this cake, I still think
> we should encourage Lips programmers to use the `cursor' property in
> these situations.

Hmm... I haven't actually tried to use the `cursor' property, but I'm
not sure it would help when point is actually outside of the text area
covered by any of the display strings.

> Should we close this bug, then?

We might as well keep it open until the problem is actually solved.


        Stefan

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 11094@debbugs.gnu.org
> Date: Mon, 02 Apr 2012 09:09:34 -0400
> 
> Hmm... I haven't actually tried to use the `cursor' property, but I'm
> not sure it would help when point is actually outside of the text area
> covered by any of the display strings.

It could do that, if you give the property an integer value.

> > Should we close this bug, then?
> 
> We might as well keep it open until the problem is actually solved.

Then please define precisely what problem needs to be solved, because
I'm definitely confused about that now.

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

>> We might as well keep it open until the problem is actually solved.
> Then please define precisely what problem needs to be solved, because
> I'm definitely confused about that now.

We could start with:

  emacs -Q
  (put-text-property (point-min) (+ 2 (point-min)) 'invisible t)
  (put-text-property (+ 2 (point-min)) (+ 4 (point-min)) 'display "<>")
  (goto-char (point-min))

where I'd expect the cursor to be drawn to the left of "<>" rather than
to the right.


        Stefan

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 11094@debbugs.gnu.org
> Date: Mon, 02 Apr 2012 16:15:17 -0400
> 
>   emacs -Q
>   (put-text-property (point-min) (+ 2 (point-min)) 'invisible t)
>   (put-text-property (+ 2 (point-min)) (+ 4 (point-min)) 'display "<>")
>   (goto-char (point-min))
> 
> where I'd expect the cursor to be drawn to the left of "<>" rather than
> to the right.

Isn't this an issue with point adjustments?  If I set
global-disable-point-adjustment non-nil, I get the cursor where you
want it.

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

>> emacs -Q
>> (put-text-property (point-min) (+ 2 (point-min)) 'invisible t)
>> (put-text-property (+ 2 (point-min)) (+ 4 (point-min)) 'display "<>")
>> (goto-char (point-min))
>> where I'd expect the cursor to be drawn to the left of "<>" rather than
>> to the right.
> Isn't this an issue with point adjustments?

No: the (goto-char (point-min)) really moves to (point-min) for me, as
can be verified with M-: (point).

> If I set global-disable-point-adjustment non-nil, I get the cursor
> where you want it.

It makes no difference for me: point as at BOB, but the cursor is drawn
after the "<>".


        Stefan

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 11094@debbugs.gnu.org
> Date: Tue, 03 Apr 2012 09:37:08 -0400
> 
> >> emacs -Q
> >> (put-text-property (point-min) (+ 2 (point-min)) 'invisible t)
> >> (put-text-property (+ 2 (point-min)) (+ 4 (point-min)) 'display "<>")
> >> (goto-char (point-min))
> >> where I'd expect the cursor to be drawn to the left of "<>" rather than
> >> to the right.
> > Isn't this an issue with point adjustments?
> 
> No: the (goto-char (point-min)) really moves to (point-min) for me, as
> can be verified with M-: (point).
> 
> > If I set global-disable-point-adjustment non-nil, I get the cursor
> > where you want it.
> 
> It makes no difference for me: point as at BOB, but the cursor is drawn
> after the "<>".

The patch below solves this problem.  Do you think it is safe enough
for the release branch?

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-03-31 19:30:53 +0000
+++ src/xdisp.c	2012-04-07 11:58:19 +0000
@@ -14042,15 +14042,18 @@ set_cursor_from_row (struct window *w, s
 		      || pos <= tem)
 		    {
 		      /* If the string from which this glyph came is
-			 found in the buffer at point, then we've
-			 found the glyph we've been looking for.  If
-			 it comes from an overlay (tem == 0), and it
-			 has the `cursor' property on one of its
+			 found in the buffer at point, or at position
+			 that is closer to point than pos_after, then
+			 we've found the glyph we've been looking for.
+			 If it comes from an overlay (tem == 0), and
+			 it has the `cursor' property on one of its
 			 glyphs, record that glyph as a candidate for
 			 displaying the cursor.  (As in the
 			 unidirectional version, we will display the
 			 cursor on the last candidate we find.)  */
-		      if (tem == 0 || tem == pt_old)
+		      if (tem == 0
+			  || tem == pt_old
+			  || (tem - pt_old > 0 && tem < pos_after))
 			{
 			  /* The glyphs from this string could have
 			     been reordered.  Find the one with the
@@ -14088,7 +14091,8 @@ set_cursor_from_row (struct window *w, s
 				}
 			    }
 
-			  if (tem == pt_old)
+			  if (tem == pt_old
+			      || (tem - pt_old > 0 && tem < pos_after))
 			    goto compute_x;
 			}
 		      if (tem)

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

> The patch below solves this problem.  Do you think it is safe enough
> for the release branch?

Let's keep it on the trunk for now.


        Stefan

bug#11094: Wrong cursor positioning with display+invisible

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 11094@debbugs.gnu.org
> Date: Sun, 08 Apr 2012 22:23:17 -0400
> 
> > The patch below solves this problem.  Do you think it is safe enough
> > for the release branch?
> 
> Let's keep it on the trunk for now.

Done as trunk revision 107809.

Should I close this bug, or is there anything else?

bug#11094: Wrong cursor positioning with display+invisible

[Stefan Monnier]

> Should I close this bug, or is there anything else?

Closed, thanks,


        Stefan