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.

Sign up for GitHub

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

Jump to bottom

[plugin] don't fail if a command returns not serializable result or error #7957

Merged
merged 2 commits into from
Jun 4, 2020
Merged

[plugin] don't fail if a command returns not serializable result or error #7957

merged 2 commits into from
Jun 4, 2020

Conversation

akosyakov
Copy link
Member

What it does

How to test

  • try to use Find All References from the editor context menu for the typescript file. References view should be revealed.

Review checklist

Reminder for reviewers

@akosyakov
[plugin] fix #7955: don't fail if a command returns not serializable …
2973eee 
…result or error

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jun 4, 2020
RomanNikitenko
RomanNikitenko reviewed Jun 4, 2020
View reviewed changes
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

@akosyakov
I tested a little and still have some problems related to Find All References
Sometimes it works well, but sometimes the behavior is the same as described in the issue.
For example, it doesn't work for me after closing References, please see the short video

find_all_references_bug

I don't know - maybe I faced with another issue...

@akosyakov akosyakov mentioned this pull request Jun 4, 2020
Closed
1 task
kittaakos
kittaakos approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have verified it in Gitpod, I do not see the error with the changes from the PR. 👍 The References view opens.

Few remarks:

  • the References view opens and works from master too despite the serialization error.
  • the References view integration is not so nice, I thought we reveal the view just as VS Code does. Apparently, we just open it but do not reveal it. (See the attached screencast)

screencast 2020-06-04 14-21-19

packages/plugin-ext/src/common/rpc-protocol.ts
function safeStringify(obj: any, replacer?: JSONStringifyReplacer): string {
try {
return JSON.stringify(obj, replacer);
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit err.

Copy link
Member Author

Choose a reason for hiding this comment

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

or leave it for debugging purposes :)

@vzhukovs
Copy link
Contributor

vzhukovs commented Jun 4, 2020

Checked whether this PR fixes #7853 as alternative of #7959. Everything works well:

focus_on_explorer

@akosyakov as I understand, this PR #7959 is redundant now?

@akosyakov
Copy link
Member Author

this PR #7959 is redundant now?

yes i think it is covered

@akosyakov
Copy link
Member Author

akosyakov commented Jun 4, 2020 •
edited
Loading

@RomanNikitenko I could reproduce your issue with closing the view. I've pushed another commit. There was a race. We tried to focus the view before it was completely registered in the shell. Now the widget manager awaits when the view widget is registered before announcing it as created. Could you check again please?

@akosyakov akosyakov mentioned this pull request Jun 4, 2020
Closed
5 tasks
@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jun 4, 2020 •
edited
Loading

Could you check again please?

@akosyakov
maybe something wrong with my assembly, but with your last changes I have just empty references view
references_bug

@vzhukovskii could you check this use case on your side please.

@vzhukovs
Copy link
Contributor

vzhukovs commented Jun 4, 2020

@vzhukovskii could you check this use case on your side please.

vscode-didact — Theia Browser Example 2020-06-04 16-32-23

The same behavior as you have

@akosyakov
Copy link
Member Author

My bad I used widget.disposed which is a signal, should use widget.isDisposed 🤦 good catch!

@akosyakov
[plugin] ensure that views are prepared before activating them
9a19f1c 
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the akosyakov/references-references-7955 branch from 5525057 to 9a19f1c Compare June 4, 2020 13:54
@akosyakov
Copy link
Member Author

@RomanNikitenko @vzhukovskii I've pushed again.

@vzhukovs
Copy link
Contributor

vzhukovs commented Jun 4, 2020

Checked, it works! 💪

vscode-didact — Theia Browser Example 2020-06-04 17-08-19

@RomanNikitenko RomanNikitenko self-requested a review June 4, 2020 14:13
RomanNikitenko
RomanNikitenko approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Tested after update - it works well for me!
thanks!

@akosyakov akosyakov merged commit fdd7c41 into master Jun 4, 2020
@akosyakov akosyakov deleted the akosyakov/references-references-7955 branch June 4, 2020 15:04
@vzhukovs vzhukovs mentioned this pull request Jun 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomanNikitenko RomanNikitenko RomanNikitenko approved these changes

@vzhukovs vzhukovs vzhukovs approved these changes

@kittaakos kittaakos kittaakos approved these changes

@vince-fugnitto vince-fugnitto Awaiting requested review from vince-fugnitto

Assignees
No one assigned
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[references] References view does not open
4 participants
@akosyakov @vzhukovs @RomanNikitenko @kittaakos