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

Remove Terminal.onDidWriteData #80836

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Remove Terminal.onDidWriteData #80836

merged 2 commits into from
Sep 13, 2019

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Sep 13, 2019

Replaced by vscode.window.onDidWriteTerminalData

Fixes #78574

Replaced by vscode.window.onDidWriteTerminalData

Fixes #78574
@Tyriar Tyriar added this to the September 2019 milestone Sep 13, 2019
@Tyriar Tyriar requested a review from weinand September 13, 2019 02:11
@Tyriar Tyriar self-assigned this Sep 13, 2019
this.detectPattern(s);
}));
});
this.disposables.push(vscode.window.onDidWriteTerminalData(e => {
Copy link
Member Author

@Tyriar Tyriar Sep 13, 2019

Choose a reason for hiding this comment

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

@weinand fyi debug-server-ready was on a deprecated API that just got removed, I didn't test this so please review.

@Tyriar Tyriar merged commit 3979479 into master Sep 13, 2019
@Tyriar Tyriar deleted the tyriar/78574 branch September 13, 2019 04:15
@weinand
Copy link
Contributor

weinand commented Sep 13, 2019

@Tyriar the serverReadyAction feature no longer works in the integrated terminal since the package.json is missing a "enableProposedApi": true.

I addition I will have to rewrite the code because the old implementation tried to be very careful to register onDidWriteData only for those terminals that actually use the feature. With the new API this is no longer possible as onDidWriteTerminalData gets all data for all terminals and the filtering has to be done after the fact.

I'm not sure that the new API is an improvement. It is definitely not performance-wise.

/cc @jrieken

@jrieken
Copy link
Member

jrieken commented Sep 13, 2019

new API is an improvement. It is definitely not performance-wise.

That's on purpose. For once to align with the global event rules and then the VSLS use-case is to listen on all terminal anyways, hence no performance difference. Tho, because we are aware that (global and private events are equally dangerous) we have no plans to ever move that API out of proposed.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 13, 2019

@weinand no longer works? That was never stable so how did it ever work? @jrieken and I have spoken at length about performance and security and that's why the plan is it leave it as proposed.

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.

Remove deprecated Terminal.onDidWriteData extension API
3 participants