bug#19200: Point adjustemnt moves *into* invisible text

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

1. Yank this in an empty buffer in fundamental-mode and evaluate it.

(progn
  (goto-char (point-min))
  (insert "1\n"
	  (propertize "3\n" 'invisible t)
	  "5\n"
	  "7\n")
	  (backward-char 2))

2. M-< C-n

3. M-: (point) RET

  This should say 3 (where get-pos-property says it's not invisible)
  but returns 5 (where get-pos-property says it is invisible)!

4. M-: (point) RET

  This now returns 3.


-- Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Hi,

this bug is tantalizing us in Magit:

  https://github.com/magit/magit/issues/2592.

If someone would want to have a look some time...that would be nice!

Suggestions of how to circumvent the problem until then are welcome
too.


Regards,

Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

> this bug is tantalizing us in Magit:
>   https://github.com/magit/magit/issues/2592.
> If someone would want to have a look some time...that would be nice!

Are you sure you're hitting exactly this same bug?
IOW can you circumvent the bug by adding M-: (point) RET ?


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> > this bug is tantalizing us in Magit:
> >   https://github.com/magit/magit/issues/2592.
> > If someone would want to have a look some time...that would be nice!
>
> Are you sure you're hitting exactly this same bug?
> IOW can you circumvent the bug by adding M-: (point) RET ?

First question: no, I'm not, it's similar, but maybe not exactly the
same.  The issue in Magit seems to be dependent to the setting
goal-column -> 0.  But second question: yes!


And I found that when I replace

  (if (zerop col)
      (beginning-of-line)
    (move-to-column col))

in the definition of line-move-to-column (at the beginning) with just

  (move-to-column col)

(i.e. the else case)

the Magit issue is fixed.  I have no clue why, however.


Shall I make a separate report - or will cursor-sensor.el care about
this stuff at some time in the future?  Looking at the code in
simple.el, fixing this stuff will not be fun...


Regards,

Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
> > > this bug is tantalizing us in Magit:
> > >   https://github.com/magit/magit/issues/2592.
> > > If someone would want to have a look some time...that would be nice!
> >
> > Are you sure you're hitting exactly this same bug?
> > IOW can you circumvent the bug by adding M-: (point) RET ?
>
> First question: no, I'm not, it's similar, but maybe not exactly the
> same.  The issue in Magit seems to be dependent to the setting
> goal-column -> 0.  But second question: yes!

And Edebug at least reveals that it is the cursor adjustment that moves
point into invisible text, so the bug subject is correct for the Magit
issue.

I can post a recipe for this specific case tomorrow.


Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Mon, 21 Mar 2016 03:23:55 +0100
> Cc: 19200@debbugs.gnu.org, Jonas Bernoulli <jonas@bernoul.li>
> 
> And Edebug at least reveals that it is the cursor adjustment that moves
> point into invisible text

If Edebug told you that, then Edebug lied.  It's vertical-motion, not
point adjustment.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

>> Are you sure you're hitting exactly this same bug?
>> IOW can you circumvent the bug by adding M-: (point) RET ?
> First question: no, I'm not, it's similar, but maybe not exactly the
> same.  But second question: yes!

OK, if M-: (point) RET "fixes" the problem, then it's probably hitting
the same bug.

> The issue in Magit seems to be dependent to the setting
> goal-column -> 0.

I don't think that's very surprising: if goal-column is non-zero, then
point doesn't end in an invisible region since you only have invisible text
in column 0, IIUC.

> And I found that when I replace

>   (if (zerop col)
>       (beginning-of-line)
>     (move-to-column col))
>
> in the definition of line-move-to-column (at the beginning) with just
>
>   (move-to-column col)
>
> the Magit issue is fixed.  I have no clue why, however.

move-to-column moves forward from BOL, whereas beginning-of-line moves
only backward, so the two probably end up at opposite ends of the
invisible region.

BTW, regardless of fixing this bug, I think you have a problem: if the
exact position within the invisible text is so important that it can end up
selecting a different line, then fixing bug#19200 won't do you much
good: there are many cases where point-adjustment won't do what you want
(e.g. it won't move point at all, even though it's somewhere in the
middle of the invisible region) and hence it will end up "at the wrong place".

The way point-adjustment is defined is too conservative for that.
So I suggest you try and make you code more robust w.r.t the position of
point within an invisible region in any case.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

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

> move-to-column moves forward from BOL, whereas beginning-of-line moves
> only backward, so the two probably end up at opposite ends of the
> invisible region.
>
> BTW, regardless of fixing this bug, I think you have a problem: if the
> exact position within the invisible text is so important that it can
> end up
> selecting a different line, then fixing bug#19200 won't do you much
> good: there are many cases where point-adjustment won't do what you want
> (e.g. it won't move point at all, even though it's somewhere in the
> middle of the invisible region) and hence it will end up "at the wrong
> place".
>
> The way point-adjustment is defined is too conservative for that.
> So I suggest you try and make you code more robust w.r.t the position of
> point within an invisible region in any case.

Well, we just want that cursor movement starting from a visible position
not end up at an invisible position - the docs seem to suggest that this
should not happen, right?

We have the following, very simple situation:

Line N
{ M invisible lines here }
Line N+M+1

Line N and Line N+M+1 are completely visible, and displayed after each
other.  Point is at the start of Line N, no invisible text near that
position.  Now I hit [down] one time and end up somewhere inside the
invisible text in between.


Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

>> The way point-adjustment is defined is too conservative for that.
>> So I suggest you try and make you code more robust w.r.t the position of
>> point within an invisible region in any case.
> Well, we just want that cursor movement starting from a visible position
> not end up at an invisible position - the docs seem to suggest that this
> should not happen, right?

It doesn't guarantee it in all cases, no.  E.g. it won't work if the
command that moves point started/ended in another buffer, or if the
command set disable-point-adjustment, plus a few other cases.

> We have the following, very simple situation:

> Line N
> { M invisible lines here }
> Line N+M+1

> Line N and Line N+M+1 are completely visible, and displayed after each
> other.  Point is at the start of Line N, no invisible text near that
> position.  Now I hit [down] one time and end up somewhere inside the
> invisible text in between.

I agree it's a bug.  I'm just pointing out that even if we fix this bug,
there will be other cases which ae not considered bugs where point will
end up in the middle of the invisible text, and where it would be odd
for your code to then operate on the selection at the "current line"
since that's not the line that the user is actually seeing.

So, yes, point-adjustment needs to be fixed (this is bug#19200), but
Magit also needs to be fixed anyway (and after that fix, it won't care
about bug#19200 nearly as much, I think).


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

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

> So, yes, point-adjustment needs to be fixed (this is bug#19200), but
> Magit also needs to be fixed anyway (and after that fix, it won't care
> about bug#19200 nearly as much, I think).

What would you suggest?  What would a different case that is not a bug
in Emacs look like?

Just adding something that tries to repair what goes wrong is not
enough.  If cursor movement can end up at a random position inside
invisible text, we would need to reimplement all moving commands, since
we cannot derive from the cursor position what the correct position
would have been.  With other words, I don't see what part of this issue
is specific to Magit.


Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

> What would you suggest?

1- Don't use invisible text, remove the text instead.
2- Start any command where it can matter with a call to a function that
   skips forward over invisible text.
3- Make your invisible text intangible (ideally with cursor-intangible,
   since the `intangible' text property has all kinds of nasty side
   effects), so the cursor can't stay inside.
4- Some other solution of your choosing.

> What would a different case that is not a bug in Emacs look like?

Hmm... I did give you examples:

    if the command that moves point started/ended in another buffer, or
    if the command set disable-point-adjustment, plus a few other cases.

> Just adding something that tries to repair what goes wrong is not
> enough.

Why not?

> If cursor movement can end up at a random position inside invisible
> text, we would need to reimplement all moving commands, since we
> cannot derive from the cursor position what the correct position would
> have been.  With other words, I don't see what part of this issue is
> specific to Magit.

Magit is the one which introduces invisible text.
The core functions can't know exactly why the text is invisible and
hence what to do about it in each and every possible case.  Only Magit
knows that.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 21 Mar 2016 11:36:23 -0400
> Cc: 19200@debbugs.gnu.org, Jonas Bernoulli <jonas@bernoul.li>
> 
> >> The way point-adjustment is defined is too conservative for that.
> >> So I suggest you try and make you code more robust w.r.t the position of
> >> point within an invisible region in any case.
> > Well, we just want that cursor movement starting from a visible position
> > not end up at an invisible position - the docs seem to suggest that this
> > should not happen, right?
> 
> It doesn't guarantee it in all cases, no.  E.g. it won't work if the
> command that moves point started/ended in another buffer, or if the
> command set disable-point-adjustment, plus a few other cases.
> 
> > We have the following, very simple situation:
> 
> > Line N
> > { M invisible lines here }
> > Line N+M+1
> 
> > Line N and Line N+M+1 are completely visible, and displayed after each
> > other.  Point is at the start of Line N, no invisible text near that
> > position.  Now I hit [down] one time and end up somewhere inside the
> > invisible text in between.
> 
> I agree it's a bug.  I'm just pointing out that even if we fix this bug,
> there will be other cases which ae not considered bugs where point will
> end up in the middle of the invisible text, and where it would be odd
> for your code to then operate on the selection at the "current line"
> since that's not the line that the user is actually seeing.
> 
> So, yes, point-adjustment needs to be fixed (this is bug#19200), but
> Magit also needs to be fixed anyway (and after that fix, it won't care
> about bug#19200 nearly as much, I think).

Can someone tell me why you think point adjustment has anything to do
with the original report for this bug?  Because I fail to see any
connection; maybe I'm missing something.

What I see is that vertical-motion, called by next-line's subroutines,
ends up at position 5, because when it tries to get to column zero, it
skips the invisible text between positions 3 and 5.  It stops at 5
because the function called to move to the goal column can only move
within a single display line.  IOW, this is a simple artifact of the
implementation of vertical-motion.

Michael, does the patch below fix the Magit problem?  (It does fix the
simple test case presented by Stefan at the beginning of this bug
report.)

diff --git a/src/indent.c b/src/indent.c
index d5162e3..e10b084 100644
--- a/src/indent.c
+++ b/src/indent.c
@@ -2222,7 +2222,8 @@ whether or not it is currently displayed in some window.  */)
 	 an addition to the hscroll amount.  */
       if (lcols_given)
 	{
-	  move_it_in_display_line (&it, ZV, first_x + to_x, MOVE_TO_X);
+	  if (it.current_x < first_x + to_x)
+	    move_it_in_display_line (&it, ZV, first_x + to_x, MOVE_TO_X);
 	  /* If we find ourselves in the middle of an overlay string
 	     which includes a newline after current string position,
 	     we need to move by lines until we get out of the string,

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Michael, does the patch below fix the Magit problem?  (It does fix the
> simple test case presented by Stefan at the beginning of this bug
> report.)

Unfortunately, no.

Here is a test case which hopefully resembles the situation in Magit:

(progn
  (insert "111" "\n222\n"
          (propertize "333\n444\n\n" 'invisible t)
          "555\n666\n"))

Turn on line-number-mode and set goal-column -> 0, and move around
(Esp. up/down).  You'll see the cursor drawn over the first "5", but
point is actually located inside the invisible text.

I wonder however why in the above recipe the M-: (point) trick does
_not_ "work".


Regards,

Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  jonas@bernoul.li,  19200@debbugs.gnu.org
> Date: Mon, 21 Mar 2016 20:24:31 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Michael, does the patch below fix the Magit problem?  (It does fix the
> > simple test case presented by Stefan at the beginning of this bug
> > report.)
> 
> Unfortunately, no.
> 
> Here is a test case which hopefully resembles the situation in Magit:
> 
> (progn
>   (insert "111" "\n222\n"
>           (propertize "333\n444\n\n" 'invisible t)
>           "555\n666\n"))

This is an entirely different problem.

> Turn on line-number-mode and set goal-column -> 0, and move around
> (Esp. up/down).  You'll see the cursor drawn over the first "5", but
> point is actually located inside the invisible text.

Why do you need line-number-mode and goal-column?  I can see the
problem just by moving with C-f from the end of the "222" line:
"C-x =" says point is on the first "3", but the cursor is shown on
"5".  Isn't this the problem you want fixed?

> I wonder however why in the above recipe the M-: (point) trick does
> _not_ "work".

Because it's an entirely different problem.

I think you should file a separate bug for it.

Meanwhile, perhaps Stefan can see if my proposed change fixes whatever
the real-life use case he first saw the problem in, not just the
recipe he posted.

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > (progn
> >   (insert "111" "\n222\n"
> >           (propertize "333\n444\n\n" 'invisible t)
> >           "555\n666\n"))
>
> This is an entirely different problem.

Hmm, ok.


> Why do you need line-number-mode and goal-column?

`line-number-mode' because I really meant `hl-line-mode': that would
have shown a visible effect, namely that cursor is located on a line
different from the one where it appears.  But yes, it's unrelated to the
recipe.

Second question: for the recipe, it's not relevant indeed, but it is in
Magit, because in Magit, they have set line-move-visual -> nil.  That
fixes the problem, but it fails to do so (only) when goal-column -> nil.

>  I can see the problem just by moving with C-f from the end of the
> "222" line: "C-x =" says point is on the first "3", but the cursor is
> shown on "5".  Isn't this the problem you want fixed?

Yes!

> > I wonder however why in the above recipe the M-: (point) trick does
> > _not_ "work".
>
> Because it's an entirely different problem.

But in Magit, it _does_ help (i.e. move the cursor out of the invisible
text).  In my recipe, which is modeled after the Magit case, it doesn't.

> I think you should file a separate bug for it.

Ok, will do.  Thanks for investing time here.


Regards,

Michael.

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > I think you should file a separate bug for it.
>
> Ok, will do.  Thanks for investing time here.

Done: bug#23079.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

> Can someone tell me why you think point adjustment has anything to do
> with the original report for this bug?  Because I fail to see any
> connection; maybe I'm missing something.

> What I see is that vertical-motion, called by next-line's subroutines,
> ends up at position 5, because when it tries to get to column zero, it
> skips the invisible text between positions 3 and 5.  It stops at 5
> because the function called to move to the goal column can only move
> within a single display line.  IOW, this is a simple artifact of the
> implementation of vertical-motion.

No, it's OK for vertical-motion to move to position 5.

But adjust_point_for_property should move it back to position
3 (because that still ends up being a forward motion, when compared to
the original point position, and it is in a "less-invisible" spot).

> Michael, does the patch below fix the Magit problem?  (It does fix the
> simple test case presented by Stefan at the beginning of this bug
> report.)

It just works around the problem, AFAICT.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 19200@debbugs.gnu.org, michael_heerdegen@web.de, jonas@bernoul.li
> Date: Mon, 21 Mar 2016 16:43:21 -0400
> 
> > Can someone tell me why you think point adjustment has anything to do
> > with the original report for this bug?  Because I fail to see any
> > connection; maybe I'm missing something.
> 
> > What I see is that vertical-motion, called by next-line's subroutines,
> > ends up at position 5, because when it tries to get to column zero, it
> > skips the invisible text between positions 3 and 5.  It stops at 5
> > because the function called to move to the goal column can only move
> > within a single display line.  IOW, this is a simple artifact of the
> > implementation of vertical-motion.
> 
> No, it's OK for vertical-motion to move to position 5.

OK, then I won't install that change.

> But adjust_point_for_property should move it back to position
> 3 (because that still ends up being a forward motion, when compared to
> the original point position, and it is in a "less-invisible" spot).

Your original report says:

  3. M-: (point) RET

    This should say 3 (where get-pos-property says it's not invisible)
    but returns 5 (where get-pos-property says it is invisible)!

But I'm sure you know very well that point adjustment code doesn't use
get-pos-property, it uses get-char-property-and-overlay, and the
latter says position 5 is visible.  There was an attempt to use
get-pos-property in that code, but it backfired and was disabled.

So what exactly is this bug about?

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

> But I'm sure you know very well that point adjustment code doesn't use
> get-pos-property, it uses get-char-property-and-overlay, and the
> latter says position 5 is visible.  There was an attempt to use
> get-pos-property in that code, but it backfired and was disabled.

No, I didn't remember (and still don't actually, tho I now see the
corresponding comments and #if0 in the code).

But that change only affects the decision of what we consider as the
boundaries of a chunk of invisible text (so it makes no difference in
the present test case where there is no such ambiguity).

Once those boundaries are found, we do use Fget_pos_property to decide
which boundary to go to.

> So what exactly is this bug about?

There are 2 odd behaviors:
- point adjustment doesn't bring us to position 3 after C-n
- M-: (point) has the side-effect of bringing us to position 3
  My guess here is that after the M-: command, at the end of
  command_loop_1, last_point_position refers to a position in another
  buffer (i.e. in the minibuffer), so it thinks there was a movement and
  hence re-runs adjust_point_for_property, which this time gets it right.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 19200@debbugs.gnu.org,  michael_heerdegen@web.de,  jonas@bernoul.li
> Date: Tue, 22 Mar 2016 14:36:10 -0400
> 
> > So what exactly is this bug about?
> 
> There are 2 odd behaviors:
> - point adjustment doesn't bring us to position 3 after C-n

Why is that a problem?  Position 3 is invisible, so we shouldn't
expect to end up with point there.

> - M-: (point) has the side-effect of bringing us to position 3
>   My guess here is that after the M-: command, at the end of
>   command_loop_1, last_point_position refers to a position in another
>   buffer (i.e. in the minibuffer), so it thinks there was a movement and
>   hence re-runs adjust_point_for_property, which this time gets it right.

Maybe.  If this is the bug to solve, I could look into it.  But your
original reports said this is actually the correct position of point,
a claim which I don't understand.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

>> - point adjustment doesn't bring us to position 3 after C-n
> Why is that a problem?  Position 3 is invisible, so we shouldn't
> expect to end up with point there.

No, you have it backwards: position 5 is invisible and position 3 is not.
The "evidence" for that is that if you go to position 3 and insert a char,
that char will be visible, whereas if you go to position 5 and insert
a char, that char will be invisible.

>> - M-: (point) has the side-effect of bringing us to position 3
>> My guess here is that after the M-: command, at the end of
>> command_loop_1, last_point_position refers to a position in another
>> buffer (i.e. in the minibuffer), so it thinks there was a movement and
>> hence re-runs adjust_point_for_property, which this time gets it right.
> Maybe.  If this is the bug to solve, I could look into it.

No, this bug is secondary.  The main bug is that we end up at position
5 after C-n.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 19200@debbugs.gnu.org,  michael_heerdegen@web.de,  jonas@bernoul.li
> Date: Tue, 22 Mar 2016 22:13:04 -0400
> 
> >> - point adjustment doesn't bring us to position 3 after C-n
> > Why is that a problem?  Position 3 is invisible, so we shouldn't
> > expect to end up with point there.
> 
> No, you have it backwards: position 5 is invisible and position 3 is not.

So you are saying that we also have a display bug, in that what should
have been on the screen is "3" and not "5"? ;-)

> The "evidence" for that is that if you go to position 3 and insert a char,
> that char will be visible, whereas if you go to position 5 and insert
> a char, that char will be invisible.

You are talking about a different kind of "invisible", the kind that
is different from how the display engine, and any cursor-motion
commands that use its layout routines, interprets "invisible".  That
is why what you expect from the related code is hard to get: it is
barely supported by the relevant code.

(Personally, I think your notion of "invisible" is also confusing for
the user, in that it puts the cursor on a character whose position is
not the same as point.  The other notion of "invisible" also has its
disadvantages, so it's not easy to decide which one is "right", but at
least it doesn't fight an uphill battle against the display engine.)

> >> - M-: (point) has the side-effect of bringing us to position 3
> >> My guess here is that after the M-: command, at the end of
> >> command_loop_1, last_point_position refers to a position in another
> >> buffer (i.e. in the minibuffer), so it thinks there was a movement and
> >> hence re-runs adjust_point_for_property, which this time gets it right.
> > Maybe.  If this is the bug to solve, I could look into it.
> 
> No, this bug is secondary.  The main bug is that we end up at position
> 5 after C-n.

Since we don't know how to fix the main bug, would it be an
improvement to solve the secondary one?

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

>> No, you have it backwards: position 5 is invisible and position 3 is not.
> So you are saying that we also have a display bug, in that what should
> have been on the screen is "3" and not "5"? ;-)

No: the character after position 3 is invisible, but the position 3 is not.
Inversely, the character after position 5 is visible while the position
is not.

> You are talking about a different kind of "invisible", the kind that
> is different from how the display engine, and any cursor-motion
> commands that use its layout routines, interprets "invisible".

No.  You just have to remember that characters are between positions and
positions are between characters, so the two can't be conflated.

> (Personally, I think your notion of "invisible" is also confusing for
> the user, in that it puts the cursor on a character whose position is
> not the same as point.

That's not my choice and that's not hard coded.  It's the choice of the
stickiness settings for that particular invisible property.  It can be
controlled by text property stickiness and overlay's marker's
insertion types.

E.g. if you use overlays to make the text invisible, then (by default)
the position's invisibility is the same as the following character's
(which is what you seem to  like).  For text-properties, by default it's
the reverse (i.e. the position's visibility is the same as the
*preceding* character).

> The other notion of "invisible" also has its disadvantages, so it's
> not easy to decide which one is "right", but at least it doesn't fight
> an uphill battle against the display engine.)

AFAIK there's no relevant interaction with the display engine.
The only real problem is that people don't realize that the reality
is more complex than what they expect.

>> No, this bug is secondary.  The main bug is that we end up at position
>> 5 after C-n.
> Since we don't know how to fix the main bug, would it be an
> improvement to solve the secondary one?

The secondary bug is pretty cosmetic and (at least in this case) is
rather helpful, so I'm not sure it would be an improvement in itself.

The issue of the main bug is not so much that we don't know how to fix
it, but that noone has bothered to investigate it to try and figure out
what is actually happening.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 19200@debbugs.gnu.org, michael_heerdegen@web.de, jonas@bernoul.li
> Date: Wed, 23 Mar 2016 11:32:29 -0400
> 
> >> No, you have it backwards: position 5 is invisible and position 3 is not.
> > So you are saying that we also have a display bug, in that what should
> > have been on the screen is "3" and not "5"? ;-)
> 
> No: the character after position 3 is invisible, but the position 3 is not.
> Inversely, the character after position 5 is visible while the position
> is not.

But we display characters, not positions.  And the cursor is displayed
"on" some character as well.

> > You are talking about a different kind of "invisible", the kind that
> > is different from how the display engine, and any cursor-motion
> > commands that use its layout routines, interprets "invisible".
> 
> No.  You just have to remember that characters are between positions and
> positions are between characters, so the two can't be conflated.

Thank you, I don't think I forgot that.

And it isn't important what I remember, because above I was talking
about what the display code does: it examines properties of characters
using the likes of get-char-property, and behaves accordingly.

> > (Personally, I think your notion of "invisible" is also confusing for
> > the user, in that it puts the cursor on a character whose position is
> > not the same as point.
> 
> That's not my choice and that's not hard coded.  It's the choice of the
> stickiness settings for that particular invisible property.  It can be
> controlled by text property stickiness and overlay's marker's
> insertion types.

That is not visible until you insert a character.  By contrast, the
characters and the cursor are visible at all times.

> > The other notion of "invisible" also has its disadvantages, so it's
> > not easy to decide which one is "right", but at least it doesn't fight
> > an uphill battle against the display engine.)
> 
> AFAIK there's no relevant interaction with the display engine.

Read the code: it's all over the place.  Why do you think
vertical-motion ends up at position 5 in the test case you presented
in this bug report?

> The secondary bug is pretty cosmetic and (at least in this case) is
> rather helpful, so I'm not sure it would be an improvement in itself.

OK, then I don't see what can be done here.

> The issue of the main bug is not so much that we don't know how to fix
> it, but that noone has bothered to investigate it to try and figure out
> what is actually happening.

Didn't I do that?  Doesn't the fact that the relevant code calls
get-char-property-and-overlay explain what happens?

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

> But we display characters, not positions.  And the cursor is displayed
> "on" some character as well.

Yes.  And from that point of view, there's no difference whether point
is at position 3, 4, or 5: the display will be the same.

So the choice of whether to put point at 3, 4, or 5 can't be just based
on "what it looks like" but on what happens when the user performs
an operation.

And "The Right Thing" will then depend on the operation, and the reason
why the text was made invisible.  Which is why the chosen position needs
to be controllable (the fact that it's controlled by stickiness is
somewhat arbitrary in this respect).

The choice of using stickiness is based on the idea that an important
operation is insertion, in which case it's important to make sure that
when the user moves around and edits a buffer that has invisible text,
she doesn't end up inserting text that's invisible (and hence get the
impression that the insertion somehow didn't even happen).

> And it isn't important what I remember, because above I was talking
> about what the display code does: it examines properties of characters
> using the likes of get-char-property, and behaves accordingly.

I still don't see any relationship with point-adjustment.

>> > The other notion of "invisible" also has its disadvantages, so it's
>> > not easy to decide which one is "right", but at least it doesn't fight
>> > an uphill battle against the display engine.)
>> AFAIK there's no relevant interaction with the display engine.
> Read the code: it's all over the place.  Why do you think
> vertical-motion ends up at position 5 in the test case you presented
> in this bug report?

I don't see how that relates.  Point-adjustment has to work regardless
of which command was used, and point can end up at position 4 or
5 rather than position 3 for all kinds of reasons unrelated to
invisibility, so if vertical-motion goes to position 5, it's really (or
at least should be) a non-issue for point-adjustment.

>> The issue of the main bug is not so much that we don't know how to fix
>> it, but that noone has bothered to investigate it to try and figure out
>> what is actually happening.
> Didn't I do that?  Doesn't the fact that the relevant code calls
> get-char-property-and-overlay explain what happens?

No: the get-char-property-and-overlay calls will only determine the
boundaries of the invisible text (i.e. they should find that the
invisible chunk goes between 3 and 5).

After that, adjust_point_for_property should start by moving point to
position 3 (because last_pt should be < 3).

And after that it should use Fget_pos_property to decide whether to stay
at position 3 or to move to position 5, and in this case it should
choose to stay at position 3.

So someone needs to step through the code and figure out why this
doesn't happen.  E.g. maybe it doesn't happen because
adjust_point_for_property is not called at all.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 19200@debbugs.gnu.org, michael_heerdegen@web.de, jonas@bernoul.li
> Date: Wed, 23 Mar 2016 12:10:26 -0400
> 
> So someone needs to step through the code and figure out why this
> doesn't happen.

I guess you expected me to be that Someone...

> E.g. maybe it doesn't happen because adjust_point_for_property is
> not called at all.

It _is_ called.

> >> The issue of the main bug is not so much that we don't know how to fix
> >> it, but that noone has bothered to investigate it to try and figure out
> >> what is actually happening.
> > Didn't I do that?  Doesn't the fact that the relevant code calls
> > get-char-property-and-overlay explain what happens?
> 
> No: the get-char-property-and-overlay calls will only determine the
> boundaries of the invisible text (i.e. they should find that the
> invisible chunk goes between 3 and 5).

The function is entered with point at 5, so 'beg' and 'end' start with
that value.

Then get_char_property_and_overlay in the "while (end < ZV" loop
returns nil for position 5, so that loop is exited immediately.

Then a similar call in the "while (beg > BEGV" loop returns t for
position 5 - 1 = 4.  Then previous-single-char-property-change returns
3, so 'beg' becomes 3.  Then another call to
get_char_property_and_overlay returns nil for position 3 - 1 = 2, and
the while loop is exited with beg = 3 and end = 5.  Since point is 5,
we land here:

	  /* Pretend the area doesn't exist if the buffer is not
	     modified.  */
	  if (!modified && !ellipsis && beg < end)
	    {
	      if (last_pt == beg && PT == end && end < ZV)
		(check_composition = check_display = true, SET_PT (end + 1));
	      else if (last_pt == end && PT == beg && beg > BEGV)
		(check_composition = check_display = true, SET_PT (beg - 1));
	      else if (PT == ((PT < last_pt) ? beg : end))
		/* We've already moved as far as we can.  Trying to go
		   to the other end would mean moving backwards and thus
		   could lead to an infinite loop.  */
		;
	      else if (val = Fget_pos_property (make_number (PT),
						Qinvisible, Qnil),
		       TEXT_PROP_MEANS_INVISIBLE (val)
		       && (val = (Fget_pos_property
				  (make_number (PT == beg ? end : beg),
				   Qinvisible, Qnil)),
			   !TEXT_PROP_MEANS_INVISIBLE (val)))
		(check_composition = check_display = true,
		 SET_PT (PT == beg ? end : beg));
	    }

last_pt is 1, so we wind up in this branch:

	      else if (PT == ((PT < last_pt) ? beg : end))
		/* We've already moved as far as we can.  Trying to go
		   to the other end would mean moving backwards and thus
		   could lead to an infinite loop.  */
		;

which does nothing.  So point never moves and stays at 5.

> After that, adjust_point_for_property should start by moving point to
> position 3 (because last_pt should be < 3).

It doesn't.

> And after that it should use Fget_pos_property to decide whether to stay
> at position 3 or to move to position 5, and in this case it should
> choose to stay at position 3.

It doesn't get there.

bug#19200: Point adjustemnt moves *into* invisible text

[Stefan Monnier]

>> So someone needs to step through the code and figure out why this
>> doesn't happen.
> I guess you expected me to be that Someone...

Actually, no, I expected Michael would be motivated to do it, since he's
the one presumably affected.

> get_char_property_and_overlay returns nil for position 3 - 1 = 2, and
> the while loop is exited with beg = 3 and end = 5.

Good so far.

> Since point is 5,
> we land here:

OK, so indeed, the problem is in

	  /* Move away from the inside area.  */
	  if (beg < PT && end > PT)

In our case, end==PT, but we do want to enter this `if's body to move PT
to `beg'.


        Stefan

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Actually, no, I expected Michael would be motivated to do it, since
> he's the one presumably affected.

I'm sorry - I've been very busy, and I'm not yet familiar enough with
that C level stuff to debug that efficiently...


Michael.

bug#19200: Point adjustment moves *into* invisible text

[John Wiegley]

>>>>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> E.g. if you use overlays to make the text invisible, then (by default) the
> position's invisibility is the same as the following character's (which is
> what you seem to like). For text-properties, by default it's the reverse
> (i.e. the position's visibility is the same as the *preceding* character).

This certainly sounds like an unfortunate discrepancy. How historical is it?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#19200: Point adjustemnt moves *into* invisible text

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Mon, 21 Mar 2016 03:15:34 +0100
> Cc: 19200@debbugs.gnu.org, Jonas Bernoulli <jonas@bernoul.li>
> 
> And I found that when I replace
> 
>   (if (zerop col)
>       (beginning-of-line)
>     (move-to-column col))
> 
> in the definition of line-move-to-column (at the beginning) with just
> 
>   (move-to-column col)
> 
> (i.e. the else case)
> 
> the Magit issue is fixed.  I have no clue why, however.

This code is not executed when running Stefan's original test case.

bug#19200: Point adjustemnt moves *into* invisible text

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> This code is not executed when running Stefan's original test case.

I guess that's the cause why my "fix" doesn't help for his recipe ;-)