Re: [Nano-devel] [PATCH] Implement incremental search v4

[Marco Diego Aurélio Mesquita]

On Wed, Jan 31, 2018 at 2:28 PM, Benno Schulenberg <address@hidden> wrote:
>
> Op 29-01-18 om 00:57 schreef Marco Diego Aurélio Mesquita:
>
>> The attached implements incremental search in nano. This version
>> implements the behaviour that was asked previously. It is still not
>> intended to be commit as is, since the code can still be improved.
>>
>> Please test it and tell me if the behaviour is good.
>
>
> I've run a quick test and found two things:
>
> 1) Run src/nano +1 NEWS, and type: ^W drastic <Enter>.  Then press
> M-W.  It says it's the only occurrence.  Move the cursor one line
> down and press M-W again.  Now it finds the second occurrence.
>

I'll investigate what causes this. Any hint?

> 2) Run src/nano +1 NEWS, and type: ^W debug <Enter>.  Note that the
> cursor sits on the first letter of "debug".  Now press M-\ and then
> type an incremental search: ^W M-I debug <Enter>.  Oops.  Instead of
> leaving the cursor of "debug" it returns the cursor to where the
> search started.  After a bit of trying, it resulted that one has to
> press Cancel (^C) to get the cursor to stay on the word "debug".
> This is the wrong way around: pressing <Enter> should leave the
> cursor where it is at that moment, and ^C should return it where the
> incremental search started.  In this way, a search (^W) works the
> same way whether or not Incremental has been switched on.
>

I'm glad you think so. The behavior I implemented is what I thought
you had asked for when you said that pressing enter in an incremental
search should do nothing. I'll fix it.

> A few comments on some details of the patch.
>
> +/* Searches with wrap around.
> + * Return true if found, false otherwise.
> + * Sets parameter 'len' to number of characters of the search */
> +bool wrap_search (char *needle)
>
> The naming of the function is poor -- it doesn't have anything to do
> with wraspping (line wrapping, soft wrapping).  All searches in nano
> wrap around, there is no need to mention this in the name of the
> function.  Also, "Sets parameter 'len'..."?  Which parameter?  When
> you add comments, make sure they are accurate.
>

Any suggestion on how it should be called? The parameter was removed
in some revision of the patch and I forgot to update it. I'll fix the
comment.

> +/* Callback for incremental search. */
> +void inc_search_cb(char *answer)
>
> Another poor naming.  Name the function after what it does, in this
> case: advance_to_first_occurrence().  Or find a shorter equivalent.
>

I'll do it.

> +   {"incremental", INCR_SEARCH},
>
> No need to shorten the name of the constant -- look at its neighbors.
> Use INCREMENTAL_SEARCH in full instead.  And sort the option to its
> alphabetical position, as that is the order of the list.
>

I'll do it.

I sent the patch just to gather opinions about the behavior. But thank
you very much for taking the time to review it.