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

Offset terminal when inline chat appears to ensure cursor is visible #210239

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Apr 12, 2024

This will overlay the terminal inline chat widget on top of the terminal the same as previously where the cursor would be visible. If it would not be visible, the terminal wrapper element is now shifted upwards to ensure it's visible. This comes with a couple of downsides:

  • The very top of the terminal cannot be accessed as overflow is clipped by the view.
  • The chat may overlap content if there is any below the cursor, which is an unlikely edge case.

This isn't as good as a zone widget-like system in xterm.js could be, but that is quite the undertaking and complicates the renderer code quite a bit. So this is a good compromise.

Fixes microsoft/vscode-copilot#4720

Recording 2024-04-12 at 07 00 17

This will overlay the terminal inline chat widget on top of the terminal the same
as previously where the cursor would be visible. If it would not be visible, the
terminal wrapper element is now shifted upwards to ensure it's visible. This comes
with a couple of downsides:

- The very top of the terminal cannot be accessed as overflow is clipped by the
  view.
- The chat may overlap content if there is any below the cursor, which is an
  unlikely edge case.

This isn't as good as a zone widget-like system in xterm.js could be, but that is
quite the undertaking and complicates the renderer code quite a bit. So this is
a good compromise.

Fixes #4720
@Tyriar Tyriar added this to the April 2024 milestone Apr 12, 2024
@Tyriar Tyriar requested a review from meganrogge April 12, 2024 14:00
@Tyriar Tyriar self-assigned this Apr 12, 2024
@Tyriar Tyriar enabled auto-merge April 12, 2024 14:20
Comment on lines +63 to +69
hideLock() {
this._overlay.value?.lockHide();
}

hideUnlock() {
this._overlay.value?.unlockHide();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this terminology is a bit confusing ... could it just be lock and unlock? vs hideLock and hideUnlock

Copy link
Member Author

Choose a reason for hiding this comment

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

Lock and unlock by themselves seem like they'd be related to positioning. Also it has another hide mechanism that this overrides

Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

👍🏼

@Tyriar Tyriar merged commit 3b3e8c2 into main Apr 12, 2024
6 checks passed
@Tyriar Tyriar deleted the tyriar/copilot-4720 branch April 12, 2024 14:35
@microsoft microsoft locked and limited conversation to collaborators Jun 6, 2024
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.

2 participants