-
Notifications
You must be signed in to change notification settings - Fork 240
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
Edit/Chat: Add ghost text alongside code #2611
Conversation
bb241b0
to
cdc2544
Compare
310678c
to
b811af5
Compare
Chatted with @valerybugakov and it may be tricky to guarantee this doesn't show alongside a completion. There doesn't appear to currently be a way to tell if VS Code has shown a completion to the user. Our best solution here would be:
However we can, at least for now, limit this only to valid user selections. That may be enough to help encourage users to use the Edit/Chat functionality and also help ensure that this ghost text does not become annoying. We should still consider how to make the "Generate" functionality more obvious:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great and it seems to be very powerful in helping us with the discoverability issue!
Left some non-blocking comments / questions.
private setGhostText(editor: vscode.TextEditor, position: vscode.Position): void { | ||
this.activeDecoration = { | ||
range: new vscode.Range(position, position), | ||
renderOptions: { after: { contentText: `${EDIT_SHORTCUT_LABEL} to Edit, ${CHAT_SHORTCUT_LABEL} to Chat` } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(more of a question for design @toolmantim 😄 ) Does it allow us to add a Cody icon to indicate this comes from Cody? This is just me, but I always find it confusing because of I don't know where all the pop-ups and decorations come from 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright this was a journey...
The API only allows contentIconPath
OR contentText
, not both. So it's very difficult to use an icon and text together.
It is technically possible though, because you could use an svg
with the <text>
element to achieve a similar effect.
![image](https://private-user-images.githubusercontent.com/9516420/297372044-2e03e105-681c-470a-8b61-b8e1d7f2fbbb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE1MzgzMjQsIm5iZiI6MTcyMTUzODAyNCwicGF0aCI6Ii85NTE2NDIwLzI5NzM3MjA0NC0yZTAzZTEwNS02ODFjLTQ3MGEtOGI2MS1iOGUxZDdmMmZiYmIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcyMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MjFUMDUwMDI0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NmMxOWZmMTc5ZjVlMTBmYzlhNDQ4NTc5YjFhYzVjYjUzNGI3MTA0YjdjYTZkMDVhM2M3YzIxZjRmMDVjZjA4MCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.yHxZXRba-ap1JsNjfcUggIPlwp8U13a8TMzClnrpnDQ)
I briefly looked into this here: #2781
Although it's possible, it's so hacky as I believe we would need to open an invisible webview just to extract certain variables out of VS Code (theme color hex value, line height, etc...)
There's some prior art: https://sourcegraph.com/github.com/cursorless-dev/cursorless. We could explore this more but IMO it has too many drawbacks to be viable right now. Tagging @toolmantim for interest
// If we find a new expanded selection position then we set it as the new ending position | ||
// and if we don't then we fallback to the original selection made by the user | ||
const newSelectionEndingPosition = | ||
(await getSmartSelection(document, activeCursorEndPosition.line))?.end || selectionRange.end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be nice if the default place to show is at the "bottom" of any selection but eitherway works too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this, but implemented it as per designs from the issue.
There's a few different directions but I think the alternative here would be to use the selection start line as the ghost text line, rather than the selection end line (cursor position). The drawback of that would be that, for large selections the ghost text might go out of view. Although we could check for that.
I think the current approach works well for now, but cc @toolmantim for thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a sick PR.
So happy to see this 🚢
Description
This PR:
onLanguage
activation event to Cody.onStartupFinished
but not as part of VS Code startup. Helps ensure our decorator is registered first, as VS Code ranks decorators in the UI depending on the order they are registered.Still TODO (implementing in a follow up PR):
Part of issue: #2009
Test plan
cody.internal.unstable