-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support matching for selected text #4502
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it is a better approach. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
let selection = window.getSelection().toString(); | ||
if (!selection) return; | ||
FindMode.updateQuery(selection); | ||
FindMode.saveQuery(); | ||
return FindMode.findNext(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both the forward and backward implementation use the same code except for the forward/backward boolean, it might make sense to use a helper function that accepts a boolean. It just depends what @philc prefers for maintainability.
Description
*
: Find next match for the selected text.#
: Find previous match for the selected text.