Re: Backward search in internal viewer

[Pavel Roskin <proski gnu org>]

Hi, Andrew!

> This patch adds backward search feature for normal search
> in internal viewer. This one is for non gnome edition, sorry.

Thanks for trying, but an implementation for GNOME would be nice too. I
understand that you cannot use input_dialog(), so this means writing a
real GNOME dialog. You can find some examples in gdialogs.c.

Another problem is that your patch doesn't implement backward search in
the Hex mode and for regular expressions. This would make the behavior of
the viewer inconsistent. From the user's point of view, backward search
makes sence in all of those modes.

> I want to receive some comments about this patch.

I'm sorry, but life is not so simple. You can simplify it though :-)

> TODO: As far as I know get_line_at () is needed only for regexp search.
> do_normal_search should be rewritten without allocating memory for string.

It doesn't look like that. See in search():

s = get_line_at (view, &p, &t);
...
search_status = (*search) (view, text, s + 1, match_normal);

Anyway, it's a minor technical detail, I think.

> BTW, I can't understand, who need MenuBarEmpty structure in main.c ?

Looks like a good catch! Should be safe to remove. It's prehistoric (i.e.
exists since the revision 1.1) and is not documented in ChangeLog.

Now let's see your patch.

> +#else
>      if ((q = _icase_search (text, data, &lng)) != 0) {
> -	view->found_len = lng;
>  	view->search_start = q - data - lng;
> +#endif
> +	view->found_len = lng;
>  	return 1;
>      }

ifdef across if looks weird. I wouldn't mind having two separate versions
of the _whole_ if statement, even if it means duplicating two or three
lines.

> -	p = (found_len ? search_start : view->last) - 1;
> +	p = found_len ? (search_start ? search_start - 1 : 0) : search_start;

Not clear. Why aren't you using view->last anymore? I understand it
affects GNOME too.

> -	    t += reverse_line_start ? reverse_line_start + 3 : 0;
> +	    t = reverse_line_start ? reverse_line_start + 2 : 0;

Same thing. Call me stupid, but I don't understand it.

> +    QuickWidget quick_widgets[] = {

If you don't make it static the whole structure will be initialized every
time. Not good.

-- 
Regards,
Pavel Roskin