Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Nov 1, 2018

What this PR does

TeamExplorerServiceHolder has picked up responsibilities beyond holding services for the Team Explorer views. This PR removes its repository watching and event marshaling responsibilities and delegates them to ITeamExplorerContext. It also removes the need for refreshing a live LocalRepositoryModel, meaning the model can now be immutable after creation.

  • Remove ActiveRepo, Subscribe and Unsubscribe from ITeamExplorerServiceHolder
  • Remove RefreshCloneUrl from IGitService
  • Expose TeamExplorerContext and JoinableTaskFactory from ITeamExplorerServiceHolder (for repo change events and marshaling)

How to test

I've changed code that listens for repository change and refreshes the Team Explorer UI. We need to test moving between repositories and changing the clone URL.

  1. Open a repository that it on GitHub
  2. Open the Team Explorer - Home view
  3. The repository URL should be visible
  4. From the command line: git remote rename _origin origin
  5. The repository URL and banner should no longer appear
  6. From the command line: git remote rename origin _origin
  7. The repository URL and banner should re-appear

Make ITeamExplorerServiceHolder responsible for holding references to
services, but not watching for and marshaling repository change events.
Delegate to ITeamExplorerContext for this.
This is no longer called by TeamExplorerServiceHolder.
@jcansdale jcansdale changed the title Remove repository responsibilities from TeamExplorerServiceHolder Remove repository responsibilities from TeamExplorerServiceHolder (repository refactor part 2) Nov 1, 2018
Contains a call chain that results in a call to a virtual method
defined by the class.
@grokys
Copy link
Contributor

grokys commented Nov 7, 2018

One thing I noticed: when I change the name of the origin remote I get an exception at this line:

var address = HostAddress.Create(repository.CloneUrl);

We should check to see if repository.CloneUrl is null there. Should that be done here or in a separate PR do you think?

@jcansdale
Copy link
Collaborator Author

@grokys,

We should check to see if repository.CloneUrl is null there. Should that be done here or in a separate PR do you think?

I'd be inclined to look at that in a separate PR. We need to handle the situation when there is no origin remote better. I'm going to show a callout notification when there is no origin remote. I'll look at this as the same time.

@jcansdale
Copy link
Collaborator Author

Merged as part of #2028.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants