Re: [Nano-devel] File insertion revamp

[David Ramsey]

It seems a lot of the issues are due to my modeling the new code after
existing code, and the existing code is in a few different styles.  On
to the reply.

On Tue, Feb 14, 2017 at 2:52 PM, Benno Schulenberg
<address@hidden> wrote:
> This description makes me dizzy.  :) And I /still/ don't get it.

The only functions we have in the current code are for moving to a
buffer and copying from a buffer.  Inserting a file is really just
reading a file off disk into its own buffer, and then merging that
buffer into the current file's buffer.

The closest function for inserting a file is be the one for copying from
a buffer, but copying a buffer is unnecessary if we're merging that
buffer.  So I needed to be able to move from a buffer in that case.  And
the only difference between moving from a buffer and copying from a
buffer... the former moves the original, while the latter makes a copy
of the original and moves the copy.

Is that clearer?

> First, when you rename things, let's rename things properly. A
> "filestruct" is something that does not exist.  What it refers to is
> in fact a buffer.  So let's call those routines "move_to_buffer" and
> "copy_from_buffer".  (Don't rename anything else -- only rename the
> things that you change.)  A buffer is indentified by a pointer to its
> first line.  Later on we'll rename "filestruct" to "linestruct",
> because that is what it is.

Okay.  Although I figure splitting the two functions and renaming them
should be separate commits then?

> Better not change the first line; then it's clearer what you change.
> (Using the full 80 characters only matters when it will /fit/ in 80
> characters.  If more lines are needed, it is okay to wrap at a
> convenient place.)

Okay.

> Better move the setting of current_x from make_new_buffer() to
> initialize_buffer_text().  (Don't move anything else.)

Okay.

> Needs a comment.

Didn't put a comment there originally because that bit is taken straight
from do_uncut_text(), which doesn't have a comment there either.  I
guess they both need comments then?

> Better call them topline and bottomline, or something like that,
> because they are not file pointers -- they don't point to files, they
> point to lines.

Okay.

> Setting got_line to FALSE is superfluous here.

Okay.

> Hmm...  I would move the assignment to data here too.

Okay.

> And maybe invert the logic, letting got_line default to TRUE and only
> setting it to FALSE in the third alternative.

Okay.

> I would call it another_line.

Okay.

> Better invert the if so it is more obvious that the if has an else.
> Also, make the preceding comment cover both cases.

Okay.

> Attached patch fixes that.

Thanks.  Although, since it doesn't have a git commit message, do I keep
it as a separate patch and write one (which seems wrong since I can't
quite imitate your writing style yet), or do I meld it with the other
undo fixes in the existing patch (which seems wrong since I didn't write
it)?

> Hm.  Some of the comments in replace_marked_buffer() make it sound as
> if I wrote it.  :) Better not do that -- just leave out those
> comments, as they just repeat those from replace_buffer().

So remove the comments that duplicate the ones in replace_buffer(), but
leave the rest?

> Also leave out the assert -- the filename cannot be NULL and cannot be
> of length zero, because it that case it would never have got here.
> (If it's a function that gets called from /many/ places, an assert
> might be a good check, but for one or two calls, it just gets in the
> way.)

Okay.

> Was there a case where this went wrong?  Where the cursor position
> would subtly change after a spell check?

Actually, yes.  (For the record, I'm using "aspell -c" as my alternative
spell checker.)

With current git, forwards: Open files.c; move to line 8, column 10;
turn the mark on; move forward to line 8, column 14 (so the word "nano"
is covered by the mark), and then press ^T to start the alternative
spell checker.  When it triggers on "nano", press "r" to replace,
replace "nano" with "nanooooo", press "i" to ignore the new word, and
after the spell checker finishes, the cursor will be on the space before
"nanooooo", with the mark starting on and covering only the position the
cursor is on.

With current git, backwards: Open files.c; move to line 8, column 14;
turn the mark on; move backward to line 8, column 14 (so the word "nano"
is covered by the mark), and then press ^T to start the alternative
spell checker.  When it triggers on "nano", press "r" to replace,
replace "nano" with "nanooooo", press "i" to ignore the new word, and
after the spell checker finishes, the cursor will be on the space where
"nanooooo" starts, with the mark starting on the space before "nanooooo"
and covering only the space before the position the cursor is on.

With this patch applied, forwards: After the spell checker finishes, the
cursor will be on the space after the first "o" of "nanooooo", with the
mark starting on the first "n" of "nanooooo" and ending on the first "o"
of "nanooooo": the same as before, not counting the shift.

With this patch applied, backwards: After the spell checker finishes, the
cursor will be on the space after the first "o" of "nanooooo", with the
mark starting on the first "o" of "nanooooo" and ending on the first "n"
of "nanooooo": the same as before, not counting the shift.

Didn't notice this until I was testing marked spell check to verify that
it worked, or I would have brought it up earlier.  I guess a proper bug
report is in order, then?

> Fuse that comment with the one before the if -- that is: make the
> comment before the if describe both cases.  The cases are nearly
> identical so there is no need to repeat most of the text.

Okay.

> Again, don't rewrap any text that doesn't need any changing.

Okay.

I suppose if I go through a function in general,

> In general: the first line of a commit message should start with a
> short tag, lowercase, and the line should not exceed 75 chars, and
> should not end in a period.  It should fit in when looking at:
>    http://git.savannah.gnu.org/cgit/nano.git/log/
>
> The rest of the commit message is normal text, normal sentences:
> capitalization and periods.

Okay.