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

Search editor go to location improvements #135227

Merged
merged 3 commits into from Oct 26, 2021

Conversation

ssigwart
Copy link
Contributor

@ssigwart ssigwart commented Oct 17, 2021

  • 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
Contributor

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

@JacksonKearl JacksonKearl self-requested a review October 23, 2021 01:01
@ssigwart
Copy link
Contributor Author

@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
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
Contributor Author

Thanks, @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?

@@ -7,7 +7,7 @@ import * as vscode from 'vscode';
import * as pathUtils from 'path';

const FILE_LINE_REGEX = /^(\S.*):$/;
const RESULT_LINE_REGEX = /^(\s+)(\d+)(:| )(\s+)(.*)$/;
const RESULT_LINE_REGEX = /^(\s+)(\d+)(: | )(\s*)(.*)$/;
Copy link
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
Contributor

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

@JacksonKearl JacksonKearl merged commit dbea36b into microsoft:main Oct 26, 2021
@JacksonKearl
Copy link
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 ssigwart deleted the goToSearch branch October 27, 2021 02:02
@ssigwart
Copy link
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
Contributor

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

@ssigwart
Copy link
Contributor Author

Great. Thank you, @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