ghost text updates #124707
ghost text updates #124707
Conversation
There was a problem hiding this comment.
LGTM. There was a lot more complexity than I would have thought to deal with those live objects. I have mixed feelings about the controller: I like it because it makes the proposed API usage explicit, I don't like it because it gives up on ever being finalized, so it will just accrue debt in the proposed API.
We could just give up on scoping the event per provider and simply have a global event that is listenable by anyone and the provider must first check that the event talks about its item.
| const opacity = String(suggestPreviewForeground.rgba.a); | ||
| const color = Color.Format.CSS.format(opaque(suggestPreviewForeground))!; | ||
|
|
||
| // We need to override the only used token type .mtk1 |
There was a problem hiding this comment.
This looks off to me. Why do we need to do something special only for mtk1? Could it be that we are just testing in a context where mtk1 is always used? How about ghosted text inside a comment, is that also always painted with mtk1?
There was a problem hiding this comment.
I don't know. We are passing an empty tokens array, so I guess there only will be mtkw and mtk1. What would be the proper way? I tried setting color directly (even with !important), but the token's color overrides that.
|
Another alternative here could be having two commands on InlineCompletionItem, one for shown second one for accepted? |
We really try hard to not leak UI state through the extension API, so that we can change the UI at some point in the future without breaking all the extensions. The new API that adds events when an item is shown makes it very clear that it will never get out of the proposed state. A second onDidShow command that would just be ignored for non-authorized extensions would not be that clear. |
|
Fair enough. And again, thanks a lot for making all those improvements so quickly ❤️ |
No description provided.