Created attachment 246657 [details] [review]
Keep the caret cursor page

Review of attachment 246657 [details] [review]:

Since ev_view_next/previous_page can be called for other methods not related to caret, I prefer to do the caret page and offset actualizations explicit in the caret move methods. So, I would remove the calls to cursor_go_to_page_start/end from ev_view_next/previous_page. Also remove the cursor page update from cursor_go_to_page_start/end , and updated explictly in the other methods, for example, in cursor_backward_char you would do:

	if (view->cursor_offset == 0) {
                if (ev_view_previous_page (view)) {
		        view->cursor_page = ev_document_model_get_page (view->model);
	                return cursor_go_to_page_end (view);
                }
                return FALSE;
        }

and similar in cursor_forward_char

        if (view->cursor_offset >= n_attrs) {
                if (ev_view_next_page (view)) {
                        view->cursor_page = ev_document_model_get_page (view->model);
		        return cursor_go_to_page_start (view);
	        }
	        return FALSE;
	}

and the same in all other methods. It's a bit more code but it's also more explicit and easy to follow.

::: libview/ev-view.c
@@ +4674,3 @@
 		return FALSE;
 
+	ev_page_cache_get_text_log_attrs (view->page_cache, view->cursor_page, &log_attrs, &n_attrs);

Here you are using the previous cursor page when called from ev_view_previous_page.

@@ +4680,2 @@
 	view->cursor_offset = n_attrs;
+	view->cursor_page = ev_document_model_get_page (view->model);

Because it's updated here.

Created attachment 246696 [details] [review]
Keep the caret cursor page

Modified patch according to the review.

I've realized another problem in the patch: ev_view_next/previous_page move to the next/previous page relative to the current_page, not the cursor_page.

I have fixed this issue by calling to ev_document_model_set_page (view->cursor_page) before moving to the next/previous page. As the code repetition was getting bigger I have refactored the next/previous page logic in two functions: cursor_go_to_next/previous_page.

Another possibility that comes to my mind, it would be to create helper functions to, given a certain page, go to the previous/next page of that certain page. In that case, ev_view_next/previous_page would call to that helper function passing view->current_page as parameter, and the caret cursor navigation functions would pass to the helper function the view->cursor_page. Let me know what you prefer.

(In reply to comment #3)
> Created an attachment (id=246696) [details] [review]
> Keep the caret cursor page
> 
> Modified patch according to the review.
> 
> I've realized another problem in the patch: ev_view_next/previous_page move to
> the next/previous page relative to the current_page, not the cursor_page.
> 
> I have fixed this issue by calling to ev_document_model_set_page
> (view->cursor_page) before moving to the next/previous page. As the code
> repetition was getting bigger I have refactored the next/previous page logic in
> two functions: cursor_go_to_next/previous_page.
> 
> Another possibility that comes to my mind, it would be to create helper
> functions to, given a certain page, go to the previous/next page of that
> certain page. In that case, ev_view_next/previous_page would call to that
> helper function passing view->current_page as parameter, and the caret cursor
> navigation functions would pass to the helper function the view->cursor_page.
> Let me know what you prefer.

Yes, I think this would be a better solution. with the current patch, if the caret cursor is at the beginning of the first page, and current page is not the first page, trying to move the caret backwards would set the first page as current one even if the caret cursor hasn't been moved.

Created attachment 246751 [details] [review]
Keep the caret cursor page

Same as attachment 246696 [details] [review] but rebased. The comments in comment 4 will be addressed in the next patch to ease the review.

Created attachment 246761 [details] [review]
Don't update the page and don't scroll if the caret was not updated

Avoid to update the page and to scroll to the cursor position if the caret was not updated.

Notice that the helper functions to go_to_next/previous_page are not enough to fix the issue in comment 4, we need to check if the rest of the operations over the cursor were successful or not. We only checked the return value of cursor_key_press_event to update the page and scroll to the cursor position to make it visible, that value means that we have managed the event (don't want to propagate it), but doesn't say if the cursor was updated or not. 

In the patch I checked if the cursor was updated by comparing the previous and new values of the cursor offset and page, but I could very well have checked the return values of the cursor operations like cursor_backward_char. I can change it if you prefer it like that.

Review of attachment 246761 [details] [review]:

I've squashed both patches, fixed some minor style issues, and pushed to git master. Thanks!

::: libview/ev-view.c
@@ +4962,3 @@
+			return TRUE;
+
+		ev_document_model_set_page (view->model, view->cursor_page);

I think we don't need this, since in case of a page change ev_document_model_set_page has already been called.