From: Omar Polo <op@omarpolo.com>
Subject: Re: search issues in tog after cursor movement
To: Mikhail <mp39590@gmail.com>
Cc: gameoftrees@openbsd.org
Date: Sat, 18 Jun 2022 18:41:31 +0200

Mikhail <mp39590@gmail.com> wrote:
> While studying tog code, I found that search behaves a little bit
> strange if you move cursor after you hit the match.
> 
> To illustrate this open tog in got repository and search for 'foo'
> (there are 3 matches), after you hit the last one, hit Home and then
> 'n', it will say 'no more matches'.
> 
> I think it's not what is expected by a user, at least mutt and editors
> don't behave that way.
> 
> Can you assess the following patch?

it took me a while to realize the change, but it's really cool, thanks!

continuing the search from the current "cursor" position is intuitive
and also what most (all?) users expect, so fwiw i'm ok with the diff.

one minor nitpick below

> diff refs/heads/main refs/heads/search
> blob - 8782d2289745425a1fdaecbd1edbcab6c230e7e6
> blob + a23ffa80af77e01eae15224e4b699b0e6e8a3020
> --- tog/tog.c
> +++ tog/tog.c
> @@ -2342,11 +2342,25 @@ search_next_log_view(struct tog_view *view)
>  			entry = TAILQ_PREV(s->search_entry,
>  			    commit_queue_head, entry);
>  	} else if (s->matched_entry) {
> +		int matched_idx = s->matched_entry->idx;
> +		int selected_idx = s->selected_entry->idx;
> +
> +		/*
> +		 * If user has moved cursor after we hit the match, position
> +		 * from where we should continue search must be changed.
> +		 */
>  		if (view->searching == TOG_SEARCH_FORWARD)
> -			entry = TAILQ_NEXT(s->matched_entry, entry);
> +			if (matched_idx > selected_idx)
> +				entry = TAILQ_NEXT(s->selected_entry, entry);
> +			else
> +				entry = TAILQ_NEXT(s->matched_entry, entry);

please, wrap nested ifs like these in braces.

		if (view->searching == TOG_SEARCH_FORWARD) {
			if (...)
				...
			else
				...
		} else {
			...
		}

while you're code is correct and even without indentation there aren't
other ways to interpret it, the "dangling else" can be confusing :)

>  		else
> -			entry = TAILQ_PREV(s->matched_entry,
> -			    commit_queue_head, entry);
> +			if (matched_idx < selected_idx)
> +				entry = TAILQ_PREV(s->selected_entry,
> +						commit_queue_head, entry);
> +			else
> +				entry = TAILQ_PREV(s->matched_entry,
> +						commit_queue_head, entry);
>  	} else {
>  		entry = s->selected_entry;
>  	}