can confirm this, but evince is actually able to select and copy text, but the normal selection rectangle is not displayed. infact, if you mimic the selection gesture on a word, right-click -> copy and paste it somewhere, it is actually pasted! i'll investigate on this.

Text extraction is implemented, but not selection rendering. Confirmed. 

Still here in Evince 2.29.2 (git master). 

Updating version number.

Still in 2.32.0 (though I cannot update the version number).

*** Bug 691172 has been marked as a duplicate of this bug. ***

Created attachment 245118 [details] [review]
Implements hightlighing in djvu text selection

Hi,

This patch implements highlighting when selecting text in djvu documents.
It implements the interface functions:
    iface->render_selection
and
    iface->get_selection_region

What do you think of the approach?

Thank you
Jonas

Review of attachment 245118 [details] [review]:

Looks pretty good in general, thanks a lot.

::: backend/djvu/djvu-document.c
@@ +609,3 @@
+		r->y2 = height - tmp   * 72.0 / dpi;
+
+		cairo_set_source_rgba (cr, base->red, base->green, base->blue,

You can't use the given color components directly, GdkColor uses values between 0 and 65535, while cairo uses double values between 0 and 1, so they should be converted by dividing by 65535.

@@ +615,3 @@
+		cairo_set_line_width (cr, 0.5);
+		cairo_set_source_rgb (cr, base->red, base->green, base->blue);
+		cairo_stroke (cr);

Instead of doing this, similar to what we do for search highlights, I think it might look better if we render the selection on top of the page using the multiply operator. For that we would need to modify the selection interface to receive the page surface too, other backends can ignore the page surface. Here we could paint the page surface into the selection surface and then the selection rectangles on top using the multiply operator, and rectangle + fill.

@@ +673,3 @@
+		r->x2 *= 72.0 / dpi;
+		r->y1  = height - r->y2 * 72.0 / dpi;
+		r->y2  = height - tmp   * 72.0 / dpi;

This is duplicated in several place, we might add a helper function to convert a doc rect to view rect.

Created attachment 245175 [details]
Screenshot

I've made a dirty hack to quickly check how it looks using the multiply blend mode. This is a screenshot showing the current patch (with the colors fixed) and my hack to use multiply blend mode. I'm using 0.5 alpha in both cases, but I think it looks even better using 0.75 for example. It looks quite similar in the end, but with the current patch you can see the selection is actually over the text while with multiply you can see it blended. What do you guys think?

There is one thing with the patch that I don't get the cursor change with djvu whil I do get it with pdf, maybe we need to implement another func?

(In reply to comment #9)
> There is one thing with the patch that I don't get the cursor change with djvu
> whil I do get it with pdf, maybe we need to implement another func?

Yes, I am working on the get_text_mapping implementation as well :)

Thanks a lot for the review and comments!

(In reply to comment #7)
> Review of attachment 245118 [details] [review]:
> 
> Looks pretty good in general, thanks a lot.
> 
> ::: backend/djvu/djvu-document.c
> @@ +609,3 @@
> +        r->y2 = height - tmp   * 72.0 / dpi;
> +
> +        cairo_set_source_rgba (cr, base->red, base->green, base->blue,
> 
> You can't use the given color components directly, GdkColor uses values between
> 0 and 65535, while cairo uses double values between 0 and 1, so they should be
> converted by dividing by 65535.
> 

Oh, thanks!

> @@ +615,3 @@
> +        cairo_set_line_width (cr, 0.5);
> +        cairo_set_source_rgb (cr, base->red, base->green, base->blue);
> +        cairo_stroke (cr);
> 
> Instead of doing this, similar to what we do for search highlights, I think it
> might look better if we render the selection on top of the page using the
> multiply operator. For that we would need to modify the selection interface to
> receive the page surface too, other backends can ignore the page surface. Here
> we could paint the page surface into the selection surface and then the
> selection rectangles on top using the multiply operator, and rectangle + fill.
> 

Thans, that sounds like a better thing to do!

> @@ +673,3 @@
> +        r->x2 *= 72.0 / dpi;
> +        r->y1  = height - r->y2 * 72.0 / dpi;
> +        r->y2  = height - tmp   * 72.0 / dpi;
> 
> This is duplicated in several place, we might add a helper function to convert
> a doc rect to view rect.

Agreed.

(In reply to comment #8)
> Created an attachment (id=245175) [details]
> Screenshot
> 
> I've made a dirty hack to quickly check how it looks using the multiply blend
> mode. This is a screenshot showing the current patch (with the colors fixed)
> and my hack to use multiply blend mode. I'm using 0.5 alpha in both cases, but
> I think it looks even better using 0.75 for example. It looks quite similar in
> the end, but with the current patch you can see the selection is actually over
> the text while with multiply you can see it blended. What do you guys think?

I don't have any strong feelings towards one or the other, I'll try to implement it myself and see it in action.

Thanks!

Created attachment 245216 [details] [review]
Updated after review

This patch implements the changes suggested in review, it also prepares for
implementing get_text_mapping by breaking out selected_region with a helper function.

See patch for get_text_mapping below.

Created attachment 245217 [details] [review]
Implement get_text_mapping

I'm not 100% sure what the other functions in this interface are for.

The get_text and get_text_layout.

What would be the effect of implementing them?

Jonas, I think (not sure) they are needed for accesibility, that is so Orca or another screen reader can read the contents of a djvu file.

Created attachment 245357 [details] [review]
Implement text_get_text

Hi, this implements text_get_text of the text interface.
This allows Orca to read the djvu-document on my machine.

I have not implemented text_get_text_layout, and I'm not sure It's possible with the current backend. But I haven't looked really hard. And I'm also not really sure what it's used for. Tried reading some Atk docs that I found googling, but didn't get wiser.

Anyone knows?

Ah, maybe for reading from where the user presses in the document?

Created attachment 245370 [details] [review]
strdup => g_strdup

Hi, since Jasons patch has been commited (https://bugzilla.gnome.org/show_bug.cgi?id=669022), I've respun these patches do use the fallback method of rendering the selection in the view.

I have three patches.

One that enables selection highlighting by implementing get_selection_region
for djvu.
One that implements text_get_text_mapping. To get the nice mouse pointer effect
when hovering over text. And one that implements text_get_text to enable screen readers to get the content of the text.

Is that configuration ok, or do you want it as one patch, or two? or not at all?

Thanks
Jonas

Created attachment 247087 [details] [review]
Enable selection highlighting

Created attachment 247088 [details] [review]
Implement get_text_mapping

Created attachment 247089 [details] [review]
Implement get_text

Review of attachment 247087 [details] [review]:

Looks great, there are a few minor details, and I have some doubts about the implementation.

::: backend/djvu/djvu-document.c
@@ +550,3 @@
+	rectangle->x2 *= 72.0 * scale / dpi;
+	rectangle->y1 = height * scale - rectangle->y2 * 72.0 * scale / dpi;
+	rectangle->y2 = height * scale - tmp * 72.0 * scale / dpi;

This is confusing, and it's actually my fault because I suggested it. We normally use a GdkRectangle for view rects. But here we still want a EvRectangle, so what we really want is to apply the dpi and invert the Y axis, I thin it's clearer to leave the previous code in djvu_document_find_find_text, since the 3 conversions we need here are actually different to each other. Sorry for the confusion.

@@ +554,3 @@
+
+static GList *
+djvu_selection_get_highlight_rects (DjvuDocument    *djvu_document,

why highlight? djvu_selection_get_selection_rects sounds better to me.

@@ +568,3 @@
+	rectangle.y1 = (height - points->y2) * dpi / 72.0;
+	rectangle.x2 = points->x2 * dpi / 72.0;
+	rectangle.y2 = (height - points->y1) * dpi / 72.0;

This would be done by the helper function I mentioned before.

@@ +578,3 @@
+
+		djvu_text_page_highlight (tpage, page_text, &rectangle);
+		matches  = tpage->results;

Why matches? looks like we are searching something

@@ +597,3 @@
+
+	document_get_page_size (djvu_document, page, &width, &height,
+				&dpi);

This could be a single line, next line is even longer.

@@ +609,3 @@
+		rect.x      = (gint) ((r->x1 * scale) + 0.5);
+		rect.y      = (gint) ((r->y1 * scale) + 0.5);
+		rect.width  = (gint) (((r->x2 - r->x1) * scale) + 0.5);

Don't line up this expressions, please.

@@ +625,3 @@
+				     EvRectangle     *points)
+{
+	DjvuDocument   *djvu_document = DJVU_DOCUMENT (selection);

Extra spaces between DjvuDocument and *djvu_document

@@ +660,3 @@
 {
+	iface->get_selected_text    = djvu_selection_get_selected_text;
+	iface->get_selection_region = djvu_selection_get_selection_region;

Do not line up these either.

::: backend/djvu/djvu-text-page.c
@@ +374,3 @@
 
+void
+djvu_text_page_highlight (DjvuTextPage *page,

This should be someting like djvu_text_page_get_selection, and it could return a GList instead of using results

@@ +388,3 @@
+
+	djvu_text_page_limits (page, page->text_structure, rectangle);
+	djvu_text_page_selection (page, page->text_structure, 0);

We are not interested in the page itself, what we need is a function like djvu_text_page_selection but that only fills the array of DjvuTextLink

@@ +397,3 @@
+		if (!(needle = strchr (haystack, '\n'))) {
+		    needle = strchr (haystack, '\0');
+		    at_end = 1;

Why not iterating the DjvuTextLink array

@@ +407,3 @@
+			result = djvu_text_page_box (page, start, end);
+			page->results = g_list_prepend (page->results, result);
+			next_word_start = end_p + 1;

I think this is based on search implementation where we have a text to find, but here we have coordinates and we want the bounding box the the text in those coordinates, the actual text itself is not important.

(In reply to comment #23)
> Review of attachment 247087 [details] [review]:
> 
> Looks great, there are a few minor details, and I have some doubts about the
> implementation.

Thanks for review! A new patch will be attached below.
I will obsolete the old patches and start a new issue with the text interface.

> 
> ::: backend/djvu/djvu-document.c
> @@ +550,3 @@
> +    rectangle->x2 *= 72.0 * scale / dpi;
> +    rectangle->y1 = height * scale - rectangle->y2 * 72.0 * scale / dpi;
> +    rectangle->y2 = height * scale - tmp * 72.0 * scale / dpi;
> 
> This is confusing, and it's actually my fault because I suggested it. We
> normally use a GdkRectangle for view rects. But here we still want a
> EvRectangle, so what we really want is to apply the dpi and invert the Y axis,
> I thin it's clearer to leave the previous code in djvu_document_find_find_text,
> since the 3 conversions we need here are actually different to each other.
> Sorry for the confusion.

No problem I'm way more confused anyhow. New suggestion and new helper function in patch below.

> 
> @@ +554,3 @@
> +
> +static GList *
> +djvu_selection_get_highlight_rects (DjvuDocument    *djvu_document,
> 
> why highlight? djvu_selection_get_selection_rects sounds better to me.
> 

Agreed.

> @@ +568,3 @@
> +    rectangle.y1 = (height - points->y2) * dpi / 72.0;
> +    rectangle.x2 = points->x2 * dpi / 72.0;
> +    rectangle.y2 = (height - points->y1) * dpi / 72.0;
> 
> This would be done by the helper function I mentioned before.
> 

Sure.

> @@ +578,3 @@
> +
> +        djvu_text_page_highlight (tpage, page_text, &rectangle);
> +        matches  = tpage->results;
> 
> Why matches? looks like we are searching something
> 

Agreed.

> @@ +597,3 @@
> +
> +    document_get_page_size (djvu_document, page, &width, &height,
> +                &dpi);
> 
> This could be a single line, next line is even longer.
> 

Yes.

> @@ +609,3 @@
> +        rect.x      = (gint) ((r->x1 * scale) + 0.5);
> +        rect.y      = (gint) ((r->y1 * scale) + 0.5);
> +        rect.width  = (gint) (((r->x2 - r->x1) * scale) + 0.5);
> 
> Don't line up this expressions, please.
> 

Ok, sorry about that, personal style I guess.

> @@ +625,3 @@
> +                     EvRectangle     *points)
> +{
> +    DjvuDocument   *djvu_document = DJVU_DOCUMENT (selection);
> 
> Extra spaces between DjvuDocument and *djvu_document

Thanks.

> 
> @@ +660,3 @@
>  {
> +    iface->get_selected_text    = djvu_selection_get_selected_text;
> +    iface->get_selection_region = djvu_selection_get_selection_region;
> 
> Do not line up these either.
> 

Ok!

> ::: backend/djvu/djvu-text-page.c
> @@ +374,3 @@
> 
> +void
> +djvu_text_page_highlight (DjvuTextPage *page,
> 
> This should be someting like djvu_text_page_get_selection, and it could return
> a GList instead of using results
> 

I will use djvu_text_page_get_selection and make it process text or boxes depending on selection variable. See patch suggestion below.


> @@ +388,3 @@
> +
> +    djvu_text_page_limits (page, page->text_structure, rectangle);
> +    djvu_text_page_selection (page, page->text_structure, 0);
> 
> We are not interested in the page itself, what we need is a function like
> djvu_text_page_selection but that only fills the array of DjvuTextLink
> 

See patch below for other suggestion.

> @@ +397,3 @@
> +        if (!(needle = strchr (haystack, '\n'))) {
> +            needle = strchr (haystack, '\0');
> +            at_end = 1;
> 
> Why not iterating the DjvuTextLink array
> 
> @@ +407,3 @@
> +            result = djvu_text_page_box (page, start, end);
> +            page->results = g_list_prepend (page->results, result);
> +            next_word_start = end_p + 1;
> 
> I think this is based on search implementation where we have a text to find,
> but here we have coordinates and we want the bounding box the the text in those
> coordinates, the actual text itself is not important.

Agreed.

Thanks!

Created attachment 247795 [details] [review]
Updated after review

Review of attachment 247795 [details] [review]:

Pushed to git master with some minor modifications, thanks!