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

[Benno Schulenberg]

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.

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.

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.

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

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

Benno