GNOME Bugzilla – Bug 703570
Focused item in search sidebar should check current page before ignore click
Last modified: 2014-02-23 14:08:09 UTC
1. Open a PDF with several pages. 2. Search for a term 3. Click in any item in the search sidebar, the view will change to the page with the term/page selected. 4. Browse the document (with the scrollbar or arrow keys) 5. Click on the same item selected (and still focused) in (3). Results: The documents stays in the same place. Expected results: Jump to the term/page selected. The workaround is to click in another term selected and select back the previous one. Although, this only works with 2 or more terms found.
hmm, that also happens with the index in the sidebar. It's because the action is done in selection-changed signal callback.
*** Bug 710742 has been marked as a duplicate of this bug. ***
I have been working on this bug and have written a button click handler. However, the signal emitted by my handler conflicts with this line of code - libview/ev-view.c:7747 if (view->find_page == page && view->find_result == result) In the past, with respect to this bug, https://bugzilla.gnome.org/show_bug.cgi?id=710778 the change was made. But, it should really be view->current_page and view->current_result or else when a user clicks on the already selected item but has scrolled to a different page, the comparison would be done with find_page instead of current_page. I thought of a tweak for this problem by emitting two signals for my handler. One that would point to the correct page but different result (say result - 1) and then it would point to the right result. That way, in essence, the problem for the user is solved. Please let me know how to proceed.
Created attachment 269186 [details] [review] I have attached the patch. Please let me know of any required changes.
Review of attachment 269186 [details] [review]: Thanks for the patch, I have a few comments. ::: shell/ev-find-sidebar.c @@ +142,3 @@ + + + sidebar->priv = G_TYPE_INSTANCE_GET_PRIVATE (sidebar, EV_TYPE_FIND_SIDEBAR, EvFindSidebarPrivate); You don't need to do this, this is already done in ev_find_sidebar_init, here you can use sidebar->priv directly. @@ +159,3 @@ + -1); + + priv->highlighted_result = gtk_tree_model_get_path (model, &iter); You are leaking any previous path in highlighted_result. This code is actually the same that what we have in selection_changed_callback, so you could move it to a helper function. ev_find_sidebar_activate_result_at_iter () or something similar @@ +162,3 @@ + + g_signal_emit (sidebar, signals[RESULT_ACTIVATED], 0, page - 1, result + 1); + g_signal_emit (sidebar, signals[RESULT_ACTIVATED], 0, page - 1, result); Don't do this double emission, I think we can remove the check from ev_view_find_set_result, it's not a big problem if it's called twice with the same parameters.
Review of attachment 269186 [details] [review]: ::: shell/ev-find-sidebar.c @@ +152,3 @@ + model = gtk_tree_view_get_model (view); + gtk_tree_model_get_iter (model, &iter, path); + gtk_tree_path_free (path); Also note that when you click on an item different than the currently selected one, both this and the selection changed will be done. Maybe we can check here if the item clicked is the currently selected one and emit the result activated signal only for that particular case.
Created attachment 269730 [details] [review] Modified the patch as per the comments Thanks for the review. I have updated my patch as per the suggestions. Please let me know if there are any further issues.
(In reply to comment #6) > Review of attachment 269186 [details] [review]: > > ::: shell/ev-find-sidebar.c > @@ +152,3 @@ > + model = gtk_tree_view_get_model (view); > + gtk_tree_model_get_iter (model, &iter, path); > + gtk_tree_path_free (path); > > Also note that when you click on an item different than the currently selected > one, both this and the selection changed will be done. Maybe we can check here > if the item clicked is the currently selected one and emit the result activated > signal only for that particular case. Sorry I forgot to include this in my latest patch. Will do that soon. Sorry for the inconvenience.
Created attachment 269732 [details] [review] Incorporated all the suggestions Sorry for earlier. I have now included all the changes suggested.
Review of attachment 269732 [details] [review]: Thanks, there are still a few minor issues that I have fixed before landing. ::: shell/ev-find-sidebar.c @@ +104,2 @@ static void +ev_find_sidebar_activate_result_at_iter (GtkTreeIter *iter, This method should receive the EvFindSidebar as the first argument @@ +113,3 @@ + if (priv->highlighted_result) + gtk_tree_path_free (priv->highlighted_result); + priv->highlighted_result = gtk_tree_model_get_path (model, iter); The code that gets the page and result form the model and emits the signal is also duplicated, it could be added here. The method is called activate_result, but it's not doing so. @@ +157,3 @@ + return FALSE; + + if(priv->highlighted_result && gtk_tree_path_compare(priv->highlighted_result, path)) if( -> if ( @@ +158,3 @@ + + if(priv->highlighted_result && gtk_tree_path_compare(priv->highlighted_result, path)) + return FALSE; You are leaking the path here.
Thanks for the required correction. Will keep these in mind for the future. Thanks for the commit, too.