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

Apply FileDecoration provider API #8911

Merged
merged 1 commit into from
May 17, 2021
Merged

Apply FileDecoration provider API #8911

merged 1 commit into from
May 17, 2021

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Dec 29, 2020

What it does

Align the FileDecoration vscode Plugin API with the latest vscode version. This API was changed and it is used in the vscode git plugin, so to be able to use the latest vscode git plugin it is needed to apply the changes in the Theia's FileDecoration plugin API.

related CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22921

How to test

  1. Remove "@theia/git": "^1.9.0" from examples/browser/package.json file.
  2. Download vscode-git-1.52.1 and copy it to the plugins folder.
  3. Start Theia and open a project cloned from GitHub.
  4. Make some changes in the project and check them in the SCM panel:
    screenshot-localhost_3000-2020 12 29-17_06_55

Review checklist

Reminder for reviewers

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

If I understand correctly, we should check if everything works the same as in master branch.
If so, it works as usual on my side.

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I've tested it according to the provided "How to test" section. The file decorations, in Git view, seems to work in the same way as in VS Code.
image

Except for the tooltips. I don't see the file tooltips as it's in VS Code.
I'm not sure if it's a bug or just not implemented feature yet.
@vinokurig could you check the tooltips, please?

@vinokurig
Copy link
Contributor Author

vinokurig commented Feb 3, 2021

@azatsarynnyy

Except for the tooltips. I don't see the file tooltips as it's in VS Code.
I'm not sure if it's a bug or just not implemented feature yet.

There are no tooltips in the master version neither for theia's native git plugin, nor for vscode git plugin. I've created an issue to add them: #9020

@azatsarynnyy
Copy link
Member

There are no tooltips in the master version neither for theia's native git plugin, nor for vscode git plugin. I've created an issue to add them: #9020

I see. Thank you for the explanation!

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

The functionality works as expected and code-wise it looks good to me. Thanks!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@vinokurig I performed a functional test and I noticed the following issues:

  • I no longer see decorations for scm in the navigator:

image

  • I see a lot of the file-change-tree-root errors which grows constantly
    image

@vinokurig
Copy link
Contributor Author

@vince-fugnitto

I no longer see decorations for scm in the navigator:

The navigator scm decorations has been fixed.

I see a lot of the file-change-tree-root errors which grows constantly

A pull-request that fixes this issue is opened: #9045

@ericwill ericwill mentioned this pull request Mar 2, 2021
46 tasks
@vinokurig vinokurig force-pushed the che-18661 branch 2 times, most recently from 2838ed0 to f56516b Compare March 3, 2021 09:39
@vinokurig
Copy link
Contributor Author

@vince-fugnitto any updates?

@vince-fugnitto
Copy link
Member

@vince-fugnitto any updates?

@vinokurig I haven't had a chance to re-review the pull-request, there are a couple of pull-requests currently opened that also need reviewing. If others can help it'd be appreciated.

@vince-fugnitto vince-fugnitto added git issues related to git scm issues related to the source control manager vscode issues related to VSCode compatibility labels Mar 4, 2021
@vince-fugnitto
Copy link
Member

@vinokurig I confirmed that the file decorations now work with/without the vscode git plugin.
I encountered an issue when using an older version of the plugin (1.45.1), is it expected that it no longer works? If so we should probably document that older versions of the plugin are now broken and should be updated.

image

@vinokurig
Copy link
Contributor Author

@vince-fugnitto

I encountered an issue when using an older version of the plugin (1.45.1), is it expected that it no longer works?

Yes it is expected, the API was changed after 1.49.3 version.

If so we should probably document that older versions of the plugin are now broken and should be updated.

You mean add a note to the CHANGELOG.md file?

@vince-fugnitto
Copy link
Member

If so we should probably document that older versions of the plugin are now broken and should be updated.

You mean add a note to the CHANGELOG.md file?

@vinokurig I think so, there are a few apps which will require an update to the builtin to work properly again, and it might be better to be upfront or transparent about older versions of the plugin not working anymore.

In addition, do you believe we should aim to merge #9045 first, the following pull-request introduces changes which results in multiple errors with file-change-root-node which are not present with the same degree on master.

I believe this is an issue:

image

and it grows constantly:

image

@vinokurig
Copy link
Contributor Author

In addition, do you believe we should aim to merge #9045 first, the following pull-request introduces changes which results in multiple errors with file-change-root-node which are not present with the same degree on master.

I agree, so could you please review #9045 as well. There is a discussion around logging an event when a new tree root is set in that PR.

@vinokurig
Copy link
Contributor Author

Depends on #8969.
VScode git plugin v 1.52.1 has (Uri as any).joinPath: https://github.com/microsoft/vscode/blob/ea3859d4ba2f3e577a159bc91e3074c5d85c0523/extensions/git/src/emoji.ts#L27 but joinPath has been added only in the later versions of the monaco-editor-core. This is important because current version of the vscode git plugin won't work after this PR, and newer versions won't work as well because of the missing joinPath function.

@RomanNikitenko
Copy link
Contributor

@vinokurig
about joinPath
FYI: #9199

@ericwill ericwill mentioned this pull request Mar 18, 2021
46 tasks
@vinokurig
Copy link
Contributor Author

@vince-fugnitto Since the joinPath issue has been resolved I updated this PR with master and now it works as expected. Could you please review it?

@vince-fugnitto vince-fugnitto self-requested a review May 12, 2021 15:57
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I performed a functional review and everything seems to work correctly 👍

I verified that the use-case described in the pull-request works for both browser and electron (scm decorations are present in the nav, and scm view). I also confirmed that using @theia/git works like on master, decorations are present.

@vince-fugnitto vince-fugnitto dismissed their stale review May 12, 2021 15:58

Review is stale.

@vinokurig vinokurig force-pushed the che-18661 branch 2 times, most recently from 653a951 to e3c430c Compare May 17, 2021 07:12
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git issues related to git scm issues related to the source control manager vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants