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

add terminal quick fix API #166326

Merged
merged 56 commits into from
Nov 23, 2022
Merged

add terminal quick fix API #166326

merged 56 commits into from
Nov 23, 2022

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Nov 15, 2022

fixes #162950

Deferring:

@meganrogge meganrogge self-assigned this Nov 15, 2022
@meganrogge meganrogge added this to the November 2022 milestone Nov 15, 2022
@Tyriar
Copy link
Member

Tyriar commented Nov 15, 2022

Feedback from API sync, plus some of my own comments I didn't bring up. Let's talk through them in our 1:1

  • Regex matching on the renderer looks like something we're not willing to do as there are too many problems, even just capping the time could still result in UI lock ups/crashes. It's foreign code in the renderer
    • Should we send a slice of terminal output on the extension host to evaluate when a command finishes?
      • My big concern here is flooding the ext host connection, this has been a complaint in live share and general remote connections
    • Should we run the regex's in a worker off the renderer process via SAB if available?
  • Should we split out command and args to make the initial matching simpler?
    • Is this flexible enough? What about mandatory command matcher and optional command line matcher?
  • Should we make exitStatus required to further minimize the chance of misuse
  • Remove TerminalCommandSelector being registered in the provider API? This is already provided in the contribution
  • For the type property, we normally use classes in the extension host instead
  • Concerns around TerminalQuickFixCommandAction.command being confused with a VS Code command were validated as the confusion happened in the call. I don't know of a good alternative. Perhaps spelling it out in the property as terminalCommand? We do this in the UI to avoid confusion between profiles and terminal profiles.
  • Should we use the open command instead of the opener service? They're probably equivalent, make sure preview tabs work via the opener service.
  • You can use hash fragments like #L... to indicate a file's line/col
  • TerminalCommandMatchResult.outputMatch can be undefined and null
  • With the command regex, this means I never get these when using my git aliases. But don't you already know I'm actually running a git process, because you change the terminal title to git?

@meganrogge
Copy link
Contributor Author

  • Is this flexible enough? What about mandatory command matcher and optional command line matcher?

I think we should require both, though it's entirely up to the extension in reality as they can just provide * as the regexp.

  • Should we make exitStatus required to further minimize the chance of misuse

Yes - done

  • Remove TerminalCommandSelector being registered in the provider API? This is already provided in the contribution

Yes - done

  • TerminalCommandMatchResult.outputMatch can be undefined and null

Fixed

@meganrogge meganrogge enabled auto-merge (squash) November 18, 2022 23:02
@Tyriar
Copy link
Member

Tyriar commented Nov 23, 2022

A point of interest dot is being added at the beginning now, caused by the UserAliases sequence?

image

@Tyriar Tyriar disabled auto-merge November 23, 2022 16:38
@Tyriar
Copy link
Member

Tyriar commented Nov 23, 2022

It was caused by P UserAliases, we need to fix that when we re-introduce

@Tyriar Tyriar merged commit e19f06e into main Nov 23, 2022
@Tyriar Tyriar deleted the merogge/quick-fix-api branch November 23, 2022 16:59
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2023
@anthonykim1
Copy link
Contributor

Is it possible for extension to use this terminal quick fix API?
I tried looking at https://code.visualstudio.com/api/references/vscode-api#CodeActionKind.QuickFix but only quick fix I see is for code action.

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.

Terminal quick fix API
4 participants