After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702071 - Position the caret navigation cursor by clicking
Position the caret navigation cursor by clicking
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-06-12 10:25 UTC by Carlos Garcia Campos
Modified: 2013-06-18 09:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Position the caret navigation cursor by clicking (1.64 KB, patch)
2013-06-12 11:02 UTC, Antía Puentes
needs-work Details | Review
Position the caret cursor by clicking (2.38 KB, patch)
2013-06-17 17:29 UTC, Antía Puentes
committed Details | Review

Description Carlos Garcia Campos 2013-06-12 10:25:56 UTC
Clicking over text should position the caret cursor there.
Comment 1 Antía Puentes 2013-06-12 11:02:03 UTC
Created attachment 246605 [details] [review]
Position the caret navigation cursor by clicking
Comment 2 Carlos Garcia Campos 2013-06-16 15:45:19 UTC
Review of attachment 246605 [details] [review]:

Patch looks good in general, but we should place the cursor on button_press, the caret page needs to be updated and the cursor position is not accurate with the patch.

::: libview/ev-view.c
@@ +4341,3 @@
+{
+	EvRectangle *areas = NULL;
+	EvRectangle *rect = NULL;

You don't need to initialize this one.

@@ +4345,3 @@
+	guint i;
+
+	*offset = -1;

This value doesn't really matter if the function returns FALSE, so we don't need to intialize it.

@@ +4350,3 @@
+		return FALSE;
+
+	ev_page_cache_get_text_layout (view->page_cache, view->current_page, &areas, &n_areas);

This should be the page you got from get_doc_point_from_location() instead of current page and it should be returned as an out parameter like the offset.

@@ +4354,3 @@
+		return FALSE;
+
+	for (i = 0; i < n_areas && *offset < 0; i++) {

You can simply return TRUE when you find the offset.

@@ +4357,3 @@
+		rect = areas + i;
+		if (x >= rect->x1 && x <= rect->x2 &&
+		    y >= rect->y1 && y <= rect->y2)

The problem with this is that you are checking if the point is inside the character bounding box, but in most of the cases character bounding boxes overlap, so you would be positioning for cursor for the previous character of the one you clicked over. A possible approach would be to divide the character bounding box in two halves, if the point is in the first half, offset is the current character (i) and if it's inside the second half the offset is the next character (i + 1).

@@ +4453,3 @@
 			ev_view_handle_link (view, link);
 		}
+	} else if (view->caret_enabled && view->rotation == 0) {

This should be done in button_press, not in release.

@@ +4457,3 @@
+
+		if (get_doc_point_from_location (view, event->x, event->y, &page, &x, &y) &&
+		    get_offset_from_doc_point (view, x, y, &offset)) {

We could merge this both into a single function get_caret_cursor_offset_at_location() that receives a location in view coordinates, similar to other _at_location methods in EvView, and calls get_doc_point_from_location() for the given x, y values. If we also check if caret is enable and rotation is 0 this if would be simpler.

@@ +4458,3 @@
+		if (get_doc_point_from_location (view, event->x, event->y, &page, &x, &y) &&
+		    get_offset_from_doc_point (view, x, y, &offset)) {
+			view->cursor_offset = offset;

We should also update the cursor page, get_caret_cursor_offset_at_location() should return both the offset and the page.
Comment 3 Antía Puentes 2013-06-17 17:29:51 UTC
Created attachment 247055 [details] [review]
Position the caret cursor by clicking

Patch updated according to comments in comment 2
Comment 4 Carlos Garcia Campos 2013-06-18 09:08:41 UTC
Review of attachment 247055 [details] [review]:

Great, pushed to git master (with just some minor cosmetic changes). Thanks!