Bug 2951

Created attachment 6050 [details] [review]
Add case_sensitive parameter to poppler_page_find_text

This patch adds a case_sensitive parameter to the poppler_page_find_text
function of the glib interface, and connects it up to the approriate lower
level module.

Be aware that this patch changes API (and ABI) of -glib library, so it should not be merged without considering impact it will have.

This appears to work correctly for the Qt4 front end, so I'm re-assigning it to the glib front end (rather than as a general poppler issue).

I don't know why this function eliminate all the options provided by the textFind() function.

With bear minimum, I cannot search another word on the same page without reload the page because the startAtLast is hardwired to True.

At lease provide a find_text_full() function if you don't want to break the compatibility with older versions.

Do you know a workaround maybe so that the same word could be searched again nore that once (without reloading the whole document)? This had been working with 0.8.7, I have noticed that bug recently using 0.12.3.

Created attachment 55270 [details] [review]
a patch to add case sensitive search to poppler glib

this patch adds the method poppler_page_find_text_full to popplers glib frontend.
it is necessary to enable case sensitive search in evince

Comment on attachment 55270 [details] [review]
a patch to add case sensitive search to poppler glib

Review of attachment 55270 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch!, it looks good to me in general, but I have a few comments. You also need to update /glib/reference/poppler-sections.txt adding the new symbols there. It would also be great if you add find options to the find demo (glib/demo/find.c), but that might be a following patch.

Thanks!

::: glib/poppler-page.cc
@@ +853,4 @@
>  }
>  
>  /**
> + * poppler_page_find_text_full:

I would use poppler_page_find_text_with_options() for consistency with poppler_page_render_for_printing_with_options() instead of _full(). Also, instead of using boolean parameters I would use a single parameter using flags something like:

GList *poppler_find_text_with_options (PopplerPage *page, const gchar *text, PopplerFindOptions options);

That way if need to add new options in the future we don't need to break the API, and we don't need to remember the position of every parameter.

@@ +860,5 @@
> + * immediately after the last find result; else starts looking at
> + * <xMin>,<yMin>.  If <stopAtBottom> is true, stops looking at the
> + * bottom of the page; else if <stopAtLast> is true, stops looking
> + * just before the last find result; else stops looking at
> + * <xMax>,<yMax>.

The body goes after the parameters not before. Parameters are referenced using @ not <>. In this case, document every option in the PopplerFindOptions enum declaration, and here say that it finds text with the given options.

@@ +873,1 @@
>   * Return value: (element-type PopplerRectangle) (transfer full): a #GList of #PopplerRectangle,

Add * Since: 0.20 since this will be new API added in poppler 0.20

@@ +940,5 @@
> +                            TRUE,   // stop_at_bottom
> +                            FALSE,  // start_at_last
> +                            FALSE,  // stop_at_last
> +                            FALSE,  // case_sensitive
> +                            FALSE); // backward

You could add an option POPPLER_FIND_DEFAULT to the enum containing the default options and use it here.

thanks for your comments. i have added another patch, considering yout recommendations.

i have not added a description of POPPLER_FIND_WHOLE_WORD since im not sure of its meaning.

Created attachment 58708 [details] [review]
add case sensitive search

Comment on attachment 58708 [details] [review]
add case sensitive search

Review of attachment 58708 [details] [review]:
-----------------------------------------------------------------

::: glib/poppler.h
@@ +180,5 @@
> +  POPPLER_FIND_BACKWARDS         = 1 << 5,
> +  POPPLER_FIND_WHOLE_WORD        = 1 << 6,
> +  POPPLER_FIND_DEFAULT           = POPPLER_FIND_STOP_AT_BOTTOM
> +} PopplerFindFlags;
> +

Exposing all the findText options does not make sense. They are part of the internal API which may change. They are used for implementing features like find first/find next. As the poppler_page_find_text_with_options returns all matches on the page it does not make sense to have options for start from top or backwards search. These options also will not work as you would expect with poppler_page_find_text since it runs in a loop with the same options but reusing the xMin, yMin from the previous call to findText. Normally you would have one set of options for the first call to findText then change the options for subsequent calls. eg the first call might use startAtTop=true, startAtLast=false then the next call will use startAtTop=false, startAtLast=true.

Also the 'whole word' option is not implemented.

I've implemented the whole words only options, and pushed a slightly modified version of the patch  with only case-sensitive, backwards and whole-words-only options. 

Thanks!