Re: [Nano-devel] [PATCH] Implement quick switch between open buffers

[Benno Schulenberg]

Op 17-12-17 om 05:33 schreef Marco Diego Aurélio Mesquita:
> The attached patch implements quick switching between open buffers in nano.
>
> Differences from previous version are:
>    - removed some changes in indentation and
>    - the option is now on the "goto" menu.
>
> Please review it. I'll sign it as soon as it gets reviewed.

Sorry, Marco, I have no interest in this feature.  And I have not seen
anyone else wanting it and applauding it.  But I do wonder: why do you
need it?  What is the use case?  And what number of files do you intend
to have open and want to switch criss-cross between?

Anyway, a few comments on the patch itself follow below.

+static bool file_mode = TRUE;
+       /* Whether we are listing files or open buffers. */

It is totally unclear what TRUE and FALSE mean here.  I would suggest
to use an enum type, consisting of the values DIRECTORY and BUFFERS,
so that it is far clearer what a check of listing_mode is doing.

+/* Fills filelist with a list current open files.
+ * Also sets longest and width. */
+void list_current_files()

I've said it before: don't just drop your functions somewhere, but put
them in a logical place.  This one belongs right after its brother,
read_the_list().

+ * Also sets longest and width. */

Don't blindly copy comments.  When you must add comments, make sure
they are accurate and tell us something new.

+    /* If needed, make room for ".. (parent dir)". */

This comment is simply wrong.  It was blindly copied.  When you copy
code, leave out all comments: there is no point in repeating the same
comments for the same piece of code.  And worse: the comments will be
wrong when the same code actually does something else.

In general: don't speak of "open files".  It sounds too much like
"To Files".  Speak of "current buffers" instead.  And instead of
"To Open", say something like "List Buffers".  I would use ^B for
that instead of ^O, which is hard to type because it feels like
writing out a file.

+    char *filename
+    int count = 0;
+    filelist_len = 0;
+    longest = 0;
+
+    openfilestruct *first = openfile;
+    openfilestruct *current = openfile;

Please don't mix declarations and plain assignments.  Put all
declarations in a single block at the beginning (including their
initializations), then the other statements.  So the above should
beL

    char *filename
    int count = 0;
    openfilestruct *first = openfile, *current = openfile;

    filelist_len = 0;
    longest = 0;

But looking closer, you don't need filename in this scope; you
need it only in one of the loops; so, declare it there.

+char *do_browse_open(void)
+{
+    char *ret;
+    file_mode = FALSE;

Use a blank line between declarations and other code.  And this
function needs an overall comment.

Benno