-
Notifications
You must be signed in to change notification settings - Fork 448
Utilizing a cache for user data #427
Conversation
08499a5 to
2d0b24b
Compare
…cements/git-client-cache # Conflicts: # src/GitHub.Api/Git/Repository.cs # src/UnityExtension/Assets/Editor/GitHub.Unity/ApplicationCache.cs
6488f60 to
bf459e2
Compare
…cements/git-client-cache
…cements/git-client-cache # Conflicts: # src/GitHub.Api/Git/RepositoryManager.cs
…cements/git-client-cache # Conflicts: # src/GitHub.Api/Git/Repository.cs
…cements/git-client-cache
…cements/git-client-cache
…cements/git-client-cache
|
Just to clarify as it's not clear from the title/description: this PR should only be caching user data? |
|
The title of this PR could be clearer, but yes, I added access to cache objects to |
|
I updated the description. |
Misunderstood the initial requirements. I believed the User object was just for currying data. So I created GitUser to represent the user data returned from GitClient and then moved the caching functionality over to User.
More rippling changes from the previous implementation attempt
…cements/git-client-cache
Moving the functionality to set the name an email to User and changing UserSettingsView to use that method. Also did some minor method renames.
The new method name is clearer as to what the method is checking for
grokys
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.
Got one question about event handlers preventing GC. Note this this also applies to Repository.
| this.cacheContainer = cacheContainer; | ||
|
|
||
| cacheContainer.GitUserCache.CacheInvalidated += GitUserCacheOnCacheInvalidated; | ||
| cacheContainer.GitUserCache.CacheUpdated += GitUserCacheOnCacheUpdated; |
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.
Will the User objects never need to be GCd? Adding these event handlers will mean that cacheContainer.GitUserCache will retain a reference to each User object.
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.
User and Repository are found on DefaultEnvironment.
Unity/src/GitHub.Api/Platform/IEnvironment.cs
Lines 30 to 31 in 2e16908
| IUser User { get; set; } | |
| IRepository Repository { get; set; } |
Unity/src/GitHub.Api/Platform/DefaultEnvironment.cs
Lines 137 to 138 in 2e16908
| public IRepository Repository { get; set; } | |
| public IUser User { get; set; } |
There is generally only one instance of User and Repository in the system created at specific points of initialization.
| User = new User(CacheContainer); |
| Repository = new Repository(RepositoryPath, CacheContainer); |
These objects are only "destroyed" when the domain reloads.
When they are constructed (either the first time or after a domain reload) these objects are being fed a CacheContainer. It will take a moment on some other threads for the User and Repository objects to get what they need to be fully functioning (they both have Initialize methods for that).
These objects store their data in file based caches specifically for this reason. Until they are fully functioning, they get to use their file based cache to supply data to the user interface.
|
This looks good to me @StanleyGoldman - Just take a look at @grokys question above |
grokys
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.
👍
Fixes: #412
Problems:
Userhas been used as a data object fromGitClientSolutions:
GitUseras the data object fromGitClientUsermuch likeRepositoryas the central front end to user data and actionsUsergets initialized with aGitClientand initiates a process to update it's dataUserlistens to invalidation events from theGitUserCacheand updates it's dataUserand user data fromRepositoryandRepositoryManagerUserSettingsViewandInitProviewViewto use the new cacheDepends on: