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

[Marco Diego Aurélio Mesquita]

On Sun, Dec 17, 2017 at 5:28 PM, Benno Schulenberg <address@hidden> wrote:
> 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?
>

I use nano to edit its own source code. Usually, I just type
"./src/nano ./src*.[ch]". Linearly iterating through these files is
not as good as choosing from a list.

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

Done.

> +/* 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.
>

The function was moved. Comments were removed or changed.

There's some code duplication between read_the_list() and
list_current_files() (now called list_current_buffers). What about a
function to adequately set 'longest' and 'width' that is called by
both?

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

The shortcut "^B" can not be used because it is tied to backward the
prompt's cursor. Chose "^R" for now. Do you have  better suggestion?

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

Done.

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

Done.

Any other change?

quick_switch.diff
Description: Text document