GNOME Bugzilla – Bug 566827
Find results display
Last modified: 2014-10-23 17:40:40 UTC
I want to have find results displayed in the list similar to that in Acrobat Reader's 'Full Search window'.
*** Bug 566829 has been marked as a duplicate of this bug. ***
*** Bug 566828 has been marked as a duplicate of this bug. ***
I have implemented required functionality, files attached: evince-find-results-3330.patch this shoul be applied to latest revision 3330 (at the moment) unfortunately i was unable to compile and test this - this revision seems to have errors, so i include original evince-find-results-3167m2.patch this should be applied to svn revision 3167 on which i was working on Screenshot-X.690-0207.pdf.png screenshot of what it looks like
Created attachment 125885 [details] [review] patch for svn revision 3330 implementing find results display in the sidebar
Created attachment 125886 [details] [review] patch for svn revsiion 3167 implelementing find results display in the side bar
Created attachment 125887 [details] screenshot of find results displayed in the sidebar
Looks fine, but can you please create a patch that follows Evince coding style.
Created attachment 126181 [details] [review] patch implementing find results display in the sidebar, coding style fixed can be applied to svn revision 3330
Coding style is still far from perfect. I suggest you to read http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html as well as Linux kernel coding style: http://lxr.linux.no/linux/Documentation/CodingStyle for example take care about "for" loops. About the patch itself, functions like split_line or add_current_page_to_model makes me wonder about their purpose. I cant agree with using rendering context to get matches on the page.
Created attachment 126624 [details] [review] another variant with coding style strictly fixed can be applied to svn revision 3333 it still uses rendering context for retrieving matches as there is no other backend independent way to do it at a time (should be the subject of next patches, if any)
this is a feature which would make evince really much more useful for people like me who read from a lot of huge ebooks while also lending a lot of professional improvement to evince. so please put it in soon. +1.
I cannot understand. This guy has developed a patch for evince. I think it is a good feature which matches with Gnome philosophy. It isnt intrusive, it's simple, and accessible. I do not understand why it isn't paid attention. He has sent several patches trying to fit to the rules, but no more feedback is given. Neither a good not a bad answer. I support the idea, I think it is great.
This has been in my todo list for a long time now, indeed. I've just added this feature to the RoadMap for 2.30 (see http://live.gnome.org/Evince/Roadmap). I'll try to look at the patch in detail soon.
looking at the attached screenshot, it seems to have many problems from usability and looks point of view. it would be nice if all the search related elements were moved to the left section instead of having results on left and bar on bottom. user interaction must remain as it is now like keyboard shortcuts,etc also if the search sidebar is closed and again opened by say pressing ctr+f then it should retain the results of the search.
Created attachment 159268 [details] [review] Sergey Pushkin's patch updated to git master Hey, I updated the patch to git master. I just applied the patch to EVINCE_2_25_4 commit and then merged to master, so no promises that will work. I will take a look at it soon.
Created attachment 159269 [details] [review] Sergey Pushkin's patch updated to git master Hi, previous patch didn't compile. This one compiles and runs as expected.
Created attachment 159289 [details] [review] Updated patch As jjmarin noticed, the previous patch was crashing sometimes, since the patch unlocked the document mutex too early. Here is an updated patch that should not crash + minor other changes.
First of all, I'd like to thank to Sergey Pushkin for his patch and to José Aliste for updating and adapting the patch for current git master. It work ok for me, it doesn't crash now :). However, I've found a minor misbehaviour. When there several result on the same line, they are showed in the wrong order on the sidebar, but they are highlighted on the page folowing the right order. So there are a mismatch between the result selected on the sidebar and the result highlighed. I attach a snapshot of this problem.
Created attachment 159375 [details] Mismatch between the result selected on the sidebar and the result highlighed on the page
Created attachment 159397 [details] [review] updated patch which fixes mismatch described in #18 comment can be applied to git revision 951e6310e0143842ea907865249e5cf25b9d00cd
(In reply to comment #18) > First of all, I'd like to thank to Sergey Pushkin for his patch and to José > Aliste for updating and adapting the patch for current git master. > > It work ok for me, it doesn't crash now :). However, I've found a minor > misbehaviour. When there several result on the same line, they are showed in > the wrong order on the sidebar, but they are highlighted on the page folowing > the right order. So there are a mismatch between the result selected on the > sidebar and the result highlighed. > > I attach a snapshot of this problem. I've fixed this, updated patch is attached
This seems useful indeed. Thanks to Sergey Pushkin.
Created attachment 228376 [details] [review] updated patch 1/2 I rebased and reworked the patch by Sergey. Most importantly, I simplified all the string handling code by using GtkTextBuffer instead of direct string manipulations. It should be easier to review.
Created attachment 228377 [details] [review] updated patch 2/2
Carlos, could you review these patches once you get the time?
I like this, but I have some concerns regarding the UI. I'm not sure the sidebar is the best place for this. The contents depend on whether you have started a new search or not. The results are still available even if the findbar is closed (which it's supposed to finish the find). We are always getting the model even when the sidebar is closed. We could show the sidebar automatically in the find results page when a new find starts, and restore it to the previous state when the find finishes. Or we could move all the find UI to the sidebar, but that would make the find depend on the sidebar and we could not use other sidebar pages while searching. Another option would be to use a separate window, like acroread does. What do you think? This is just about the UI, I'll review the patch to comment about the implementation.
Another downside of placement in the sidebar is the limited room for context shown with each match, making it harder for the user to locate the desired match at a glance. That constraint would be lifted by moving the whole thing to a separate window. Plus, I agree that being able to see other sidebar material during the search is helpful, allowing the user to see even more context when choosing which matches are relevant.
Review of attachment 228376 [details] [review]: ::: libview/ev-jobs.c @@ +1563,3 @@ + + job_find = EV_JOB_FIND (job); + buffer = gtk_text_buffer_new (NULL); It seems the buffer is leaked, it's created here but never freed. Move the creation after the early return, so that you don't created it if there aren't matches. @@ +1568,3 @@ + if (!matches) + return; + Extra empty lines here. @@ +1572,3 @@ + page = ev_document_get_page (job->document, job_find->current_page); + + occno = 0; Please use variable names easier to understand, it took me a while to realize occno mean occurrence number. @@ +1598,3 @@ + occno++; + for (k = 0; k < occno; k++ ) { + if (!gtk_text_iter_forward_search (&find_iter, text_to_find, 0, &start, &end, NULL)) Instead of converting the text and line to lowercase in case of case insensitive search, you could simply use the flag GTK_TEXT_SEARCH_CASE_INSENSITIVE, and keep the original line in the model. It looks weird that all serach results in the sidebar are lowercase. @@ +1666,3 @@ + + if (job->model) { + gtk_list_store_clear (GTK_LIST_STORE (job->model)); why clear the model right before unref it? @@ +1705,2 @@ job_find->pages[job_find->current_page] = matches; + ev_job_find_process_matches (job); This could make the find job slower in general, and the sidebar could be even hidden. Maybe we could move these to the sidebar widget? The benefit of doing it here is that the document lock is held already. ::: libview/ev-view.c @@ +5793,2 @@ void +ev_view_find_arbitrary (EvView *view, gint page, gint result) hmm, what about ev_view_find_highlight_result?
I just rebased this work on top of master so we can play with the UI a little more. To make it easier, I push it as wip/find_results branch in the GNOME repo. I still haven't address the code review.
I just reworked the patches against current master, fixed the issues spotted on the code review and also a bug on false positives that was making evince to crash on some files. Now, code wise it shoulde be more or less ok. Let's discuss the ui again?
I've been reviewing the branch and reworked it in another branch, see: https://git.gnome.org/browse/evince/log/?h=wip/find-results2 In the end I'm using a sidebar, but not a new page in the existing sidebar, but a sidebar that is shown instead of the normal one when search is active. It's shown and closed when the find bar is shown and closed. This is very similar to what mac os x does, what do you think? If nobody objects I'll merge it in master next week.
The feature works nicely. I just noticed two minor issues with the context. One is that regardless of how wide the find sidebar is, only a small amount of context is shown. I had expected that making it wider would allow more context to be shown. The second is that occasionally a bit of vertical whitespace sneaks into the context. This seems to happen when the found text is in a bullet point or similar construct. I'll add a couple of screenshots to illustrate.
Created attachment 247561 [details] Extra vertical whitespace in findbar when found text is in a bullet point
Created attachment 247562 [details] Extra vertical whitespace in findbar when found text is in code example
Thanks for testing it, yes. I'm aware of those issue, I'll try to fix them in follow up patches.
Carlos, thanks for taking the time to finish this! Please merge it.
(In reply to comment #32) > I've been reviewing the branch and reworked it in another branch, see: > > https://git.gnome.org/browse/evince/log/?h=wip/find-results2 > > In the end I'm using a sidebar, but not a new page in the existing sidebar, but > a sidebar that is shown instead of the normal one when search is active. It's > shown and closed when the find bar is shown and closed. This is very similar to > what mac os x does, what do you think? If nobody objects I'll merge it in > master next week. I see several issues: I. Full screen behaviour: The behaviour in full screen is odd, although no much better than the current one. What I see: 1. Press F11 to go to full screen 2. Press Ctrl-F or the Search Button The toolbar hides the search entry and the top of the sidebar. After some seconds the toolbar goes away, also the focus. So, the search entry is there with the sidebar, but whatever I type is not entered there and it does not search. So, to make it work, I have to search in blind. That is, Ctrl+F and type, but I can't see what I am entering. With 2 displays is ... playful. I can move the cursor out of the screen, the toolbar hides and I can see the search entry. As soon as I go back to evince with the mouse, the toolbar appears again and I can not see the search entry. To get the focus on evince, I click in any place, but the search is not receiving anything I type. The current behaviour is not better, though. See Bug 701062 II. Closing the search bar If I click in the document view, how can I close the search bar? It seems I can only close it with the toolbar or changing the focus to the search entry. Ctrl+F does not respond. If evince is in fullscreen, ESC leaves the full screen mode. If not, it does not do anything. III. Document view is resized The search entry and the sidebar makes the document view smaller. If I had set the window size to a size which is comfortable to read, when I search I barely read can the document because is resized. This is exacerbated when using dual page, because if there are several matches in both pages and I try to navigate with the sidebar, I can barely see which match is the selected one in the document. The blue-ish selections are not very distinguishable in smaller sizes. This happens because the sidebar is vertical and makes the document stretch.
(In reply to comment #38) > [...] > II. Closing the search bar > > If I click in the document view, how can I close the search bar? It seems I > can only close it with the toolbar or changing the focus to the search entry. > Ctrl+F does not respond. Similar to this. Ctrl+F should grab the focus of the search entry even if the search bar is already opened. I found myself searching for a word, changed to an editor and then came back to evince and browsing the document. When I wanted to search for another word, I pressed Ctrl+F and it didn't work and the search entry didn't have the focus. So, I had to select manually the search entry to start a new search.
On second thoughts on my last comment, it would be a regression. A kind of annoying regression after some use (I think it should be easy to fix, though). As a nitpick note, depending of the width of the sidebar the layout in general does not look nice. It seems (or feels) there is lack of alignment.
(In reply to comment #38) > (In reply to comment #32) > > I've been reviewing the branch and reworked it in another branch, see: > > > > https://git.gnome.org/browse/evince/log/?h=wip/find-results2 > > > > In the end I'm using a sidebar, but not a new page in the existing sidebar, but > > a sidebar that is shown instead of the normal one when search is active. It's > > shown and closed when the find bar is shown and closed. This is very similar to > > what mac os x does, what do you think? If nobody objects I'll merge it in > > master next week. > > I see several issues: Thanks for testing it. > I. Full screen behaviour: > > The behaviour in full screen is odd, although no much better than the current > one. > > What I see: > 1. Press F11 to go to full screen > 2. Press Ctrl-F or the Search Button > > The toolbar hides the search entry and the top of the sidebar. > > After some seconds the toolbar goes away, also the focus. So, the search entry > is there with the sidebar, but whatever I type is not entered there and it does > not search. > > So, to make it work, I have to search in blind. That is, Ctrl+F and type, but > I can't see what I am entering. > > With 2 displays is ... playful. I can move the cursor out of the screen, the > toolbar hides and I can see the search entry. As soon as I go back to evince > with the mouse, the toolbar appears again and I can not see the search entry. > To get the focus on evince, I click in any place, but the search is not > receiving anything I type. > > The current behaviour is not better, though. See Bug 701062 > I think all fullscreen issues would be solved if we never hide the toolbar while a search is active. > > II. Closing the search bar > > If I click in the document view, how can I close the search bar? It seems I > can only close it with the toolbar or changing the focus to the search entry. > Ctrl+F does not respond. The findbar is closed using the toggle button in the toolbar or pressing ESC. Before we had a toogle button in the toolbar it made sense to close the findbar by clicking anywhere outside the findbar, but I find it very confusing with the current design. I prefer to have a explicit way to close it, we can add a close button to the findbar itself if the toggle button in the toolbar is not obvious enough or convenient. With the find results display clicking a match in the sidebar would close the findbar, so this is not an option anymore. > If evince is in fullscreen, ESC leaves the full screen mode. If not, it does > not do anything. This is just a bug, the findbar should take precedence over the unfullscreen action. > > III. Document view is resized > > The search entry and the sidebar makes the document view smaller. If I had set > the window size to a size which is comfortable to read, when I search I barely > read can the document because is resized. > > This is exacerbated when using dual page, because if there are several matches > in both pages and I try to navigate with the sidebar, I can barely see which > match is the selected one in the document. The blue-ish selections are not > very distinguishable in smaller sizes. > > This happens because the sidebar is vertical and makes the document stretch. Yes, the only thing I can think of to fix this, is to add a close button to the find sidebar. This problem is not specific to the find sidebar, the same happens with the normal sidebar if you are using fit-width zoom. After trying the separate window approach I don't think it's an option either. It always hides part of the document, so you end up moving it around all the time, and it wouldn't work in fullscreen mode either. I'll try to solve some of the issues before merging the branch, as well as the ones reported by Ben, and some others I've found too. Then, please file separate bug reports for the remaining issues.
(In reply to comment #39) > (In reply to comment #38) > > [...] > > II. Closing the search bar > > > > If I click in the document view, how can I close the search bar? It seems I > > can only close it with the toolbar or changing the focus to the search entry. > > Ctrl+F does not respond. > > Similar to this. Ctrl+F should grab the focus of the search entry even if the > search bar is already opened. Sounds good to me. > I found myself searching for a word, changed to an editor and then came back to > evince and browsing the document. When I wanted to search for another word, I > pressed Ctrl+F and it didn't work and the search entry didn't have the focus. > So, I had to select manually the search entry to start a new search. It makes a lot of sense, yes.
(In reply to comment #40) > On second thoughts on my last comment, it would be a regression. A kind of > annoying regression after some use (I think it should be easy to fix, though). > > As a nitpick note, depending of the width of the sidebar the layout in general > does not look nice. It seems (or feels) there is lack of alignment. That's because the search entry is aligned to the left, maybe we could center it like ephy does.
(In reply to comment #41) > (In reply to comment #38) > > (In reply to comment #32) > > [...] > > II. Closing the search bar > > > > If I click in the document view, how can I close the search bar? It seems I > > can only close it with the toolbar or changing the focus to the search entry. > > Ctrl+F does not respond. > > The findbar is closed using the toggle button in the toolbar or pressing ESC. > Before we had a toogle button in the toolbar it made sense to close the findbar > by clicking anywhere outside the findbar, but I find it very confusing with the > current design. I prefer to have a explicit way to close it, we can add a close > button to the findbar itself if the toggle button in the toolbar is not obvious > enough or convenient. With the find results display clicking a match in the > sidebar would close the findbar, so this is not an option anymore. ESC works only if the search bar has the focus. I think ESC should work globally and perform an action whether there is something to close or not (given a priority). In that way, closing the search bar would not depend on the mouse.
Merged in master. I've switched to use text layout and pango instead of GtkTextBuffer, so that we don't need to search again the string in the GtkTextBuffer. This also allowed me to easily remove the line breaks, resolve the hyphenations, and add more context, so that all problems reported by Ben should be fixed. You need poppler from git master for this to work properly in many documents, though, I'll bump the requirements once there's a new poppler version. Also fixed some of the issues reported by German, except the fullscreen problems, that are actually an existing problem not introduced by the find sidebar. Please, file new bug reports for any remaining or new issue. Thanks
Btw, thanks also to Sergey for writing the original patch and Jose for keeping it updated and insisting me to review it :-)
Has this been implemented into latest evince viewer? I downloaded the latest windows version, and this feature appears to be missing (or I am not sure how to turn it on).
Unfortunately, the latest Windows version is an old one (2.32). There has been many releases since then (14), but not releases for Windows (somebody has to do the work).