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

Use 'viewId' as progress location #8700

Merged
merged 1 commit into from
Nov 12, 2020
Merged

Use 'viewId' as progress location #8700

merged 1 commit into from
Nov 12, 2020

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Nov 2, 2020

What it does

Fixes #8703

At the moment it's possible to use ProgressLocation enum only as a location at which progress should show.
The PR is aligning location field to the corresponding field of VS Code to provide an opportunity to use viewId as progress location.

How to test

I created a test plugin, it contains commands for testing

  1. Please clone the plugin and build it using yarn.
  2. Copy the plugin to theia/plugins folder.
  3. Use the following commands from Command Palette (F1) for testing:
  • Start progress for Explorer view
  • Start progress for Search view
  • Start progress for Debug view
  • Start progress for SCM view

First 3 commands use viewId (new behavior) to show progress while running the given callback, the last command uses ProgressLocation enum to show progress for SCM view (existed behavior).

progress_API

  1. Use case for a custom view is described here Use 'viewId' as progress location #8700 (comment) - thanks a lot @kittaakos for the case and the extension for testing

Review checklist

Reminder for reviewers

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

@RomanNikitenko RomanNikitenko marked this pull request as ready for review November 3, 2020 07:30
Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

Tested 4 commands with the provided plug-ins
I see progress as expected on 4 examples

image

More compliance with VS Code 🎉

@kittaakos
Copy link
Contributor

I also give it a try...

@kittaakos
Copy link
Contributor

I also give it a try...

This does not seem to work with a custom view inside a custom view container. I will try to come up with a public, stripped-down example later today. Is this urgent to merge, @RomanNikitenko?

@RomanNikitenko
Copy link
Contributor Author

This does not seem to work with a custom view inside a custom view container. I will try to come up with a public, stripped-down example later today. Is this urgent to merge, @RomanNikitenko?

I tested the solution for explorer, debug, search and scm views only, so I didn't face the problem related to a custom view inside a custom view container. Please provide steps to reproduce or an example for testing.

About

Is this urgent to merge

I provided these changes within another issue. If it works for main views and doesn't work for some specific case - I think we could merge the PR and create a separate issue for the specific case to have an ability investigate this area more deeply.
WDYT?

Anyway I'm open for discussion - if there is the opinion that we can not merge the PR as is - then the solution should be improved before merge.

@kittaakos
Copy link
Contributor

I tested the solution for explorer, debug, search and scm views only, so I didn't face the problem related to a custom view inside a custom view container. Please provide steps to reproduce or an example for testing.

Sure, I will. All cases should work after merging this PR.

I provided these changes within another issue. If it works for main views and doesn't work for some specific case - I think we could merge the PR and create a separate issue for the specific case to have an ability investigate this area more deeply.
WDYT?

No, I don't see it this way. Although nobody asked me for a review here, I spent my time investigating the VS Code source and proposed a reasonable code change. I tried your modification in-action; it did not work. Now I am creating a VS Code example for you. I do not want to come back for another follow-up PR review. This should be solved with this PR. Thank you for your understanding, and I am hoping to see this change in Theia ASAP. 👍

@kittaakos
Copy link
Contributor

Please provide steps to reproduce or an example for testing.

Here is the repo with the example, you can build one and try it.

In VS Code, you open the new view container, it has one view (it shows your user home) and you execute the Do Something command. It should show progress for two seconds:

screencast 2020-11-03 13-03-37

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Nov 3, 2020

No, I don't see it this way.

OK, As I mentioned above:

Anyway I'm open for discussion - if there is the opinion that we can not merge the PR as is - then the solution should be improved before merge.

===================================

Although nobody asked me for a review here

But it's not true :-)
review

I spent my time investigating the VS Code source and proposed a reasonable code change.

Now I am creating a VS Code example for you.

thanks a lot!

@kittaakos
Copy link
Contributor

But it's not true :-)

I did not see the review request, sorry 😊. I just wanted to try the new feature.

@RomanNikitenko RomanNikitenko marked this pull request as draft November 3, 2020 13:24
@RomanNikitenko
Copy link
Contributor Author

@kittaakos
I tried your example - cloned and built it, then copied to the theia/plugins folder.
Unfortunately I got the error related to an activation the extension.

viewId

Could you take a look what's wrong in my steps?

@kittaakos
Copy link
Contributor

Could you take a look what's wrong in my steps?

I made some changes and verified it, it should work now. Thanks, @RomanNikitenko 🙏

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@RomanNikitenko
Copy link
Contributor Author

@kittaakos
could you review when you have a chance

progress_test_view

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 with a custom view, it works. Thank you, @RomanNikitenko 👍

@RomanNikitenko RomanNikitenko merged commit 5c164f7 into master Nov 12, 2020
@RomanNikitenko RomanNikitenko deleted the progressLocation branch November 12, 2020 09:25
@github-actions github-actions bot added this to the 1.8.0 milestone Nov 12, 2020
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.

Align location field of ProgressOptions with VS Code
3 participants