Subject: Re: bug#56682: feature/improved-locked-narrowing 9dee6df39c: Reworked locked narrowing.

From: Eli Zaretskii <[email-masked]>
Date: Sat, 01 Apr 2023 14:11:59 +0300

> Date: Wed, 29 Mar 2023 14:52:08 +0000
> From: Gregory Heytings <[email-masked]>
> cc: [email-masked], [email-masked]
> 
> 
> >
> > I finally had enough free time to do that.  You will find the result in 
> > the scratch/long-lines-cleanup branch.  I contains the following four 
> > commits, in that order:
> >
> > 1. 85ed1c9ca6 with the renamings and documentation improvements
> >
> > 2. 7e26a5c774 with the patch mentioned above
> >
> > 3. afc2c6c13c with a bug fix, which makes cursor motion commands much 
> > more accurate in long lines
> >
> > 4. 974e4f3333 with a minor cleanup
> >
> 
> I just pushed a fifth commit (2093e010dc) to fix character motion commands 
> in terminals.

Thanks.

I took a look, and these commits do more than just renaming.  They
also change the code in non-trivial ways, and I'm not thrilled about
changing code in those parts at this late stage.

Specific comments:

afc2c6c13c:

  . Misses an optimization opportunity: where pos == BEGV, you can use
    BEGV_BYTE instead of calling CHAR_TO_BYTE.
  . I'm not sure I'm happy about calling find_newline in a loop where
    previous code just made a trivial calculation.  Imagine a buffer
    with a lot of short lines.  What problem exactly is being solved
    here, and how does this change solve it?

2093e010dc:

  . Why such a strange method of finding out whether we are on a TTY
    frame?  The usual method is FRAME_WINDOW_P (XFRAME (w->frame)).
  . The continuation glyph can be present not only on TTY frames, but
    also on GUI frames when one or both of the fringes are disabled,
    so the test needs to be augmented.  I think you need to use
    WINDOW_LEFT_FRINGE_WIDTH and WINDOW_RIGHT_FRINGE_WIDTH.