bug#64696: 30.0.50; indent-to inherits preceding text properties, including 'invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, 64696@debbugs.gnu.org
> Date: Sat, 22 Jul 2023 14:05:00 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Test #2 is unexpected - we are inside invisible region, but
> >> current-column reports as if everything were visible.
> >
> > current-column produces incorrect results when the newline before the
> > current line is invisible.  It always starts from the beginning of the
> > current physical line, even if that is in invisible text.
> 
> Then why does test #3 produce current-column = 0?
> The newline before current line is also invisible there.

I'm guessing it's sheer luck or something.  Bugs are like that: they
don't always behave consistently if you change the conditions
slightly.

I didn't look at that case, and frankly, I'm not thrilled to do that.
The code starts by going to the previous newline, and doesn't pay
attention to whether the newline is itself invisible.  Thereafter, it
looks only for _changes_ in the invisible property, so it will not
consider a stretch of text starting from that newline as invisible.
Given this strategy, it should be clear that the result cannot be
correct in general when the newline at BOL is invisible; all the rest
are details that are not really relevant, and spending time on
understanding exactly what happens in this or that particular case is
from my POV a waste of time.

> > We could teach current-column about invisible newlines, see the patch
> > below.  But I'm not sure this is justified, nor whether it won't break
> > something.  The patch below also has a disadvantage that it will still
> > behave as before for a buffer that is not displayed in any window; if
> > we want that to be fixed as well, the changes will need to be more
> > extensive.  (Basically, we will need to write a non-display version of
> > back_to_previous_visible_line_start.)
> >
> > With the patch below, Test #2 shows "current-column = 6", which is
> > correct, since the cursor is shown after "* Test", with all the rest
> > invisible.
> 
> This will definitely break indentation code.

But is correct, don't you agree?

> Look at `indent-line-to':
> 
>   (beginning-of-line 1)
>   (skip-chars-forward " \t")
>   (let ((cur-col (current-column)))
> <...>
> ((> cur-col column) ; too far right (after tab?)
>            (delete-region (progn (move-to-column column t) (point))
>                           ;; The `move-to-column' call may replace
>                           ;; tabs with spaces, so we can't reuse the
>                           ;; previous start point.
>                           (progn (beginning-of-line 1)
>                                  (skip-chars-forward " \t")
>                                  (point))))

This obviously assumes the text is not invisible.

> I am pretty sure that it is not the only breakage.

I don't insist in making that change.  Quite the opposite, actually.
I also expect it to break gobs of indentation code where invisible
text is involved.  Indentation code should probably temporarily remove
the invisibility spec, while indenting, or something.

Once again, the indentation feature was designed to work on PL source
text and on human-readable text files where the user types text that
is completely, or almost completely, visible.  It was not designed for
Outline and similar modes which hide large portions of the buffer
text.  So it is a small wonder that it doesn't work well in those
cases.

The main motivation to fix scan_for_column to consider more visual
effects was so vertical cursor motion works as expected when large
portions of text are hidden.