Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

twio142
Copy link

@twio142 twio142 commented Jun 20, 2024

Description

*: Find next match for the selected text.
#: Find previous match for the selected text.

Copy link
Contributor

@UncleSnail UncleSnail left a 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.

@@ -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) {
Copy link
Contributor

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.

this.query = {};
this.updateQuery(query);
this.saveQuery();
}
if (!this.query) return;
Copy link
Contributor

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.

findSelected() {
let selection = window.getSelection().toString();
if (!selection) return;
return FindMode.findNext(false, selection);
Copy link
Contributor

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.

Copy link
Author

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!

Copy link
Contributor

@UncleSnail UncleSnail left a 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.

Comment on lines +225 to +229
let selection = window.getSelection().toString();
if (!selection) return;
FindMode.updateQuery(selection);
FindMode.saveQuery();
return FindMode.findNext(true);
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants