Enforce editor options from lists/trees in extension commands #109947
Conversation
| /** | ||
| * Remember the last `IOpenEvent` that occured. | ||
| */ | ||
| setOpenContext(context: IOpenEvent<unknown>): IDisposable; |
There was a problem hiding this comment.
Could we move something like this directly into the IOpenerService? That already knows OpenOption. With that the service could handle everything internally and the commands wouldn't be required to know this (tho the command do need some tweak so that they don't use the editor service directly).
There was a problem hiding this comment.
@jrieken isn't that risking the exposure of this "hack" to anyone that uses the opener service? What I like about this approach is that it is exclusively used for the 2 commands and nothing else and we know only extensions can call it.
There was a problem hiding this comment.
With that it is definitely widely available but I don't see it as a hack but as something really nice. Thinking of all the bugs I have fixed for "internal" features like outline, peek, go-to-links etc I would have really wanted this automagic behaviour a couple of years back.
Tho, fine for me if you wanna keep it local (at least for now) but I think this approach is actually good and we should use it internally as well
|
Talked with Joao and we feel that an explicit API might still be beneficial, I suggest to talk about it again in the next API sync to get to some conclusion. |
There was a problem hiding this comment.
The async nature of commands is what will kill us here. A command's open context will leak onto the following command, if the first takes a while to complete, which happens much more often than we'd hope. This will create issues of once a week my editor doesn't open in preview, which will be hard to reproduce and straight up impossible to fix.
I'm much more in favor of a solution which requires extension adoption, since only that can give us a fool proof solution, as far as I can see. I don't fear API adoption: given we have such a lively extension ecosystem, adoption will come fast.
This is not impossible because we know which extension has triggered the open-command and we can always install the 'extra open args' for a specific extension only. For this exploration, in the name of simplicity, we decided to skip that but we have all the information at hand |
That will not help for the case which imho is also the most likely one: you see a tree from 1 extension and interact with it multiple times with multiple items in short time frame. Chances are high that if the opening takes long, the context we set is always wrong. |
|
You mean the user clicks fives times before the first open-command is triggered from the extension? A stack would help. The UI could do nothing while the command is still running, e.g not dispatch another event. I don't know but I don't see real road blockers here, sure things get more complex to implement but not impossible. But what I do not share is the optimistic expectation that extension author jump on adopting an API change quickly - if you ask me it's too niche for that. |
|
A stack won't help, since commands don't finish in the reverse order they started executing... Blocking UI events while commands don't complete is even more mind boggling to me. Isn't this just asking for trouble? 🤔 We could potentially block actual important UI interactions just because git show takes 5 seconds to complete which, believe me, happens more often that you'd expect. I really don't buy it. In order to close the |
I don't think that we are on the same page. This has nothing to do with blocking the UI but with sequentialising things (e.g how we do it for save): when running the command for some node is slow then nothing is "blocked" but the next invocation for that exact node waits for the previous to finish. One could argue it's a bug that the UI doesn't show that the command is taking a long time. |
|
Closing, as we don't seem to be pushing on this solution. |
This is a continuation of #109779 specifically for extensions (see #85636 and #101522).
We have 3 built in components that open editors from tree usages:
None of these currently have a way to enforce our list/tree options, specifically wether to:
We discussed this in the API call yesterday and concluded that we should build a solution that does not require any adoption because we know an adoption will not happen for all extensions.
As such, if we keep the context of the user gesture for the duration of the command invocation and enforce it in very specific dedicated places where we know that an editor opens (
vscode.open,vscode.diff), we can solve this for most cases.My first solution was to add this entirely into
mainThreadEditorsbut I felt it is wrong that e.g. custom trees, timeline and changes view depend on it. So now I made this API inIListServiceand want to put it out here to get some initial feedback. A slight variant I had in mind but did not push for is to let theIListServiceitself remember each open event for a specific duration, but that also felt wrong given that we only need this in very few places and not all trees/lists.@joaomoreno mainly asking you for feedback if this direction is OK from the POV of list service and @jrieken for the hook in the extension commands (
vscode.open,vscode.diff)//cc @eamodio @alexr00