Re: [Nano-devel] File insertion revamp

[Benno Schulenberg]

On Mon, Feb 13, 2017, at 21:18, David Ramsey wrote:
> - revamps read_file() and related bits to insert a file into the current
> buffer, much like do_uncut_text() does (so that it also counts inserted
> lines properly when determining whether it needs to do a focused
> refresh); in the process, it separates a new function
> move_from_filestruct() from copy_from_filestruct(), as the latter would
> unnecessarily duplicate the file in memory

0004:

> Rename copy_from_filestruct() to move_from_filestruct(), and make it
> work on a filestruct instead of a copy of that filestruct.
> 
> Accordingly, make a new copy_from_filestruct() that copies its
> filestruct and then calls move_to_filestruct() on the copy.

This description makes me dizzy.  :)  And I /still/ don't get it.

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.

-    /* Put the top and bottom of the current filestruct at the top and
-     * bottom of a copy of the passed buffer. */
+    /* Put the top and bottom of the current filestruct at the top and bottom
+     * of the passed buffer. */

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.)

0005:

> Note that all this makes read_file() depend on the position of
> current[current_x] to know where to insert the file.  replace_buffer()
> reinitializes the buffer to empty before it calls read_file(), but
> leaves current_x set to its old out-of-range value, which leads to a
> segfault later in move_from_filestruct().  Set current_x to zero in
> replace_buffer() to avoid this.

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

+    ssize_t was_lineno = openfile->current->lineno;
+

Needs a comment.

+    filestruct *fileptr_top;
+       /* The top of the buffer where we store the complete file. */
+    filestruct *fileptr_bot;
+       /* The bottom of the buffer where we store the complete file. *

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.

+       if (got_line) {
+           num_lines++;
+           fileptr_bot->next = make_new_node(fileptr_bot);
+           fileptr_bot = fileptr_bot->next;
+           got_line = FALSE;
+       }

Setting got_line to FALSE is superfluous here.

Hmm...  I would move the assignment to data here too.
And maybe invert the logic, letting got_line default to
TRUE and only setting it to FALSE in the third alternative.
I would call it another_line.

+    /* If we did get a newline, put it in properly. */
+    } else
+       fileptr_bot->data = mallocstrcpy(NULL, "");

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

0006:

Great!  Lots of deletions!  :)

0007:

Make a little file, consisting of two lines:

aaa
bbb

The latter line should not end with a newline.  Then run
    src/nano +9,29 NEWS
and type: ^R littlefile <Enter>
Okay,  aaa and bbb get inserted.
Now type M-U.
The bbb remain.

Attached patch fixes that.

0008:

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().
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.)

> This also fixes cursor position adjustment when the marked text changes
> from being spell-checked.

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

+    /* Replace the text of the current buffer with the spell-checked text. */
+    } else
+#endif
+       replace_buffer(tempfile_name);

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.

-/* Open the specified file, and if that succeeds, blow away the text of
- * the current buffer and read the file contents into its place. */
+/* Open the specified file, and if that succeeds, blow away the text of the
+ * current buffer and read the file contents into its place. */

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

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.

Benno

-- 
http://www.fastmail.com - mmm... Fastmail...

adjust-making-an-undo-record-for-an-insertion.patch
Description: Text Data