-
Notifications
You must be signed in to change notification settings - Fork 448
Fixes to populate remote url text field after publish #195
Conversation
| .FinallyInUI((success, ex) => | ||
| { | ||
| if (success && !String.IsNullOrEmpty(user.Name)) | ||
| if(GitClient != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm why is GitClient null here? is the view racing the EntryPoint initialize code?
Also, spaaaaaaaaace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if should be with the condition above:
if ((cachedUser == null || String.IsNullOrEmpty(cachedUser.Name)) && GitClient != null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't being called anywhere, let's nuke it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newRepositoryRemoteUrl and repositoryRemoteUrl are both used..
Unity/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/SettingsView.cs
Lines 344 to 345 in 4a208bd
| newRepositoryRemoteUrl = EditorGUILayout.TextField(GitRepositoryRemoteLabel + ": " + repositoryRemoteName, newRepositoryRemoteUrl); | |
| var needsSaving = newRepositoryRemoteUrl != repositoryRemoteUrl && !String.IsNullOrEmpty(newRepositoryRemoteUrl); |
| .FinallyInUI((success, ex) => | ||
| { | ||
| if (success && !String.IsNullOrEmpty(user.Name)) | ||
| if(GitClient != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| hasRemote = activeRemote.HasValue && !String.IsNullOrEmpty(activeRemote.Value.Url); | |
| if (!hasRemote) | |
| { | |
| repositoryRemoteName = DefaultRepositoryRemoteName; | |
| newRepositoryRemoteUrl = repositoryRemoteUrl = string.Empty; | |
| } | |
| else | |
| { | |
| repositoryRemoteName = activeRemote.Value.Name; | |
| newRepositoryRemoteUrl = repositoryRemoteUrl = activeRemote.Value.Url; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.