-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve vscode tab API #13730
Improve vscode tab API #13730
Conversation
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden do we have a list of issues this PR fixes? |
I think its just the List above with these three points. Are you still seeing Problems or discrepancies somewhere? |
It can easily be that i overlooked problems. Its sadly super hard to align this perfectly to vscode because phosphor is just doing things differently to vscodes layouting. Like events just happening in different order |
Also, we should try to remove the workaround in the tests I introduced for the 1.88.1 built-ins. #13679 (comment) |
Removing the workaround for #13679 makes the tests fail again. Seems that one at least is still there. |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
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.
Otherwise, looks good to me.
@@ -103,7 +103,6 @@ describe('TypeScript', function () { | |||
const editorWidget = widget instanceof EditorWidget ? widget : undefined; | |||
const editor = MonacoEditor.get(editorWidget); | |||
assert.isDefined(editor); | |||
await timeout(1000); // workaround for https://github.com/eclipse-theia/theia/issues/13679 |
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.
This is not the workaround: the workaround is not waiting for the context key typescrip.isManagedFile
.
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.
Also, I need the 1 second timeout for the tests to run reliably, locally.
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.
The typescript.isManagedFile
context key is working unreliably. When opening a TypeScript file in the editor and querying the context key value, I always get false
, even though a TypeScript editor is open (and focused).
I've reinstated the 1 second timeout, as the issue seems to be unrelated to the tab API.
@tsmaeder FYI the latest commit fixes an error in the plugin host that appeared due to an off-by-one error. Note that the |
@msujew we should keep the issue reference, since adding the one second timeout is necessary as long as setting the context does not work reliably. Otherwise, this PR looks fine as far as it goes. Unfortunately, |
FYI I believe I've mostly fixed #13679, it's just a minor issue left: The The original off-by-one issue mentioned in the issue is fixed, it's just a matter of correctly ordering the events. |
What it does
This improves and fixes some things for the vscode tab api.
How to test
Here is a small test extension.
This shows message and logs into the console the different tabApi events.
Follow-ups
Review checklist
Reminder for reviewers