-
Notifications
You must be signed in to change notification settings - Fork 448
Fixes to refresh ChangesView on start #134
Conversation
2dac6e9 to
e2ddd68
Compare
e2ddd68 to
fe565bd
Compare
|
|
||
| public override void OnEnable() | ||
| { | ||
| base.OnEnable(); |
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.
Probably don't want to just to this always at initialize, since it calls Repository Refresh and all views get initialized. The logic of doing this on enable is correct, it's just that OnEnable might get called before the Repository instance is actually set, which means that we need to also do this whenever OnGUI is called. #91 introduces some methods that get called to update handlers and data, and the logic then becomes like this:
| public override void OnEnable() | |
| { | |
| base.OnEnable(); | |
| AttachHandlers(Repository); | |
| } | |
| public override void OnDisable() | |
| { | |
| base.OnDisable(); | |
| DetachHandlers(Repository); | |
| } | |
| public override void OnRepositoryChanged(IRepository oldRepository) | |
| { | |
| base.OnRepositoryChanged(oldRepository); | |
| DetachHandlers(oldRepository); | |
| AttachHandlers(Repository); | |
| } | |
| private void AttachHandlers(IRepository repository) | |
| { | |
| if (repository == null) | |
| return; | |
| repository.OnLocalBranchListChanged += RunRefreshEmbeddedOnMainThread; | |
| repository.OnActiveBranchChanged += HandleRepositoryBranchChangeEvent; | |
| repository.OnActiveRemoteChanged += HandleRepositoryBranchChangeEvent; | |
| } | |
| private void DetachHandlers(IRepository repository) | |
| { | |
| if (repository == null) | |
| return; | |
| repository.OnLocalBranchListChanged -= RunRefreshEmbeddedOnMainThread; | |
| repository.OnActiveBranchChanged -= HandleRepositoryBranchChangeEvent; | |
| repository.OnActiveRemoteChanged -= HandleRepositoryBranchChangeEvent; | |
| } |
I could have sworn that that PR had already landed in master, really confused how it's not there yet, so that needs to land in order for these refresh bugs to get fixed.
shana
left a comment
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.
There's a better fix that should have been in master already, bleh :/ Added comment about that on the line.
|
Closing this out in favor of #91 |
Fixes: #132