GNOME Bugzilla – Bug 699486
incremental find
Last modified: 2014-10-09 21:59:33 UTC
It would be nice if the Find operation were incremental, just like in other GNOME apps such as gedit.
"It would be nice" -> please set severity=enhancement
Agreed that this would be good. Not sure how to make it happen. Older gedits had different UI and shortcuts for incremental vs. modal search, but I don't know whether that's a thing we want to do. Either way, it's low priority for now.
@Adam Dingle: Could you please be more specific on your expectations for a solution? By find operation being incremental, do you mean, that in filediff mode the search should be executed as you are typing?
Robert: Yes, that's exactly what I mean. It should work more or less as it does in gedit.
Created attachment 268875 [details] [review] Use gedit-like incremental search in file diff https://bugzilla.gnome.org/show_bug.cgi?id=699486
Please find attached a patch implementing incremental search, by simply executing a search whenever the text in the findbar search entry changes.
Robert, this is a clever first stab at an implementation. I tried out your patch and noticed a couple of issues: 1. If I type a few characters into the Find box and then press Backspace, Meld searches forward for the next matching text. Instead, in this situation it should go back to the previous match (just as gedit would). 2. If I type a few characters into the Find box, Meld moves to the first match for those characters. If I then press Enter, Meld moves to the next match for the same text. I should also be able to press Shift+Enter to move to the previous match. (Alternatively, you could let the user press the up and down arrow keys to move between matches, just like in gedit. Either way is fine with me.) Otherwise this seems fine.
Created attachment 269639 [details] [review] Proposed patch Here's an updated patch: * implements incremental search in the filediff view * when deleting something from the search, reset the search, start the search for the new string from the top * assign Shift+Return to find the previous match
Robert, thanks for this latest patch. This seems to work quite well. I have only one more small feedback point: when I press Enter to go to the next match it's quite fast, but when I press Shift+Enter it's noticeably slower and I can see the "Previous" button in the toolbar activate (perhaps that's what slows things down a bit). It would be a tad nicer if Shift+Enter and Enter worked the same way. Otherwise this seems fine to me (though the Meld maintainer might have other comments).
(In reply to comment #9) > Robert, thanks for this latest patch. This seems to work quite well. Glad to hear that. > > I have only one more small feedback point: when I press Enter to go to the next > match it's quite fast, but when I press Shift+Enter it's noticeably slower and > I can see the "Previous" button in the toolbar activate (perhaps that's what > slows things down a bit). It would be a tad nicer if Shift+Enter and Enter > worked the same way. I will look into that. By the way, I have only added an alternate shortcut for the previous search action, that works that way (by activating the previous button), are you aware of the Find Next (Ctrl+G) and Find previous (Ctrl+Shift+G) shortcuts? Could you please try if the find next with Ctrl+G is also slow (for me it's quite fast, and can't find out whether you are seeing it as slow is due to the fact that the button has to activate, and the search happens only afterwards). > > Otherwise this seems fine to me (though the Meld maintainer might have other > comments). We'll see his comments too :)
Created attachment 269670 [details] [review] Proposed patch second part This patch changes the way the search (find next and find previous) shortcuts (Ctrl+G and Ctrl+Shift+G) work: Emitting the activate signal, and then catching it and handling it shows the button pressed down and released, and the search only takes place afterwards, causing a noticeable lag as opposed to just calling the clicked signal handlers (called when the activate event is handled) for the previous/next buttons. This has the downside that the buttons lack the visual feedback that they have been pressed, but significantly improves response time of the searches, and works more like the search in gedit (the buttons are not activated there either when pressing the Up/Down shortcut to jump between the matches).
Review of attachment 269639 [details] [review]: Generally this looks good, but I think there are some details of how it should work that need sorting out. I'll play with the behaviour a bit and see if I think of anything else. Also, I think that one of the major benefits of incremental find is highlighting all matches; it would be good to also do that in Meld. Finally, I'm worried about the speed here. I would suspect that real incremental find would keep a list of candidate matches as you type, so that when the user types another character you only need to check the current set of matches to see what continues to match. We really should do this (and would need to for highlighting matches. Thanks for looking at this! ::: meld/meldwindow.py @@ +183,1 @@ This will apply the accelerator to all of Meld. I think we really only want the Shift+Return shortcut to do anything different when the findbar text entry is focused. ::: meld/ui/findbar.py @@ +127,3 @@ + it = buf.get_iter_at_offset(0) + buf.place_cursor(it) + self._find_text(0) This restarts the find from the top of the file; it should restart it from where the find orginally started. This may be tricky to get right; gedit's behaviour (which I think feels correct) is that the find point is not moved until the user defocuses the find bar or clicks Next/Prev. For example, 0. ad 1. xxx 2. ad 3. abc 4. abad 5. xxx If my cursor is on the first xxx and I start a find, I type 'ab' and the match on line 3 is highlighted. If I hit backspace and 'd' and am now searching for 'ad', line 2 should be the result line, not line 4.
(In reply to comment #12) > Review of attachment 269639 [details] [review]: > > Also, I think that one of the major benefits of incremental find is > highlighting all matches; it would be good to also do that in Meld. GtkSourceView has introduced GtkSourceSearchContext and GtkSourceSearchSettings in 3.10, which will easily take care of the searching, highlighting, jumping around, with pretty nifty performance. This is used in gedit too. > > Finally, I'm worried about the speed here. I would suspect that real > incremental find would keep a list of candidate matches as you type, so that > when the user types another character you only need to check the current set of > matches to see what continues to match. We really should do this (and would > need to for highlighting matches. > The GtkSourceView search and replace support with searchcontext is well-tested, and the performance is OK in gedit, so it should work in meld too, you shouldn't worry about that. > This will apply the accelerator to all of Meld. I think we really only want the > Shift+Return shortcut to do anything different when the findbar text entry is > focused. Got that, I will try to fix that. > > ::: meld/ui/findbar.py > @@ +127,3 @@ > + it = buf.get_iter_at_offset(0) > + buf.place_cursor(it) > + self._find_text(0) > > This restarts the find from the top of the file; it should restart it from > where the find orginally started. This may be tricky to get right; gedit's > behaviour (which I think feels correct) is that the find point is not moved > until the user defocuses the find bar or clicks Next/Prev. For example, > > 0. ad > 1. xxx > 2. ad > 3. abc > 4. abad > 5. xxx > > If my cursor is on the first xxx and I start a find, I type 'ab' and the match > on line 3 is highlighted. If I hit backspace and 'd' and am now searching for > 'ad', line 2 should be the result line, not line 4. That's right, I got a bit greedy here, but I will find a way to fix this. Thanks for the heads-up. All in all, thanks for the notes. Are you OK with bumping the required Gtk+ version for meld to 3.10? Or should I implement the highlighting and stuff conditionally only for Gtk+ >= 3.10?
Note that Meld does not yet even use GTK 3. See bug 699254.
I've been pretty bad at keeping that bug up to date. Meld in HEAD definitely uses GTK 3; we just haven't had a release yet. I'm working on fixing our single-instance activation in GApplication as we speak, and then I will hopefully do a 3.11.1 release. Having said that, we currently require GTK 3.6 & co, and I'm not intending on bumping that for this release. If the search-replace framework from GtkSourceView works out well, then I'd think about bumping to 3.10 for the next release. So in answer, I would be *extremely* happy to use GtkSourceSearch*, but for the moment it has to be conditional on the version, falling back to our existing stuff. Alternatively, you should feel free to write that code as though it won't be conditional, and I can look at only merging those changes after the next Meld stable release... just depends on how hard it is to do both.
Created attachment 276895 [details] [review] Proposed patch third part - GtkSourceView highlight all matches This proposed patch adds highlighting all search matches thanks to the GtkSourceView support added in 3.10.
Created attachment 276901 [details] [review] Proposed patch fourth part - migrate search to GtkSourceView search This patch migrates the custom search implementation to use the GtkSourceView search implementation instead, to avoid reinventing the wheel.
@Kai Willadsen: Could you please review the full patch set and comment: * I have migrated the search to gtksourceview search * search is incremental now, as the bug requests * search highlights all matches now, as you have requested I would be interested in any kind of feedback, I will fix the patches if required. @Adam Dingle: Could you please test the full patch set and provide some feedback? Performance, anything?
Robert, thanks for these latest patches. I applied them all to meld (from git master) and tried it out. Overall this seems to work great, and it's plenty fast! Just a couple of comments: 1. The message "Wrapped" open appears in the status bar even when the search has not wrapped past the end of the file. To see this, for example, run $ bin/meld bin/meld bin/meld Meld will say that the two files are identical, but that doesn't matter for our purposes. Now move the cursor to the top of the file, press Ctrl+F and type 'm'. Meld will find and highlight the first 'm', and the message 'Wrapped' will appear even though the search has not wrapped. If you continue typing 'meld', then press Enter several times, you'll see that the message 'Wrapped' somtimes appears incorrectly as you move from instance to instance of the search string. 2. If I type a string which is not found, it would be nice to display a message to that effect, or to turn the search box red (as happens in gedit).
Created attachment 276918 [details] [review] Proposed patch fourth part - migrate search to GtkSourceView search Migrate custom search implementation to the one provided by GtkSourceView, and additionally fixed the issue Adam has pointed out, with the wrapped box sometimes appearing when it should not.
Created attachment 276920 [details] [review] Proposed patch part 5. - visual indication of no match In case of a search without a match added a red background to the search entry, just like in gedit. The code already had a snippet to do this, although with a fixme as it did not work.
I just tried these latest patches. Seems to work great! I'd love to see this land.
Just a very quick review now. Unfortunately I still have GtkSourceView 3.8 on my main dev box, so I can't actually test the patches out myself yet. I'll try and update or grab a VM to test it as soon as I can. * Is this intention that 3/4/5 be applied on top of 1/2? Fine if so... just checking. * It looks to be like 3/4 should probably be squashed... particularly since half of the changes in 4 are removing semicolons added in 3. * In part 5 you add in some CSS to the FindBar class itself. Unless there's a reason not to do so, I'd prefer this to be in our main meld.css file. Anyway, the actual changes themselves look good to me, and I *will* try to find more time to review them soon. These won't go into 3.11.x however, due to the dependency bump; I'll need to make a 3.13 branch for them or something. I may take another look at just merging the first two patches before 3.12.
(In reply to comment #23) > Just a very quick review now. Unfortunately I still have GtkSourceView 3.8 on > my main dev box, so I can't actually test the patches out myself yet. I'll try > and update or grab a VM to test it as soon as I can. > > * Is this intention that 3/4/5 be applied on top of 1/2? Fine if so... just > checking. Yes, all five patches should be applied in order, I'm thinking of merging all of them together. > * It looks to be like 3/4 should probably be squashed... particularly since > half of the changes in 4 are removing semicolons added in 3. Yes, that was my mistake, but as all five patches are for incremental search, I would say merging all of them could make sense. > * In part 5 you add in some CSS to the FindBar class itself. Unless there's a > reason not to do so, I'd prefer this to be in our main meld.css file. I didn't know about the meld.css file, I have seen in another meld file that it uses a custom local css provider, that's where I got the idea from, but if a meld.css exists, I agree that it would be a better place for it, will move it there, and repropose the patch. > > Anyway, the actual changes themselves look good to me, and I *will* try to find > more time to review them soon. These won't go into 3.11.x however, due to the > dependency bump; I'll need to make a 3.13 branch for them or something. I may > take another look at just merging the first two patches before 3.12. No problem if it won't get into 3.11, 3.13 is fine for me. Regarding merging the first two patches in the stable branch: I'm OK with that, the rest of the patches was required to have all your requests implemented, like highlighting all matches, coloring the findbar to red in case of no matches. Actually, the first two patches should be enough for fixing this bug, the rest are just features for comfort and reducing duplicated code. All in all, thanks for the preliminary review and the positive attitude, I will soon re-propose two patches: * one merging the first two patches, for easier review and merging * a second one merging 3-4, requiring the dependency bump * a third one for coloring the findbar in case of no matches (fixed to move the custom css into meld.css) These make sense separately, can be applied separately, on whichever branch you'd like.
Created attachment 277141 [details] [review] Implemented incremental search Implemented incremental search by handling the search entry text changed and activated signals to jump to the next match.
Created attachment 277142 [details] [review] Use GtkSourceView support for search and highlight all matches Migrated custom search implemented in meld to be handled by the GtkSourceView search support, reducing duplicated code and adding additional features like highlighting all matches.
Created attachment 277143 [details] [review] Visual indication of no matches found Fixed setting the search entry background color to red in case no matches are found. Based on the initial feedback, I have moved the css to the main meld.css file, resubmitting patch.
(In reply to comment #23) > Just a very quick review now. Unfortunately I still have GtkSourceView 3.8 on > my main dev box, so I can't actually test the patches out myself yet. I'll try > and update or grab a VM to test it as soon as I can. By the way, there's no need for a VM, jhbuild works well, for meld 21 required modules will be downloaded and built, and you can test everything without updating and possibly breaking your dev box. [1] https://developer.gnome.org/jhbuild/3.12/
@Kai Willadsen: Did you manage to review/try some of the updated patches?
Review of attachment 277141 [details] [review]: Generally looks good to me. Some PEP8 issues, and the on_find_entry_changed() logic is a little weird for editing the search term anywhere except the end (which, granted, is the 90% case). ::: meld/meldwindow.py @@ +182,1 @@ I'd be okay with this keybinding, but only on the actual findbar text boxes themselves. Having it at the window level leads to some pretty strange behaviours (and could in fact break things). ::: meld/ui/findbar.py @@ +123,3 @@ entry.override_background_color(Gtk.StateType.NORMAL, self.orig_base_color) + current_search = self.find_entry.get_text(); No semicolons please. @@ +126,3 @@ + if current_search.find(self.last_search) == 0: # added a new character + self._find_text(0) + elif self.last_search.find(current_search) == 0: # deleted a character Inline comments should be preceeded by at least 2 spaces... or just stick them on a new line. @@ +130,3 @@ + it = buf.get_iter_at_offset(0) + buf.place_cursor(it) + self._find_text(0) I'm slightly confused by this logic. If I add a character to the start or the middle of a search term (rather than typing at the end), this doesn't do anything. Is that the intended behaviour?
Review of attachment 277142 [details] [review]: Other than the single (important) comment below, this all looks good. However, could you please check lines for stray semicolons at the end... there really shouldn't be any in the file at all. ::: meld/ui/findbar.py @@ +52,3 @@ self.textview = textview + if textview is not None: + self.search_context = GtkSource.SearchContext.new(textview.get_buffer()) This doesn't work for (one of) pygobject 3.10/gtksourceview 3.10; more to the point, it doesn't work out of the box on Fedora 20, which is what I'm testing on. The allow-none annotation seems not to be respected, so you'll need to add a None argument on the end here. Also, this line is too long.
Review of attachment 277143 [details] [review]: Looks good to me.
Sorry for the delay. A mixture of real life and other things got in the way, and jhbuild was not cooperative. I've reviewed the behaviour on F20 and with the few exceptions mentioned above, it all looks good to me. With those changes, I'd be happy to commit this first thing for 3.13. Unfortunately another change has broken actually showing the find bar in current master, so that's a different story for now. I'd suggest developing against 3.11.1 or f064296ca0bf6e9efebde58e5afd063b71f34537, and I'll rebase/fix any conflicts that occur as a result. (I've also just noticed that the review UI failed to capture the relevant code block for the keybinding change. I'm talking about the Shift-Return, which is bound to the main MeldWindow.)
(In reply to comment #33) > Sorry for the delay. A mixture of real life and other things got in the way, > and jhbuild was not cooperative. I've reviewed the behaviour on F20 and with > the few exceptions mentioned above, it all looks good to me. With those > changes, I'd be happy to commit this first thing for 3.13. Thanks. > > Unfortunately another change has broken actually showing the find bar in > current master, so that's a different story for now. I'd suggest developing > against 3.11.1 or f064296ca0bf6e9efebde58e5afd063b71f34537, and I'll rebase/fix > any conflicts that occur as a result. Yes, I've just noticed that showing the findbar is broken in the current trunk, since the resizable panes have been merged. > > (I've also just noticed that the review UI failed to capture the relevant code > block for the keybinding change. I'm talking about the Shift-Return, which is > bound to the main MeldWindow.) I will fix that, thanks for the heads-up.
Created attachment 279450 [details] [review] Implemented incremental search Implemented incremental search by handling the search entry text changed and activated signals to jump to the next match. Obsoleting the previous patch, fixed the issues pointed out in the code review: * semicolons deleted from the end of the line * make the Shift+Return shortcut to find previous work only on the findbar * removed hacks to "cache" last search (suggested in comments for performance) as this breaks entering characters in the middle of the search text, and as this patch will be followed by GtkSource.SearchContext migration, which would obsolete that "hack" anyway
Created attachment 279451 [details] [review] Use GtkSourceView support for search and highlight all matches Migrated custom search implemented in meld to be handled by the GtkSourceView search support, reducing duplicated code and adding additional features like highlighting all matches. Obsoletes the previous patch because this fixes the issues pointed out in the reviews: * line being too long, wrapped in two lines * added None parameter to GtkSource.SearchContext constructor * removed semicolons from the end of the line
Created attachment 279452 [details] [review] Visual indication of no matches found Fixed setting the search entry background color to red in case no matches are found. Based on the initial feedback, I have moved the css to the main meld.css file, resubmitting patch.
@Kai Willadsen: I have resubmitted the patches with the changes you have suggested (haven't found a better way for handling Shift+Return only in the findbar than handling it in the key_event handler, but if you would like another approach, feel free to suggest a better way). The third patch has also been resubmitted, because it didn't apply cleanly after the changes done in the previous too, so I have updated it. If you spot anything else to fix, let me know, I'll work on it.
Thanks, I'll try and take a look at these tomorrow. For reference, I've hopefully just fixed the size allocation issues in master, so the FindBar should work there again. Please let me know if it doesn't, or if you notice any other size allocation issues.
(In reply to comment #39) > Thanks, I'll try and take a look at these tomorrow. > > For reference, I've hopefully just fixed the size allocation issues in master, > so the FindBar should work there again. Please let me know if it doesn't, or if > you notice any other size allocation issues. Yes, the findbar appears again, haven't noticed any other sizing issues (yet), but I will do some more testing and let you know if I find anything else.
So this all looks good to me. I'm happy to call these patches done, and they're just waiting on a 3.13 branch to land on. I've been extremely time-poor recently, and there are a few things that need to happen before 3.12 goes out, but... hopefully soon. There are also a few extra things that the sourceview search makes possible for us, including better regex errors and faster replace-all functionality. I'll just break these out into their own bugs at some point however.
Review of attachment 279452 [details] [review]: For styling the entry when stuff is not found, we now simply use .error and the style is present in the gtk theme without needing custom css: https://git.gnome.org/browse/gedit/commit/?id=acade79b8b301dd474fc561f87081ef680d6a948
(In reply to comment #42) > Review of attachment 279452 [details] [review]: > > For styling the entry when stuff is not found, we now simply use .error and the > style is present in the gtk theme without needing custom css: > https://git.gnome.org/browse/gedit/commit/?id=acade79b8b301dd474fc561f87081ef680d6a948 I guess that will only be available in 3.14. In that case, we will need to bump the gtk+ dependency to 3.14, but as these changes will land in the 3.13 branch anyway (when it will be branched), I guess that shouldn't be a problem. Is that right, Kai, should I update the error styling patch and bump the dependecy to avoid adding custom styling and use the style from the theme instead?
@Kai Willadsen: could you please check the question from comment 43, to see whether you want to apply my patches the way they are, or go with the GTK styling instead (requiring a dependency bump) for better theming support?
(In reply to comment #44) > @Kai Willadsen: could you please check the question from comment 43, to see > whether you want to apply my patches the way they are, or go with the GTK > styling instead (requiring a dependency bump) for better theming support? The patches are fine as-is. I'm definitely not going with a dependency bump to 3.14 this cycle... maybe next cycle I'll think about it. This cycle I'll bump from 3.8 to either 3.10 or 3.12, depending. Thanks for following up though.
These patches have (finally) been pushed to master. Thank you for your forbearance.
(In reply to comment #46) > These patches have (finally) been pushed to master. Thank you for your > forbearance. Glad to hear that! Thank you for your support and patience too.