Automatically select a search match

hubgit commented

2022-02-23 23:35:42 +01:00

(Migrated from github.com)

When the search query changes, it's common behaviour for the first match at or after the current selection to be selected.

This adds a new findNextInclusive function, similar to the existing findNext but allowing a match at the starting position, and calls it when the search query changes. If the search query is not valid, or there are not matches, the selection is emptied.

https://user-images.githubusercontent.com/75253002/155420290-c70b65e8-77ac-4eb7-9ddc-6ccb6896ef8e.mov

When the search query changes, it's common behaviour for the first match at or after the current selection to be selected. This adds a new `findNextInclusive` function, similar to the existing `findNext` but allowing a match at the starting position, and calls it when the search query changes. If the search query is not valid, or there are not matches, the selection is emptied. https://user-images.githubusercontent.com/75253002/155420290-c70b65e8-77ac-4eb7-9ddc-6ccb6896ef8e.mov

hubgit commented

2022-02-23 23:36:09 +01:00

(Migrated from github.com)

I'd be happy to make this behaviour configurable with an option, if the current behaviour is the desired default.

marijnh commented

2022-02-25 10:02:31 +01:00

(Migrated from github.com)

Typing in the search dialog should not change the editor selection, I think. Also, as a command, findNextInclusive doesn't seem very useful, since it will stay stuck on the current match if you execute it multiple times.

Typing in the search dialog should not change the editor selection, I think. Also, as a _command_, `findNextInclusive` doesn't seem very useful, since it will stay stuck on the current match if you execute it multiple times.

hubgit commented

2022-02-25 10:09:09 +01:00

(Migrated from github.com)

Typing in the search dialog should not change the editor selection, I think.

This is the behaviour of the search dialog in Chrome, VS Code, PHPStorm, CodeSandbox (basically everything I've tested), and is what I expected to happen, but I'm happy to make it configurable with an option if you'd prefer it not to be the default.

as a command, findNextInclusive doesn't seem very useful

That's a fair point, I can turn it into a regular function if it wouldn't be useful as a command.

> Typing in the search dialog should not change the editor selection, I think. This is the behaviour of the search dialog in Chrome, VS Code, PHPStorm, CodeSandbox (basically everything I've tested), and is what I expected to happen, but I'm happy to make it configurable with an option if you'd prefer it not to be the default. > as a _command_, findNextInclusive doesn't seem very useful That's a fair point, I can turn it into a regular function if it wouldn't be useful as a command.

marijnh commented

2022-02-25 10:25:57 +01:00

(Migrated from github.com)

As an option, I'd be okay with this. But note that, at least in VS Code, it's not starting at the current selection, it's starting at the point where the selection was at the start of the current query editing session (however that's defined), otherwise it'll still creep forward as you're editing, missing matches that were nearer to the start of the original selection.

Also, it looks like you're currently not actually updating the query state field anymore in the modified code.

As an option, I'd be okay with this. But note that, at least in VS Code, it's not starting at the _current_ selection, it's starting at the point where the selection was at the start of the current query editing session (however that's defined), otherwise it'll still creep forward as you're editing, missing matches that were nearer to the start of the original selection. Also, it looks like you're currently not actually updating the query state field anymore in the modified code.

hubgit commented

2022-02-25 14:59:39 +01:00

(Migrated from github.com)

Interesting, there seems to be a few distinct approaches to where each new query starts from.

Assuming the query to be replaced by either:
a) selecting the whole query and typing over it
or
b) selecting the whole query, pressing Backspace, then typing the new query
this is what I've observed:

Chrome, PHPStorm: continues moving forwards with each new query; moves to the start of the document if Backspace is pressed.
VS Code: moves back to the original selection with each new query, even if Backspace is pressed.
Ace: continues moving forwards with each new query, even if Backspace is pressed.

[edited as more testing found that Ace actually has a distinct behaviour]

Interesting, there seems to be a few distinct approaches to where each new query starts from. Assuming the query to be replaced by either: a) selecting the whole query and typing over it or b) selecting the whole query, pressing Backspace, then typing the new query this is what I've observed: * Chrome, PHPStorm: continues moving forwards with each new query; moves to the start of the document if Backspace is pressed. * VS Code: moves back to the original selection with each new query, even if Backspace is pressed. * Ace: continues moving forwards with each new query, even if Backspace is pressed. [edited as more testing found that Ace actually has a distinct behaviour]

marijnh commented

2022-02-25 15:06:00 +01:00

(Migrated from github.com)

The second approach seems simpler and less surprising.

hubgit commented

2022-02-25 16:30:23 +01:00

(Migrated from github.com)

I've found an alternative way to do this using an updateListener extension, acting on the setSearchQuery effect, but it needs access to the QueryType in order to call queryType.nextMatch(tr.state.doc, from, from). Would it be possible to export a getSearchQueryType method from the search extension?:

/// Get the current search query type from an editor state.
export function getSearchQueryType(state: EditorState): QueryType | undefined {
  return state.field(searchState, false)?.query
}

I've found an alternative way to do this using an `updateListener` extension, acting on the `setSearchQuery` effect, but it needs access to the `QueryType` in order to call `queryType.nextMatch(tr.state.doc, from, from)`. Would it be possible to export a `getSearchQueryType` method from the `search` extension?: ```ts /// Get the current search query type from an editor state. export function getSearchQueryType(state: EditorState): QueryType | undefined { return state.field(searchState, false)?.query } ```

hubgit commented

2022-02-26 12:19:24 +01:00

(Migrated from github.com)

I've updated the PR but it's still implementing the simplest "continue moving forwards" behaviour, now behind a config option.

As this is the behaviour I'm looking for, but perhaps shouldn't be the default, it feels like this would be better in a separate extension, in which case exporting a getSearchQueryType method would be the preferred option.

If it helps, that getSearchQueryType method would also be useful for implementing an "n of n" counter in a custom search panel.

I've updated the PR but it's still implementing the simplest "continue moving forwards" behaviour, now behind a config option. As this is the behaviour I'm looking for, but perhaps shouldn't be the default, it feels like this would be better in a separate extension, in which case exporting a `getSearchQueryType` method would be the preferred option. If it helps, that `getSearchQueryType` method would also be useful for implementing an "n of n" counter in a custom search panel.

marijnh commented

2022-02-28 10:53:57 +01:00

(Migrated from github.com)

QueryType is not currently part of the package's public interface, and doesn't seem very suitable for being made public. Would a getCursor(text: Text, from: number, to: number): Iterator<{from: number, to: number}> method on SearchQuery cover your use cases?

`QueryType` is not currently part of the package's public interface, and doesn't seem very suitable for being made public. Would a `getCursor(text: Text, from: number, to: number): Iterator<{from: number, to: number}>` method on `SearchQuery` cover your use cases?

hubgit commented

2022-02-28 13:14:40 +01:00

(Migrated from github.com)

Yes, it looks like getCursor would be sufficient.

Yes, it looks like `getCursor` would be sufficient.

marijnh commented

2022-02-28 14:19:25 +01:00

(Migrated from github.com)

Please take a look at attached patch, and let me know if it allows you to do what you're trying to do.

hubgit commented

2022-02-28 16:58:51 +01:00

(Migrated from github.com)

That's working nicely now, thanks @marijnh.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

Automatically select a search match #9

Pull request closed