-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Command and Keybinding for adding multi cursor from search result #82510
Command and Keybinding for adding multi cursor from search result #82510
Conversation
options: { | ||
preserveFocus: false, | ||
pinned: true, | ||
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.
Is this selection necessary since you select everything in the next block?
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.
Yes, this selection is necessary, because the next block that selects everything uses this selection (via let findController = CommonFindController.get(codeEditor)
).
What that block does is, select all occurrences of what is selected right now.
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 see, I was misunderstanding. I think this isn't exactly what we want then because it should select all of the matches from the search view, not "all occurrences of what is selected right now".
For example, all the matches could be different if you are matching a regex, and some identical text in a different place in the document may not actually match. Does this work without the second block at all?
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.
This does not work for regex. I'll fix it with a commit in a few days :)
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.
ping @roblourens
options: { | ||
preserveFocus: false, | ||
pinned: true, | ||
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.
Yes, this selection is necessary, because the next block that selects everything uses this selection (via let findController = CommonFindController.get(codeEditor)
).
What that block does is, select all occurrences of what is selected right now.
Created function selectAllUsingString for this purpose.
@roblourens Review please :) |
@roblourens Any idea why did check fail? |
let isRegex = this.searchWidget.searchInput.getRegex(); | ||
let isWholeWords = this.searchWidget.searchInput.getWholeWords(); | ||
let isCaseSensitive = this.searchWidget.searchInput.getCaseSensitive(); | ||
let contentPattern = this.searchWidget.searchInput.getValue(); |
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.
This is coming from the search input, not the query, so the user could have changed this without rerunning the search, then it won't be the same, right?
Actually, can we avoid triggering a find, and just select the exact ranges that are in the actual match?
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.
Two questions:
- How do I get all the matches in
searchView.ts
? - Should I create a public wrapper method for _setSelections method and use that in
searchView.ts
or this task need to be done in some other way?
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 have the matches right here in the FileMatchOrMatch
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.
Review please @roblourens
Sorry for the slow review, I was at a conference |
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.
Looks good now, but pleae fix the merge conflicts
This PR fixes #81407