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

Conversation

shana
Copy link
Contributor

@shana shana commented Sep 15, 2015

Fixes #48

This PR grew a bit more than I expected. The main fix is that we were only looking at the local path to see if VS had loaded a new repository, but it turns out that when you publish a repository, the url change also needs to be monitored so the UI is updated accordingly. This required some changes to how repositories are compared (via SimpleRepositoryModel) and how changes to the active repo are tracked.

This led to having ISimpleRepositoryModel be used everywhere instead of using IGitRepositoryInfo, so we can compare things better.

  • Adds IView Error observable so we can expose errors and let the UI handle them instead of showing error messages directly from the view model
  • Switches to ISimpleRepositoryModel for all objects representing a repository, doing away with direct usage of IGitRepositoryInfo (which is VS's and doesn't have much information we can use)
  • Checks and updates ActiveRepo.CloneUrl whenever the underlying repository url changes (user changes it manually on the command line or in VS)
  • Hides the publish form properly (and disposes it) when publishing is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not strictly required, but might as well.

@haacked
Copy link
Contributor

haacked commented Sep 16, 2015

Hmm, build is broken. Also, could this be broken up into smaller PRs or is this all logically the same fix?

@shana
Copy link
Contributor Author

shana commented Sep 16, 2015

I broke some of the tests I recently added for the SimpleRepositoryModel because I changed how arguments get validated, and I was too tired to fix it up last night, fixing today.

@shana shana force-pushed the shana/48-publish-sticks-around branch 2 times, most recently from 442c51e to f6a1273 Compare September 16, 2015 12:00
IGitRepositoryInfo is a VS type that doesn't cache url information.
Switch to our ISimpleRepositoryModel which caches the local and remote
information and that we can control.
@shana shana force-pushed the shana/48-publish-sticks-around branch from f6a1273 to 6559c03 Compare September 16, 2015 12:09
Now that ActiveRepo is a SimpleRepositoryModel, we need a way to update
the url, for when a user adds or removes the origin remote manually
inside Visual Studio (in the git repo settings), or via the command
line.
This is required so that the UI updates properly and rechecks whether
the repo is ours or not.
If the origin remote url changes, update ActiveRepo that as soon as
possible.
@shana shana force-pushed the shana/48-publish-sticks-around branch from 028abe6 to 7dd53df Compare September 16, 2015 12:46
@shana
Copy link
Contributor Author

shana commented Sep 16, 2015

I took out the extension classes cleanup #89, the disposable calls #88, the get started fix #87 and the error handling #90 to cut down on the amount of clutter in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this comparison is the actual fix to #48.

@shana shana changed the title Fixes to publish form, error handling and more Fix publish form not getting hidden after publishing is done Sep 16, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't NullGuard.Fody catch this? If not, you could use the nameof operator here: throw new ArgumentNullException(nameof(path));

Yay C# 6!

@haacked
Copy link
Contributor

haacked commented Sep 17, 2015

So the original issue suggests we should load the Home page after a successful publish. I tested this and we stay on the Sync tab, but we update the state to that of a published repository. Staying in the Sync tab feels right to me.

@abuchholtzau do you have strong feelings either way?

This was referenced Sep 18, 2015
haacked added a commit that referenced this pull request Sep 18, 2015
Fix publish form not getting hidden after publishing is done
@haacked haacked merged commit a960664 into master Sep 18, 2015
@haacked haacked deleted the shana/48-publish-sticks-around branch September 18, 2015 20:34
@haacked
Copy link
Contributor

haacked commented Sep 18, 2015

vader-haack-thumbsup

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish stays on Synchronization page
2 participants