Fixes to populate remote url text field after publish#195
Fixes to populate remote url text field after publish#195StanleyGoldman merged 1 commit intomasterfrom
Conversation
| .FinallyInUI((success, ex) => | ||
| { | ||
| if (success && !String.IsNullOrEmpty(user.Name)) | ||
| if(GitClient != null) |
There was a problem hiding this comment.
hmmm why is GitClient null here? is the view racing the EntryPoint initialize code?
Also, spaaaaaaaaace.
There was a problem hiding this comment.
This if should be with the condition above:
if ((cachedUser == null || String.IsNullOrEmpty(cachedUser.Name)) && GitClient != null)
There was a problem hiding this comment.
GitClient was null for me when I rebuilt code with Unity running. I then figured it was possible during a domain reload.
There was a problem hiding this comment.
I don't know which space we are referring to. 🙈
| gitEmail = Repository != null ? Repository.User.Email : String.Empty; | ||
| repositoryRemoteName = DefaultRepositoryRemoteName; | ||
| repositoryRemoteUrl = string.Empty; | ||
| newRepositoryRemoteUrl = repositoryRemoteUrl = string.Empty; |
There was a problem hiding this comment.
This isn't being called anywhere, let's nuke it
There was a problem hiding this comment.
newRepositoryRemoteUrl and repositoryRemoteUrl are both used..
Unity/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/SettingsView.cs
Lines 344 to 345 in 4a208bd
| .FinallyInUI((success, ex) => | ||
| { | ||
| if (success && !String.IsNullOrEmpty(user.Name)) | ||
| if(GitClient != null) |
There was a problem hiding this comment.
This if should be with the condition above:
if ((cachedUser == null || String.IsNullOrEmpty(cachedUser.Name)) && GitClient != null)
|
If I'm reading this correctly, none of these changes (except GitClient null check) are needed, and instead we should be setting |
| AttachHandlers(Repository); | ||
|
|
||
| activeRemote = Repository != null ? Repository.CurrentRemote : null; | ||
| remoteHasChanged = true; |
There was a problem hiding this comment.
Setting remoteHasChanged is not enough. We need to update activeRemote at some point.
Part of my thinking was that activeRemote was only read from here..
Unity/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/SettingsView.cs
Lines 229 to 239 in 30e665c
There was a problem hiding this comment.
Yeah, you're right, there's no point in having the field if it's only ever touched here, and it shouldn't be touched anywhere else anyway
a9ee787 to
48be91c
Compare
Fixes #185
The problem here was due to the use of
activeRemoteinSettingsView.When a user is publishing, they are looking at the
HistoryView. Which means theSettingsViewis disabled, and when a remote is added the event handlerSettingsView.OnRepositoryChangedwill not run and won't updateactiveRemotewith the new value.I removed
activeRemoteand switched it toRepository.CurrentRemote.I also fixed two errors in the
SettingsViewthat would appear in the user log.