Support matching for selected text #4502
Conversation
There was a problem hiding this comment.
I've definitely wanted this feature a few times, and your implementation looks good. I would encourage this to get merged.
I did leave a few notes. I'm sorry if they ramble a bit, but they should cover my thoughts on a few other possible implementations. I personally think the best is to avoid adding the need to pass in a query parameter by instead setting FindMode.query from findSelected. That makes it more clear that these commands are replacing the find query. It also means there is less change to other places in the code and the changed functionality can all be kept near the new functions. I coded and tested this suggested implementation and it seems to work well.
Obviously, the final implementation approval is up to @philc, but I am open to help implement any of his suggestions if needed to get this merged.
content_scripts/mode_find.js
Outdated
| @@ -266,7 +266,12 @@ class FindMode extends Mode { | |||
| } | |||
|
|
|||
| // Returns null if no search has been performed yet. | |||
| static getQuery(backwards) { | |||
| static getQuery(backwards, query) { | |||
| if (query) { | |||
There was a problem hiding this comment.
Since both query and this.query have similar names, it will be confusing in the future what each represents. I think it would be helpful to add a quick comment here to explain the difference and why we need this.
A possible suggestion, based in my understanding of your implementation, might be:
// Update the this.query if we are passed in a selection query parameter
However, see my next comment to potentially change this implementation, in which case, the comment wouldn't be needed.
content_scripts/mode_find.js
Outdated
| this.query = {}; | ||
| this.updateQuery(query); | ||
| this.saveQuery(); | ||
| } | ||
| if (!this.query) return; |
There was a problem hiding this comment.
If we don't want to replace this.query, I think these changes might warrant revising this function and all of its uses to instead use the passed in query parameter rather than this.query. Otherwise, we can avoid passing a query parameter by updating this.query elsewhere (as explained at the end).
If we always pass query in, when we are calling it with a n or N command, we can pass in the this.query query text, but when we call it with * or # we can pass in our selection.
This would also solve a (potential) bug in the current implementation where if we start a search query, (example: /foo), then select a word and search it (example: select bar and go next *), it replaces our search query so that pressing n will go to the next instance of "bar", not the next instance of "foo". This is the default behavior of Vim, so it is probably what we want in this case. However, if we want to implement a command in the future that searches but shouldn't replace the query string, this will cause a bug when we try to use this function. It seems like it might be a good idea to not bake the query replacing logic into getQuery for this reason.
Even given we want to keep this behavior (I think we probably should), we can still refactor this. In that case, I would suggest removing the query parameter here and just setting FindMode.query when we call it in normal_mode.js.
content_scripts/mode_normal.js
Outdated
| findSelected() { | ||
| let selection = window.getSelection().toString(); | ||
| if (!selection) return; | ||
| return FindMode.findNext(false, selection); |
There was a problem hiding this comment.
If we do want to avoid passing in a query (see comments above), then we can add the two lines above this return.
FindMode.updateQuery(selection);
FindMode.saveQuery();
As you found, to fix the undefined object error, we also need to add an initialization to the start or the updateQuery method, like:
this.query = {};
Then everything works without needing to pass a query into the findNext or getQuery methods.
There was a problem hiding this comment.
You're right, it is a better approach. Thanks!
There was a problem hiding this comment.
The implementation looks great. Thanks for your work on this!
I ran the unit tests and they pass, but one more thing you could do is run deno fmt. For me, I always need to remove the deno.lock file first, then it works. It also updates some other files I wasn't just working on, so I always only commit the files I have touched, to avoid adding changes that aren't in the scope of this PR.
In this case, now that the findCommands list in commands.js is a longer line than recommended, it expands the list into several lines. I would recommend running the formatter and committing the commands.js file.
Also, it's worth considering whether we want to add a unit test to check if the search query gets correctly replaced when we call a findSelection, but that might be a bit complicated to test since we would need to select something in the browser first, so it's probably not worth the complexity. It's worth a thought though, since this is a somewhat complicated functionality that someone might not expect and could easily accidentally break in the future.
Again, good work. I hope we can get this merged.
Description
*: Find next match for the selected text.#: Find previous match for the selected text.