GNOME Bugzilla – Bug 704335
ev_view_accessible_get_selection should work as expected
Last modified: 2014-06-10 16:02:19 UTC
The implementation of the atk_text_get_selection is broken in EvViewAccessible. It should return the selected text and the start and end positions of the selected region.
Created attachment 249288 [details] [review] Return the text and offset range of the text selected in the current page The implementation of atk_text_get_selection returns the start and end offsets of the selected region. Evince works with offsets per page, so we would need to work with global offsets to avoid errors when the selection covers several pages. As a first approach, this patch returns the text and offsets of the text selected in the current page, if any.
Review of attachment 249288 [details] [review]: Looks good, there are just a few minor details. Thanks. ::: libview/ev-view-accessible.c @@ +659,3 @@ + + *start_offset = start; + *end_offset = end; Please, don't line up the = @@ +692,3 @@ return NULL; + ev_document_doc_mutex_lock (); Limit the mutex to the ev_selection_get_selected_text call, if there aren't selections in the current page we would be locking for nothing. @@ +696,1 @@ + for (l = view->selection_info.selections; l != NULL && !current_page; l = l->next) { Break the loop when you find the selections instead of using a boolean local variable. @@ +696,3 @@ + for (l = view->selection_info.selections; l != NULL && !current_page; l = l->next) { + EvViewSelection *selection = (EvViewSelection *)l->data; + EvPage *page; This is only used in the if (get_selection_bound, you can move it there @@ +704,3 @@ + current_page = TRUE; + if (get_selection_bounds (view, selection, &start, &end) && start != end) { + page = ev_document_get_page (view->document, selection->page); This returns a new EvPage, that you are leaking. Call g_object_unref after ev_selection_get_selected_text @@ +711,3 @@ + + *start_pos = start; + *end_pos = end; Please, don't line up the = ::: libview/ev-view.c @@ +4298,3 @@ +{ + gint offset = + _ev_view_get_caret_cursor_offset_at_doc_point (view, page, doc_x, doc_y); Use two different lines here better. gint offset; offset = _ev_view_get_caret_cursor_offset_at_doc_point (view, page, doc_x, doc_y);
Created attachment 249502 [details] [review] Return the text and offset range of the text selected in the current page Patch updated according to comments in comment 2
Comment on attachment 249502 [details] [review] Return the text and offset range of the text selected in the current page Pushed, thanks!
*** Bug 701790 has been marked as a duplicate of this bug. ***