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

CHE-3896 add the needed methods for Webview API #4892

Merged
merged 1 commit into from
Apr 24, 2019
Merged

CHE-3896 add the needed methods for Webview API #4892

merged 1 commit into from
Apr 24, 2019

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Apr 12, 2019

Add the needed methods for Webview API.

How to test these changes with a test webview-sample:

  1. With Theia https://youtu.be/aN6MrDkkQMs
  2. With VS Code https://youtu.be/Mh3I6ypdSQU

#3896

Signed-off-by: Oleksii Orel oorel@redhat.com

@akosyakov
Copy link
Member

akosyakov commented Apr 15, 2019

@olexii4 please think how we will test it, e.g. provide a sample VS Code extension, play with it in VS Code to learn requirements, describe what to do to verify that APIs work in different scenarios, make the screencast showing it

@akosyakov akosyakov self-requested a review April 15, 2019 05:32
@olexii4 olexii4 force-pushed the CHE-3896 branch 2 times, most recently from fd9f72f to 575b75e Compare April 17, 2019 07:33
@olexii4
Copy link
Contributor Author

olexii4 commented Apr 17, 2019

The test plugin
https://github.com/olexii4/webview-sample.git

I am working on the video.

@olexii4 olexii4 marked this pull request as ready for review April 17, 2019 07:57
@olexii4 olexii4 force-pushed the CHE-3896 branch 2 times, most recently from 7f526bb to a0c2a07 Compare April 18, 2019 11:32
@olexii4
Copy link
Contributor Author

olexii4 commented Apr 18, 2019

https://youtu.be/aN6MrDkkQMs
https://youtu.be/Mh3I6ypdSQU

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.

It does not really work for me: Open 3 files in the same tab group and then try to use the sample extension to open in 3rd view position. It opens in the same group, not even trying to create a new one.

view

@olexii4
Copy link
Contributor Author

olexii4 commented Apr 23, 2019

It does not really work for me: Open 3 files in the same tab group and then try to use the sample extension to open in 3rd view position. It opens in the same group, not even trying to create a new one.

view

I tried to implement the same behavior as in VSCode. But I can fix it

@akosyakov
Copy link
Member

I tried to implement the same behavior as in VSCode. But I can fix it

let me try the same in VS Code

@akosyakov
Copy link
Member

akosyakov commented Apr 23, 2019

@olexii4 in the same VS Code it is based on editor groups (tab groups in Theia), not individual editors like in this PR.

See:
vscode_view

Here 3 editors in first group and I'm trying to open the webview in 3rd group, so VS Code opens a new group to right.

@olexii4
Copy link
Contributor Author

olexii4 commented Apr 23, 2019

@akosyakov Now I see. Previously I tested VS Code with two editor groups (tab groups) and behavior was different:
https://youtu.be/y7K6Pwdsoqw
Ok. I will fix it

@akosyakov
Copy link
Member

@olexii4 It looks like a bug to me. It only happens if the first group has a focus, if you remove a focus (select a navigator) it will open a new group in VS Code as well. Could you file an issue against VS Code to ask whether it is a bug, no need to point to this PR? Maybe we are overlooking something.

@olexii4
Copy link
Contributor Author

olexii4 commented Apr 23, 2019

@akosyakov Totally agree with you. It is like a bug for me too.
Yes, I could file an issue against VS Code to ask whether it is a bug.

@olexii4
Copy link
Contributor Author

olexii4 commented Apr 23, 2019

@akosyakov WDYT?

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.

It works good now 👍

Please have a look at comment. I have concerns that it won't work with multiple webviews.

packages/plugin-ext/src/main/browser/webviews-main.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/webviews-main.ts Outdated Show resolved Hide resolved
Signed-off-by: Oleksii Orel <oorel@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants