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

scm/git UI fixes #5254

Merged
merged 1 commit into from
May 29, 2019
Merged

scm/git UI fixes #5254

merged 1 commit into from
May 29, 2019

Conversation

@akosyakov
Copy link
Member

Let's keep this PR to recover all lost changes in one commit that we don't create too much noise with fixing what already used to work. I'm going to go through git extension history starting from Jan now and list commits which should be recovered here. Hope is not much.

@vinokurig
Copy link
Contributor Author

@akosyakov OK, merged previous PRs

@akosyakov
Copy link
Member

@vinokurig ok, please close other PRs then, I am half through changes, will check/test the rest after the lunch. I open issues for everything which i find and will retest them against this PR after analyzing commits.

@akosyakov
Copy link
Member

akosyakov commented May 27, 2019

@vinokurig I went through all changes from Jan and updated a comment with referenced issues. I'm starting testing and will mark verified as done.

@akosyakov
Copy link
Member

@vinokurig please try to toggle SCM several times:
togglescm

icon and widget caption are not rendered after toggling couple times

@akosyakov
Copy link
Member

@vinokurig https://github.com/theia-ide/theia/pull/5254/files#diff-a2eb5b2bcd08435891fb7969b75c9fccL119 looks bogus, it means if you move git widget to any other area except left updating title won't work anymore.

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.

I've verified existing issues and commented at the code which looks suspicious. Let me know if you need my help to resolve remaining. I hope we can fix them and merge tomorrow.

@vinokurig
Copy link
Contributor Author

vinokurig commented May 27, 2019

@akosyakov

@vinokurig please try to toggle SCM several times:
icon and widget caption are not rendered after toggling couple times

Fixed

@vinokurig
Copy link
Contributor Author

@akosyakov

@vinokurig https://github.com/theia-ide/theia/pull/5254/files#diff-a2eb5b2bcd08435891fb7969b75c9fccL119 looks bogus, it means if you move git widget to any other area except left updating title won't work anymore.

Fixed

@akosyakov
Copy link
Member

akosyakov commented May 28, 2019

@vinokurig everything seems to work now, except #3800 removed repositories are not detected, i would expect that there should be an event for onDidChangeSelectedRepositories with undefined but it does not happen

@akosyakov
Copy link
Member

@vinokurig are you looking into #3800? i can do if you don't have time, we need to resolve and merge it today

@akosyakov akosyakov changed the title Fix Git open changes icon scm/git UI fixes May 28, 2019
@vinokurig
Copy link
Contributor Author

vinokurig commented May 28, 2019

@akosyakov

@vinokurig are you looking into #3800? i can do if you don't have time, we need to resolve and merge it today

I am working on it right now

@vinokurig
Copy link
Contributor Author

@akosyakov
There is GitRepositoryProvider that provides Git specific repositories. The selected repository in GitRepositoryProvider is set by SCM service. To get rid of GitRepositoryProvider it is needed to completely rework Git extension and other related extensions. It looks like very big part of work.
I agree that firing repository deleted event from Git extension is not good, but I can't detect deletion of projects folder in the Scm because File watcher event returns only an array of deleted files.

@akosyakov
Copy link
Member

akosyakov commented May 28, 2019

I don't mean to remove GitRepositoryProvider but make it git specific facade of ScmService. Handling of ScmProvider also should be moved to GitRepositoryProvider instead of keeping allRepositories.

ScmService should be a single source of the whole scm state, git extension should populate it and provide git specific view instead of duplicating this state partially and trying to sync it.

Ok, let's open an issue to refactor it.

@akosyakov
Copy link
Member

Have you managed to track down #5254 (comment)?

Why status bar elements are not removed.

@vinokurig
Copy link
Contributor Author

vinokurig commented May 28, 2019

Have you managed to track down #5254 (comment)?
Why status bar elements are not removed.

working on it now

@akosyakov
Copy link
Member

Here https://github.com/theia-ide/theia/blob/59fdb74937de03764ec9cdb6255dbcd7832c55a2/packages/git/src/browser/git-contribution.ts#L274-L288

I wonder whether we should reset to undefined selected repository for git extension as well. Otherwise it keeps a reference to a repository which is not selected anymore. Maybe it is a reason.

@vinokurig
Copy link
Contributor Author

@akosyakov Fixed icon cleanup

@vinokurig
Copy link
Contributor Author

@akosyakov any updates?

@akosyakov
Copy link
Member

now it cannot detect when a new repository is cloned after removing

Screen Shot 2019-05-28 at 16 16 47

Strangely it is detected in the status bar, but not in SCM view. Order of items also is bogus.

Also if you have a folder opened with 2 repos and delete one. The status bar item to pick a repo should disappear and appear when this repo is cloned again. It was working nicely before.

@vinokurig
Copy link
Contributor Author

@akosyakov Is it ok to fix it tomorrow?

@akosyakov
Copy link
Member

@vinokurig yes @marcdumais-work said that we can wait till SCM is cleaned up

To show how it used to work:
beforescm

@akosyakov
Copy link
Member

@vinokurig after testing more previous state: it is fine that new repositories cannot be detected without explicit refresh, but then status bar should not snow bogus elements like in #5254 (comment)

With explicit refresh with multiple git repositories, all new repositories should be detected.

@vinokurig
Copy link
Contributor Author

@akosyakov Fixed icons order

@akosyakov
Copy link
Member

@vinokurig ok, testing, are you looking into detection addition/deletion repositories on refresh?

@vinokurig
Copy link
Contributor Author

@vinokurig ok, testing, are you looking into detection addition/deletion repositories on refresh?

yes

@akosyakov
Copy link
Member

akosyakov commented May 29, 2019

When I delete the repo it still shows a branch picker in the status bar:
Screen Shot 2019-05-29 at 09 28 21

and when i clone it again, it shows SCM elements in status bar, although it should not show anything (i did not trigger refresh):
Screen Shot 2019-05-29 at 09 29 29

For right now i see state is duplicated several times:

  • GitContribution.dirtyResporitories duplicating GitRepositoryProvider.allRepositories which duplicating ScmService.repositories
  • and what discussed yesterday GitRepositoryProvider.selectedRepository duplicating ScmService.selectedRepository

I suspect deleted repository is still cached somewhere.

@vinokurig
Copy link
Contributor Author

vinokurig commented May 29, 2019

@akosyakov detection addition/deletion repositories is fixed

When I delete the repo it still shows a branch picker in the status bar:

fixed

@vinokurig
Copy link
Contributor Author

@akosyakov

when i clone it again, it shows SCM elements in status bar, although it should show anything (i did not trigger refresh

couldn't reproduce, maybe it is fixed with the latest commits

@akosyakov
Copy link
Member

@vinokurig thank you for all effort! it works good now. Please squash commits, we are waiting for travis to be green then.

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
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.

please merge, thank you again

@vinokurig vinokurig merged commit afc3499 into master May 29, 2019
@vinokurig vinokurig deleted the theia-5245 branch May 29, 2019 09:07
@marcdumais-work
Copy link
Contributor

@akosyakov So if I understand correctly, this is not blocking the 0.7.0 release any more?

@akosyakov
Copy link
Member

@marcdumais-work yes, it should be good for 0.7.0

@akosyakov
Copy link
Member

@vinokurig Could you have a look at #5300? i've missed during testing :(

@vinokurig
Copy link
Contributor Author

@akosyakov yes, working on it

@vinokurig
Copy link
Contributor Author

@akosyakov

@vinokurig Could you have a look at #5300? i've missed during testing :(

fixed

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.

3 participants