Skip to content

Search editor go to location improvements - #135227

Merged
Jackson Kearl (JacksonKearl) merged 3 commits into
microsoft:mainfrom
ssigwart:goToSearch
Oct 26, 2021
Merged

Search editor go to location improvements#135227
Jackson Kearl (JacksonKearl) merged 3 commits into
microsoft:mainfrom
ssigwart:goToSearch

Conversation

@ssigwart

@ssigwart Stephen Sigwart (ssigwart) commented Oct 17, 2021

Copy link
Copy Markdown
Contributor
  • Added option to go to and select the match
  • Fixed cursor placement with initial whitespace
  • Also fixes separator typo

This PR fixes #135226.

- Added option to go to and select the match
- Fixed cursor placement with initial whitespace
@JacksonKearl

Copy link
Copy Markdown
Contributor

Thanks for creating this! Could you please split out the selection patch so that this PR impacts only search editor code?

@ssigwart

Copy link
Copy Markdown
Contributor Author

Jackson Kearl (@JacksonKearl), thanks for looking. Which part do you want me to split out? I assume you mean to only keep the code to fix one of the two issues in this branch, but I'm not sure which one you want kept in here and which to open a new PR for.

@JacksonKearl

Copy link
Copy Markdown
Contributor

This PR should only have the code changes needed for #135226. I would try to avoid changing src/vs/editor/contrib/gotoSymbol/goToCommands.ts for #128927.

@ssigwart

Copy link
Copy Markdown
Contributor Author

Thanks, Jackson Kearl (@JacksonKearl)! I update it. A far as I can tell, I cannot accomplish #128927 with an extension. If I create a new pull request for that, do you think it could be merged?


const FILE_LINE_REGEX = /^(\S.*):$/;
const RESULT_LINE_REGEX = /^(\s+)(\d+)(:| )(\s+)(.*)$/;
const RESULT_LINE_REGEX = /^(\s+)(\d+)(: | )(\s*)(.*)$/;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The separator should always be length two, yes? So two spaces on the alternate side:

/^(\s+)(\d+)(: |  )(\s*)(.*)$/;

@JacksonKearl

Copy link
Copy Markdown
Contributor

Pushed a change to fix an off-by-one on context lines, besides that looks good. Thanks for this!

@JacksonKearl
Jackson Kearl (JacksonKearl) merged commit dbea36b into microsoft:main Oct 26, 2021
@JacksonKearl

Copy link
Copy Markdown
Contributor

As for #128927, IMO handling untyped args in the goTo provider for this narrow use case is a bit odd, but I also don't know a better way. The search result editor is a bit of a patchwork of existing editor features, to change the underlying constructs too much for the narrow use case of the search editor doesn't feel great.

@ssigwart
Stephen Sigwart (ssigwart) deleted the goToSearch branch October 27, 2021 02:02
@ssigwart

Copy link
Copy Markdown
Contributor Author

Thanks. I was thinking if I could get decorations (#48364) I could write an extension to do this instead, but that's not supported. What if I implement it without the args? I haven't checked yet, but I'm wondering if after calling executeCommand for editor.action.goToDeclaration, I can just set the selection on the new editor.

@JacksonKearl

Copy link
Copy Markdown
Contributor

If you can implement it with changes localized to search editor files I would consider it.

@ssigwart

Copy link
Copy Markdown
Contributor Author

Great. Thank you, Jackson Kearl (@JacksonKearl). I found a way to do that in #136002.

@github-actions github-actions Bot locked and limited conversation to collaborators Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search editor opened results cursor placement is wrong with indentation

2 participants