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.

Sign up for GitHub

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

Jump to bottom

Support matching for selected text #4502

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

Support matching for selected text #4502

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b786e1c
find mode: support matching for selected text
twio142 Jun 20, 2024
8578bd8
avoid passing in a query to `FindMode.findNext`
twio142 Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion background_scripts/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ const Commands = {
"Vomnibar.activateEditUrl",
"Vomnibar.activateEditUrlInNewTab",
],
findCommands: ["enterFindMode", "performFind", "performBackwardsFind"],
findCommands: ["enterFindMode", "performFind", "performBackwardsFind", "findSelected", "findSelectedBackwards"],
historyNavigation: ["goBack", "goForward"],
tabManipulation: [
"createTab",
Expand Down Expand Up @@ -423,6 +423,8 @@ const defaultKeyMappings = {
"/": "enterFindMode",
"n": "performFind",
"N": "performBackwardsFind",
"*": "findSelected",
"#": "findSelectedBackwards",

// Vomnibar
"o": "Vomnibar.activate",
Expand Down Expand Up @@ -509,6 +511,8 @@ const commandDescriptions = {
enterFindMode: ["Enter find mode", { noRepeat: true }],
performFind: ["Cycle forward to the next find match"],
performBackwardsFind: ["Cycle backward to the previous find match"],
findSelected: ["Find the selected text"],
findSelectedBackwards: ["Find the selected text backwards"],

goPrevious: ["Follow the link labeled previous or <", { noRepeat: true }],
goNext: ["Follow the link labeled next or >", { noRepeat: true }],
Expand Down
11 changes: 8 additions & 3 deletions content_scripts/mode_find.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

// check if the query has been changed by a script in another frame
const mostRecentQuery = FindModeHistory.getQuery();
Expand Down Expand Up @@ -358,9 +363,9 @@ class FindMode extends Mode {
return FindMode.saveQuery();
}

static findNext(backwards) {
static findNext(backwards, query) {
// Bail out if we don't have any query text.
const nextQuery = FindMode.getQuery(backwards);
const nextQuery = FindMode.getQuery(backwards, query);
if (!nextQuery) {
HUD.show("No query to find.", 1000);
return;
Expand Down
12 changes: 12 additions & 0 deletions content_scripts/mode_normal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,18 @@ const NormalModeCommands = {
}
},

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!

},

findSelectedBackwards() {
let selection = window.getSelection().toString();
if (!selection) return;
return FindMode.findNext(true, selection);
},

// Misc.
mainFrame() {
return focusThisFrame({ highlight: true, forceFocusThisFrame: true });
Expand Down