Re: [Nano-devel] File insertion revamp

[David Ramsey]

On Sun, Feb 19, 2017 at 5:29 AM, Benno Schulenberg
<address@hidden> wrote:

> 0001-0005: Already applied and pushed.  Slightly modified here and
> there.  The comments in 0002 didn't match the code any more

Okay.  Thanks.

> Better drop the bool 'another_line'.  Just do a continue
> in the else, instead of setting another_line to FALSE.  It
> saves an if, and an indentation step in the rest.

Done locally.

> Better move this to to right after:
>
> +           bottomline->data = read_line(buf, len);
>
> and update the preceding comment accordingly: /* Store the data and
> make a new line. */

Done locally.

> +    else {
> +       bool force_another_line = FALSE;
>
> Don't declare a variable in the middle of a function.

Not done yet (although I've locally renamed force_another_line to
mac_line_needs_newline, which should make what it's used for clearer),
because I don't understand what you mean here.

Are you objecting to declaring a variable at the beginning of an else
block instead of globally?  Because that's another thing that's done all
over the code elsewhere.  Also, the variable is only used in the scope
of that else block, so I figured declaring it globally was overkill.

Besides, something like it is needed to fix possibility 2a below, and
duplicating code to do the same thing seemed much worse.

> +           /* If it's a DOS or Mac line, strip a '\r' from it. */
>
> The second occurrence of this comment is out of place.
> Leave the if part out of it.  Just say: /* Strip the '\r'. */

Done locally.

> I'm not sure that the whole routine doesn't create a node too many
> when NONEWLINES is set.  Haven't tested it much.

It shouldn't.  If I got the code right, the logic is as follows:

Possibility 1: The last line of the file is "some words here\n".  The
line gets read up to the newline and processed into a buffer line.  The
end of the file is read, and, since everything in the file was read, a
buffer line containing a blank string (what the buffer sees as a
newline) is added to the end.  Since the file ends in a newline, the
magicline code later is unused.  (Correct.)

Possibility 2: The last line of the file is "some words here" (note no
newline).  The line gets read up to the end of the file.  Since
everything in the file was not read, the line is processed into a buffer
line afterwards, and *no* buffer line containing a blank string is added
to the end.  Since the file doesn't end in a newline, the magicline code
later will add one only if NONEWLINES isn't set.  (Correct.)

The problem I was trying to fix was a version of possibility 2 that the
old version of the revamp didn't account for (I'll call it 2a).

Possibility 2a before the new revamp: The last line of the file is "some
words here\r".  The line gets read up to the end of the file, and marked
as a Mac format line.  Since everything in the file was not read, the
line has its '\r' stripped and is processed into a buffer line
afterwards, and *no* buffer line containing a blank string is added to
the end.  Since the file doesn't end in a newline, the magicline code
later will add one only if NONEWLINES isn't set.  (Incorrect, since '\r'
is a Mac newline and should have been treated as such.)

Possibility 2a after the new revamp: The last line of the file is "some
words here\r".  The line gets read up to the end of the file, and marked
as a Mac format line.  Since everything in the file was not read, the
line has its '\r' stripped and is processed into a buffer line
afterwards, and (since the part that stripped the '\r' flagged the line)
a buffer line containing a blank string is added to the end when it
otherwise wouldn't be.  Since the file ends in a newline, the magicline
code later is unused.  (Correct, since '\r' is a Mac newline and is
treated as such.)

> Declarations should be a single block; no blank lines.

Okay.

(As an aside, it's not always obvious what your preferred coding style
is.  Do you have any plans to add a coding style document or something
similar?  No offense, but... I'd prefer to spend more time working on
code and less time working on style nits, and that would make it much
easier than having to do it piece by piece this way.)

> Also, do the latter ones need to be set to NULL?

If you look at extract_buffer() (nano.c:line 321), it will move the text
covered by the coordinates into the buffer only if the buffer is empty
(that is, if it's NULL).  Otherwise, it'll tack the text onto the end of
the text already in the buffer.  (The cutting code uses that
functionality.)

If I don't set these to NULL and leave them uninitialized, the attempt
to tack text onto the end of their nonexistent text will segfault.

> +    extract_buffer(&discard_top, &discard_bot, top, top_x, bot, bot_x);
> +    free_filestruct(discard_top);
>
> It seems that the second parameter of extract_buffer() is seldom used.
> Maybe drop it?  And only go find the last line of the buffer when it
> is needed?  This is probably best done as a follow-up patch.

I'll look into that soon, but I'd much prefer to get back to making the
softwrap code work after this.

> The topline of the buffer to be discarded could then simply be called
> 'trash'.

Considering that extract_buffer() is also used to cut text, which (a) is
not necessarily trashing it if we want to paste it, and (b) involves
tacking the cut text onto the previous cut text if applicable, as
mentioned above, that seems like a major misnomer to me.