Re: [Nano-devel] Softwrap navigation overhaul

[Benno Schulenberg]

On Tue, Jan 31, 2017, at 01:06, David Ramsey wrote:
> On Sat, Jan 28, 2017 at 6:12 AM, Benno Schulenberg
> <address@hidden> wrote:
> > Better rake them together into a single patch and send it
> > to the list separately, get it applied before the overhaul.
> > Keep such things out of the patch set.
> 
> Okay.

With single patch I mean: all the miscellaneous comment tweaks
and corrections.  A coordinated logical change, like patch 37,
should stay a separate patch, of course.

> > But I find the order in which you do things strange.  First you remove
> > tens of things, kind of removing half of the softwrap functionality,
> > and then you start adding back new functions.
> 
> When implementing it, it was easier to remove the ad hoc bits that only
> applied to the current softwrap implementation, and then implenet the
> new bits with the ad hoc bits removed.
> 
> > I would have expected it the other way around: first add the new stuff
> > you need, then transform the movement functions to use the new stuff,
> > and at the end scrap all the then dead code.
> 
> So when it comes to stuff like do_down(), instead of removing code and
> then adding new code, you'd prefer to combine those into one code
> replacement?

You can keep it two separate patches, if you like: one for removal,
and one for addition of the right stuff.  But it would be nice if the
second would come right after the first.

> > It... works.  But nano becomes unusably slow (when syntax coloring is
> > active) when a softwrapped line takes up nearly the whole screen: it
> > takes two seconds to respond to <Up> or <Down> at the top or bottom
> > of the screen.
> 
> It may be partially the effects of having to process such a long line.

Yes, it's all the strlenpt() calls.  And mainly because most of them
start from the head of the line instead of from a known point that
is on the left edge of the screen.

> (I tried it in KWrite for comparison, and that refuses to softwrap
> anything past 1024 characters.)

What does it do  then?  After 1024 characters, a line just runs off
the screen?  And when you go to character 1200 it scrolls horizontally?

> > What I don't like is: when on the bottom row only the first chunk of
> > a softwrapped line is visible, and you put the cursor on the very end
> > of that row, pressing <Right> or <Ctrl+Right> or <Down> there, will
> > scroll just one chunk.  Pressing <End> instead will scroll half a
> > screen (all in smooth scrolling mode).  That isn't right.  It should
> > scroll just enough rows to get the cursor back onto the bottom row.
> 
> I know why: they're using edit_redraw(), so they're using flowing
> scrolling mode.

If home and end used flowing mode, they would do the right thing.
Apparently focusing is TRUE here, when it should be FALSE.

> > 11: Why unwrap the assert?  And why this:
> >
> > -    /* Move to the next line in the file. */
> > +    /* Move the current line of the edit window down. */
> >
> > We're scrolling down, which means that all the text moves up.
> > The comment now seems to say the opposite.
> 
> The comment in the same place in do_up() (move.c, line 418) says "Move
> the current line of the edit window up.",

Yes, but that comment is old -- it dates from 2005.  I have tweaked
and adjusted comments as I went along fixing things, but haven't
done it in a systematic manner.

> and the unwrapped assert is
> because the similar assert in the same place in do_down() (move.c, line
> 416) is unwrapped, despite being overly long.  Why are those things okay
> in do_down(),

They are not okay.  I just didn't get around to changing them.
If you want to know whether I "approve" of something, look at
git blame.  If I changed the line, it is most likely in my style.
If I never touched it, it doesn't mean that I approve.

> > 19, 20: Don't call them reverts.  Because it makes it sound as
> > if those changes were wrong.  But you're changing the whole
> > way that softwrap behaves, so you're not reverting anything,
> > you are changing the entire logic.
> 
> Since those were ad hoc fixes that papered over the problems in bugs
> they referenced, I thought those *were* wrong in a sense.

They didn't paper over the problems, they fixed them.  They weren't
a general solution for all of the softwrap problems, but if each of the
problems had been tackled in this way, it would have been  a complete
solution.  Sure, it couldn't handle a softwrapped line that fills more
than the screen -- but, really, who wants to handle such giant lines?

> > Why is that if within the for loop?  Put it outside and drop the
> > i==nrows.
> 
> I was trying to avoid calling strnlenpt() unless absolutely necessary
> (and if i is zero or less, it won't be), since it's an expensive
> operation.

You mean: if nrows is zero or less?  Okay, but I would still do it
outside the for loop: if (nrows > 0) initialize.

> > In the rest of the function, the comments are excessive.  They are so
> > long that I can't be bothered to read them.
> 
> I'll pare down their wording.

In your comments you sometimes say what is going to be assigned to
what variable: you retell the code in words.  There is no need for
that: the code already shows what is being done with the variables.
The comments (if any are needed) should be at least one level "higher".

> > Also, all this #ifndef NANO_TINY and if ISSET(SOFTWRAP) is making
> > things into a jumble.  Better split each function into two blocks: one
> > for the non-softwrap case, and one for the softwrapping one.  So there
> > will be just one check for isset softwrap and just one #ifndef per
> > function.  Don't bother #ifdeffing the variable.
> 
> Okay.  Do I do this even if code the two paths have in common gets
> duplicated between #ifdefs?

If necessary, yes.

> > Just use rows_from_tail straightaway.
> 
> I figured there was no way to know how many lines there were between
> filebot and current, and if the file were huge, it could be out of int
> range.

Loading a file of two hundred thousand lines takes some three seconds
on my machine.  An integer can count up to two billion lines -- loading
a file of that size would take eight hours, and require eighty gigabytes
of memory (when an avarage line is forty characters long).  So... the
chances we'll run into that problem... will become interesting in maybe
ten years time.

> > Further, the only time that this maxchunk is used is in
> > current_is_below_screen().  So, instead of computing this maxchunk in
> > three different places, just compute it when it's needed.  The call of
> > current_is_below_screen() in do_right()... it should be possible to do
> > that in a cheaper way.
> 
> I actually was waffling over how to do that, and went with maxchunk
> because I figured if you objected to maxlines-like logic, you would have
> removed it yourself already.

No.  Something stlll being there doesn't mean that I like it.
It just means that I don't know how to get rid of it.  There
is a comment somewhere in one of the now-closed softwrap bugs
on Savannah where I protest against the maxlines mechanism
(then still maxrows).  And in debugging output I've often seen
compute_maxlines() being called twice in a row, completely
unnecessarily.  So I hated that function, but never got around
to stamping it out.

> I did have an alternate
> solution in mind that wouldn't require maxchunk or related bits at all;
> I'll go with that in the updated version.

Okay.

> However, this leads to one more potential comment fix: get_page_start()
> refers to nano's scrolling "horizontally within a line in chunks", but
> obviously not in the same kind of chunks as in softwrap mode.  What
> terminology would you use for that instead?

Hm.  Good one.  :)  Well, they are chunks too, just slightly
smaller ones, because the "pages" overlap by six characters
(eight, if the dollar signs weren't getting displayed).  So
I would add to the "horizontally within a line in chunks":
"(a bit smaller than the chunks used in softwrapping)".

> Would you object to the softwrap-specific version's still being called
> update_softwrapped_line(), or not?

Sounds good.

> > Superfluous variable: line.  And line_x isn't an x, it's a column
> > position -- it would be better called leftedge or something.  (Yes,
> > mouse_x should have been called mouse_col too.)
> 
> This is getting muddled.  It's line_x because it's the starting
> x-coordinate of the current chunk; if it were the column position of
> that chunk, then the overhauled do_up() and do_down() wouldn't have to
> call xplustabs() on its value to get the column position when adjusting
> for placewewant.  In other words, current[line_x] should point to the
> character at line_x, which it *does* in the overhaul.

But somewhere you add line_x to mouse_x, and mouse_x is a
column position:

            openfile->current_x = actual_x(openfile->current->data,
                                        line_x + mouse_x);

> Your pseudocode version isn't quite right.  It doesn't properly
> duplicate the smart home behavior in non-softwrap mode, before or after
> the overhaul,

Okay.

> and it doesn't account for smart home in softwrap mode
> when the indent is long enough to take up more than one chunk.

Hmmm...

> Softwrap-mode smart home:
> 
>      get indent;
>      if (softwrap) {
>          get leftedge_x;
>          if (leftedge_x < indent)
>              x = indent;
>          else
>              x = leftedge_x;
>      }

I think the second if should be:

          if (x < indent)

> Merging all these cases into one function is the tricky part.

True.  It is more complicated than I at first thought.

> > Why not:
> >
> > +    if ((xplustabs() % editwincols) == (editwincols - 1))
> 
> I think that should work, yes.

Well... when using simple characters, yes.  But when there are
two-column characters involved...  Problem.  The same problem
as in bug #49440 (https://savannah.gnu.org/bugs/?49440).

Benno

-- 
http://www.fastmail.com - Access your email from home and the web