Re: [Nano-devel] New Feature: completion from external dictionary

[Marco Diego Aurélio Mesquita]

On Wed, Jul 11, 2018 at 7:52 AM, Benno Schulenberg <address@hidden> wrote:
>
> Op 09-07-18 om 21:45 schreef Marco Diego Aurélio Mesquita:
>> +             while (sint->dict != NULL) {
>> +                     struct filestruct *item = sint->dict;
>> +                     sint->dict = sint->dict->next;
>> +                     free(item);
>> +             }
>
> You could have dropped this.  It is not needed to make things work.
>

Kept for now. Will remove if you ask me to.

>> +struct filestruct;
>> +     /* Foreward declaration of filestruct. */
>
> Okay for now, but better send a separate patch first to reorder the
> declarations.
>

Kept for now. Will move it after a review confirming that other items are OK.

>> +     char *completer;
>> +             /* The path to the completer dicitonary to this type of file. 
>> */
>> +     struct filestruct *dict;
>> +             /* The dicitonary cache to this type of file. */
>
> This is confusing.  'completer' is the path to a dictionary, so I would
> call that 'dictionary'.  And 'dict' is this dictionary read into memory,
> providing a long list of possible completions, so I would call it...
> 'completions', or maybe 'list_of_words'.
>

Done.

> Also, putting them together is confusing.  The things at the start of
> the syntaxtype are things that the user specified explicitly.  At the
> end of the syntaxtype follow two things that nano has determined.  So,
> put this 'list_of_words' after 'nmultis' and before '*next'.
>

Done.

>> +bool is_good_file(char *file);
>
> If you put the reading of the dictionary into files.c, you don't
> need this declaration.
>

This one is a bit confusing. The function is is_good_file is declared
in rcfile.c. I broke get_dict in two functions: get_dictionary and
load_dictionary. The load_dictionary is the one which actually opens
and loads a dictionary, so I declared it in files.c. The
get_dictionary function is declared in rcfile.c. Declaring
is_good_file in proto.h is no longer needed, but now we have to
declare both get_dictionary and load_dictionary in proto.h.

>>       live_syntax->headers = NULL;
>>       live_syntax->magics = NULL;
>>       live_syntax->linter = NULL;
>> +     live_syntax->completer = NULL;
>> +     live_syntax->dict = NULL;
>>       live_syntax->formatter = NULL;
>
> Again, separate what is explicitly specified from what comes about
> as a result.
>

Done.

>> +filestruct *get_dict(char *shard)
>> +{
>> +     char *file = openfile->syntax->completer;
>
> Superfluous alias.
>

Removed.

>> +     filestruct *lines = openfile->syntax->dict, *tmpline = NULL, *curline 
>> = NULL;
>
> Too many declarations on one line.  Split into two.
>

Improved.

>> +     if (lines)
>> +             goto end;
>> +
>> +     statusbar(_("Loading dictionary, please wait"));
>
> Don't print this message before you're actually going to read a file.
>

Done.

>> +     /* Don't open directories, character files, or block files. */
>> +     if (!is_good_file(file))
>> +             goto end;
>> +
>> +     /* Open the included syntax file. */
>> +     rcstream = fopen(file, "rb");
>
> Move the reading of the file to files.c.
>

Done.

>> +     char *buf = NULL;
>> +     ssize_t len;
>> +     size_t n = 0;
>> +     size_t lineno = 0;
>
> Then these declarations won't sit somewhere in the middle but
> at the start of a function, as they should.
>

Done.

>> +     while ((len = getline(&buf, &n, rcstream)) > 0) {
>> +             tmpline = curline;
>> +             curline = nmalloc(sizeof(filestruct));
>> +             curline->next = NULL;
>> +             curline->prev = tmpline;
>> +             if (curline->prev != NULL)
>> +                     curline->prev->next = curline;
>> +             curline->data = mallocstrcpy(NULL, buf);
>> +             curline->lineno = ++lineno;
>> +             curline->multidata = NULL;
>
> Ow, what a waste...  You use an entire linestruct thing just to
> store a single word from the dictionary?  :|
>

The way it is implemented right now allow us to have multiple words per line.

> Anyway, okay for now.  But I don't think you need to set
> multidata nor prev nor even lineno, as the completion code
> doesn't use those fields.
>
> Also, don't call this curline and tmpline -- they are not lines
> but mere words.  Better use 'current_word' and 'new_word', or
> something like that.
>

Done.

>> +     if (pletion_line == openfile->fileage) {
>> +             pletion_line = get_dict(shard);
>> +             pletion_line = pletion_line == NULL ? openfile->fileage : 
>> pletion_line;
>
> This looks weird.  Better use a temporary variable, and only change
> pletion_line when get_dictionary() doesn't return NULL.
>

Done.

> By the way, why do you pass 'shard'?  It is not used anywhere in
> that function.
>

It is a leftover from a previous implementation where the dictionary
was provided by the output of a command. Removed.

> But... this means that when a syntax specifies a dictionary, then nano
> will only find possible completions in that dictionary and no longer
> in the current file?
>

Yes.

> Oh, and please always send a complete patch, with commit message and
> relevant explanations and signed-off line.
>

Done.

Thanks for the review.

Can you check the attached updated version?

0001-Implement-completion-from-external-dictionary.patch
Description: Text Data