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

Introduce SCM Plugin-Api #4279

Merged
merged 4 commits into from
May 24, 2019
Merged

Introduce SCM Plugin-Api #4279

merged 4 commits into from
May 24, 2019

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Feb 7, 2019

This PR contains:

  • SCM Plugin API methods
  • SCM Service witch handles registered providers and fires related events. This service is designed to handle scm menu contributions (support scm menu contributions #4171)
  • Git toolbar items are handled by SCM Service.

#4103, #4666, #4266
VsCode Git Extension integration:
ezgif-1-729b91e580a8

@benoitf
Copy link
Contributor

benoitf commented Feb 7, 2019

hello @vinokurig
Which SCM VSCode extension do you have used to check SCM ?

It may help people to know which one you used and how it is working, etc.

@@ -0,0 +1,187 @@
/********************************************************************************
* Copyright (C) 2018 Red Hat, Inc. and others.
Copy link
Member

@akosyakov akosyakov Feb 8, 2019

Choose a reason for hiding this comment

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

2019, please review copyrights for new filws

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

packages/scm/src/browser/scmService.ts Outdated Show resolved Hide resolved
@akosyakov
Copy link
Member

Should not SCM extension provide SCM UI and not rely on git extension for it?

@vinokurig
Copy link
Contributor Author

@akosyakov It should provide UI, I think this is going to be done in the scope of #4171. The Scm Service doesn't relay on git extension, Git sends events to Scm Service.

@benoitf The Api is needed for VsCode Git extension (eclipse-che/che#4266) but since there is no UI part for SCM Service yet, it is hard to test it in the current state.

@kittaakos
Copy link
Contributor

Reviewing...

@akosyakov
Copy link
Member

#4171 is just about menu contributions

the description of #4103 says:

We need to extract Git-agnostic UI to a new extension. This extension should provide APIs similar to VS Code. Git extension has to be updated to use this API.

This PR does not seem to implement it yet. I'm fine doing migration via several PRs though. The issue should be resolved when git extension is completely refactored to work via scm extension.

examples/browser/package.json Outdated Show resolved Hide resolved
packages/git/package.json Outdated Show resolved Hide resolved
packages/git/src/browser/git-view-contribution.ts Outdated Show resolved Hide resolved
packages/git/src/browser/git-view-contribution.ts Outdated Show resolved Hide resolved
packages/git/src/browser/git-view-contribution.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/scm-service-impl.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/scm-service-impl.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/scm-service-impl.ts Outdated Show resolved Hide resolved
packages/scm/src/browser/scm-service-impl.ts Outdated Show resolved Hide resolved
packages/scm/src/common/scm.ts Outdated Show resolved Hide resolved
@vinokurig
Copy link
Contributor Author

@akosyakov
#4171 is just about menu contributions

the description of #4103 says:

We need to extract Git-agnostic UI to a new extension. This extension should provide APIs similar to VS Code. Git extension has to be updated to use this API.

This PR does not seem to implement it yet. I'm fine doing migration via several PRs though. The issue should be resolved when git extension is completely refactored to work via scm extension.

Ok, I am going to continue with UI, but I think this can be merged as a part 1.

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.

I'm fine, waiting approval from @kittaakos as he did a review

@kittaakos
Copy link
Contributor

Thanks for the reminder, @benoitf. I will look into the changes once more when the DCO check and the builds are fine.

@vinokurig vinokurig force-pushed the theia-4103 branch 3 times, most recently from 20a5ba6 to 7f27542 Compare February 19, 2019 11:28
@vinokurig
Copy link
Contributor Author

@kittaakos The tests passed locally but they failed at travis-ci job

@kittaakos
Copy link
Contributor

Thank you for your patience, @vinokurig! I have looked into your changes, tried it locally with multiple repositories. It works, however, when I have three repositories in my workspace when I do a browser refresh and I take the heap dumb, I can see nine ScmRepositoryImpl instances in the memory. Presumably, it should be three. Please take a look.

screen shot 2019-02-20 at 10 01 46

@vinokurig
Copy link
Contributor Author

Sorry, couldn't reproduce, I got the same result after refreshing three times:
screenshot from 2019-02-20 14-02-46

@kittaakos
Copy link
Contributor

Thanks for checking, I will try once more.

@kittaakos
Copy link
Contributor

I could. Here are the steps:

  • Open Theia in a WS.
  • Clone any repository (R1) into the workspace.
  • Clone a second repository (R2) into the workspace.
  • Refresh the browser.
  • Activate the Git widget.
  • Make sure you have activated all three repositories (R1, R2, and theia) from the status bar.
  • Refresh the browser.
  • Create the heap dump. (From this point after each refresh, I have three new instances.)

I hope this helps to fix the leak.

screencast 2019-02-20 14-34-04

@vinokurig
Copy link
Contributor Author

It is strange, but I couldn't reproduce your scenario, have you pulled the latest version?

@kittaakos
Copy link
Contributor

have you pulled the latest version?

Yes.

@kittaakos
Copy link
Contributor

I'll give it a try once more...

@vinokurig
Copy link
Contributor Author

I'll record a video of my steps ASAP

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
@vinokurig
Copy link
Contributor Author

@benoitf done

@benoitf
Copy link
Contributor

benoitf commented May 21, 2019

@svenefftinge is it ok on your side to merge this PR ?

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Yes, sorry fr the delay. I thought it was already merged.

@vinokurig vinokurig merged commit c0a4970 into master May 24, 2019
@vinokurig vinokurig deleted the theia-4103 branch May 24, 2019 06:33
@akosyakov
Copy link
Member

akosyakov commented May 24, 2019

@vinokurig you did not address my comment #4279 (comment) @svenefftinge this PR lost changes from master, we have to revert or fix it asap

@akosyakov
Copy link
Member

This is Git VsCode Extension and it has the same icon:

I was not taliknig about VS Code exteniosn, but theia Git extenison. It is bogus now on master. Also you have not fixed menu items in the main menu.

@@ -33,9 +33,8 @@ before(() => {
});

describe('theia left panel', () => {
it("should show 'Explorer' and 'Git'", () => {
it("should show 'Explorer'", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vinokurig, indeed. but why?

what's the reason to not show SCM?

@AlexTugarev AlexTugarev mentioned this pull request May 24, 2019
@akosyakov
Copy link
Member

akosyakov commented May 24, 2019

I've randomly picked a commit from history for Git extension and it's turned out to be lost as well:
d334126#diff-dca13dfc45fedafbb7c0b5a3f5bf180c

@vinokurig please analyze changes to git extension done since you've started working on it and make sure that everything is recovered.

@AlexTugarev
Copy link
Contributor

@vinokurig, as pointed out by @akosyakov, there are missing changes. we should take a step back, revert this PR and improve it.

@AlexTugarev AlexTugarev restored the theia-4103 branch May 24, 2019 11:23
@vinokurig
Copy link
Contributor Author

Ok, I'll create a new PR with the changes

@AlexTugarev
Copy link
Contributor

Ok, I'll create a new PR with the changes

could you clarify? would you revert for now?

@vinokurig
Copy link
Contributor Author

Is it possible to keep it and provide fixes ASAP?

@akosyakov
Copy link
Member

akosyakov commented May 24, 2019

@vinokurig it has to be fixed beginning next week, since there are products and example apps relying on next version of git extension. I will help with analysis Monday morning, depending on how much is lost, we decide whether we revert or fix it on the same day.

if @AlexTugarev @svenefftinge are not objecting, if you need git extension working as before today, then go ahead and revert.

@vinokurig
Copy link
Contributor Author

@akosyakov @AlexTugarev I've opened a PRs with fixes #5254, #5255, #5256, could you please tkae a look. Can we avoid revert SCM with those fixes?

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.

10 participants