GNOME Bugzilla – Bug 730252
Evince doesn't scroll to result when not in continuous mode
Last modified: 2015-02-19 18:31:37 UTC
Created attachment 276675 [details] [review] Scroll to the search result selected by user Evince doesn't scroll to page with result selected by user when it is not in continuous mode. Attached patch fixes this for me. Marek
Review of attachment 276675 [details] [review]: ::: libview/ev-view.c @@ +7642,3 @@ _ev_view_transform_doc_rect_to_view_rect (view, page, rect, &view_rect); + if (!view->continuous) + ev_document_model_set_page (view->model, page); hmm, I don't think it's correct. jump_to_find_result assumes you already called jump_to_find_page, so there must a be a call to jump_to_find_result where we forgot to call jump_to_find_page. Could you check that, please?
Created attachment 276768 [details] [review] Scroll to the search result selected by user (In reply to comment #1) > Review of attachment 276675 [details] [review]: > > ::: libview/ev-view.c > @@ +7642,3 @@ > _ev_view_transform_doc_rect_to_view_rect (view, page, rect, > &view_rect); > + if (!view->continuous) > + ev_document_model_set_page (view->model, page); > > hmm, I don't think it's correct. jump_to_find_result assumes you already called > jump_to_find_page, so there must a be a call to jump_to_find_result where we > forgot to call jump_to_find_page. Could you check that, please? It is not called in ev_view_find_set_result(). But even if I call it there it doesn't jump there, so I have to call the ev_document_model_set_page() in jump_to_find_page(). Another problem I found during fixing this is that when there are more than 1 results on a page then requesting next search result from the same page will not return you to the page with the search results if you moved from the page in the meantime (when you are not in continuous mode). This applies also to previous results (see the next patch).
Created attachment 276769 [details] [review] Show correct page when next search result requested
Review of attachment 276768 [details] [review]: Thanks!
Review of attachment 276769 [details] [review]: LGTM, except the next/prev_page changes that I'm not sure. ::: libview/ev-view.c @@ +8416,2 @@ ev_document_model_set_page (view->model, next_page); + view->current_page = next_page; This looks unrelated to this patch, and I'm not sure it's correct either, the current page should be updated in ev_view_change_page(). @@ +8433,2 @@ ev_document_model_set_page (view->model, prev_page); + view->current_page = prev_page; Same here, what happens if you change the page using any other way, like going to any page from the page selector or using go to first/last page?
Comment on attachment 276768 [details] [review] Scroll to the search result selected by user (In reply to comment #4) > Review of attachment 276768 [details] [review]: > > Thanks! Thank you for the review. I've pushed the patch to master.
Created attachment 290901 [details] [review] Show correct page when next search result requested (In reply to comment #5) > Review of attachment 276769 [details] [review]: > > LGTM, except the next/prev_page changes that I'm not sure. > > ::: libview/ev-view.c > @@ +8416,2 @@ > ev_document_model_set_page (view->model, next_page); > + view->current_page = next_page; > > This looks unrelated to this patch, and I'm not sure it's correct either, the > current page should be updated in ev_view_change_page(). > > @@ +8433,2 @@ > ev_document_model_set_page (view->model, prev_page); > + view->current_page = prev_page; > > Same here, what happens if you change the page using any other way, like going > to any page from the page selector or using go to first/last page? I'm not sure why I placed it there. It works without these 2 hunks too. So I've removed them.
*** Bug 739325 has been marked as a duplicate of this bug. ***
Review of attachment 290901 [details] [review]: Perfect, please, push it to both branches. Thanks!
(In reply to comment #9) > Review of attachment 290901 [details] [review]: > > Perfect, please, push it to both branches. Thanks! Thank you. Do you want me to push the first one to the "gnome-3-14" branch too?
I think the first one was pushed before I branched, otherwise please cherry-pick both.
(In reply to comment #11) > I think the first one was pushed before I branched, otherwise please > cherry-pick both. Actually, I've pushed it today so it was not in 3.14 branch yet. I've pushed them both to 3.14 a minute ago then. Thank you for all the reviews. I'm closing this as fixed.
*** Bug 739612 has been marked as a duplicate of this bug. ***
*** Bug 744809 has been marked as a duplicate of this bug. ***