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

fix #5506: Make processId and cwd in TerminalWidget return promise when throw error #5553

Merged
merged 1 commit into from
Jul 8, 2019

Conversation

BroKun
Copy link
Member

@BroKun BroKun commented Jun 21, 2019

Add the ready interface to ensure that the correct processId can be obtained.

Signed-off-by: Brokun brokun0128@gmail.com

@BroKun
Copy link
Member Author

BroKun commented Jun 21, 2019

Currently, it works for me.
I think we can look at whether to avoid multiple calls to resolve and whether to update 'ready' when attach.

@@ -203,22 +204,30 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
return this.term;
}

get ready(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

please don't expose it for now, keep protected

@akosyakov
Copy link
Member

@BroKun How do you test it? Code-wise it looks good.

@akosyakov akosyakov added vscode issues related to VSCode compatibility terminal issues related to the terminal labels Jul 4, 2019
@BroKun BroKun force-pushed the GH-5506 branch 4 times, most recently from d25d11d to e2bff98 Compare July 8, 2019 05:32
…rn promise when throw error

Signed-off-by: Brokun <brokun0128@gmail.com>
@BroKun BroKun changed the title fix #5506: Add the ready interface to ensure that the correct process… fix #5506: Make processId and cwd in TerminalWidget return promise when throw error Jul 8, 2019
@BroKun
Copy link
Member Author

BroKun commented Jul 8, 2019

@BroKun How do you test it? Code-wise it looks good.

I modified the solution to this problem because I noticed that onDidOpen can fix the exceptions caused by the call sequence. However, since processId and cwd cannot be captured by reject when an exception is thrown, it causes a problem.

As for testing, I feel that the current solution does not require special testing. The problem scenario can be observed by adding a wait time before the start method.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

oh, really good catch with async code!

@akosyakov
Copy link
Member

As for testing, I feel that the current solution does not require special testing. The problem scenario can be observed by adding a wait time before the start method.

It is better to be safe and double check.

@akosyakov akosyakov merged commit 7fe439a into eclipse-theia:master Jul 8, 2019
@BroKun BroKun deleted the GH-5506 branch July 8, 2019 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants