PrecacheAsync on ImageCache #275
Conversation
In-memory layer of caching in ImageCache
up to spead
|
Hi @jurepurgar, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
| /// </summary> | ||
| /// <param name="uri">Uri of the image</param> | ||
| /// <param name="storeToMemoryCache">Indicates if image should be available also in memory cache</param> | ||
| /// <returns>void</returns> |
There was a problem hiding this comment.
Not sure if this is the right name. Could also be CacheAheadAsync, AssureCachedAsync or something else.
There was a problem hiding this comment.
I do not see why we cannot use this method within GetFromCacheAsync ?
There was a problem hiding this comment.
It does not return the BitmapImage. The point is it does not create BitmapImage for performance reasons, it just downloads the file if it is not there or it is outdated. If true is passed for storeToMemoryCache than it just calls GetFromCacheAsync because than BitmapImage has to be created to be stored in memory cache. Hope it makes sense.
There was a problem hiding this comment.
Understood..Can we just then add a parameter to return a bitmap or null? I would like to factorize here
There was a problem hiding this comment.
PreCache it is :) Also rewritten the whole thing so there is no duplicated code.
|
LGTM! |
| { | ||
| public Task<BitmapImage> Task { get; set; } | ||
|
|
||
| public bool IsAssureOnly { get; set; } |
There was a problem hiding this comment.
Not sure I'm convinced by this property name as it gives no indication what it's actually used for. What's the context for this property?
There was a problem hiding this comment.
It indicates the kind of Task. Whether it is only to assure image is in the cache (called by PreCacheAsync, that does not create BitmapImage) or not (called by GetFromCacheAsync). The term "AssurreOnly" is used also as parameter on method GetFromCacheInternalAsync to indicate the kind of task needed. Do you have another suggestion for the name? GetFromCacheInternalAsync should probably be called GetOrAssureAsync?
There was a problem hiding this comment.
In that case, I would say the property should be called IsCacheOnly or IsCacheDownloadOnly. Using Assure everywhere makes no sense because there's no context of what you're wanting to assure people of :)
There was a problem hiding this comment.
I agree. What about IsAssureCachedOnly? Because download does not happen if image is already in the cache.
There was a problem hiding this comment.
I think instead of Assure we should be using Ensure as that would convey the right meaning
There was a problem hiding this comment.
@hermitdave I disagree. Ensure what? What are we ensuring? There is no context given as it stands.
There was a problem hiding this comment.
As discussed with @ScottIsAFool i think IsAssureOnly is very confusing. Something like EnsureCachedCopy would be more appropriate
There was a problem hiding this comment.
What about IsPreCache, since the public method is called PreCacheAsync? Anyway I think we should align the name with public method, but I don't know which is more appropriate "PreCacheAsync" or "EnsureCachedAsync".
There was a problem hiding this comment.
I wouldn't find IsPreCache on the file but i think PreCacheAsync or EnsureCachedAsync are both okay.
I do however strongly think that once we merge these changes, someone should go throught the file and clean it up.. maybe streamline it a bit
There was a problem hiding this comment.
I went with the term "PreCache" everywhere. Let me know if it is OK.
|
LGTM |
| { | ||
| image = GetFromMemoryCache(key); | ||
| if (isPreCache && image != null) | ||
| { |
There was a problem hiding this comment.
Shouldn't this return image, not null? If it should be null, maybe a small comment just to explain what's happening here.
There was a problem hiding this comment.
It doesn't matter, because if isPreCache is true we don't care about the image, we just want it to be in the cache. On the other hand the whole if statement could be omitted I guess, since it doesn't really matter if we return image or null. Let me know how would you like to have it.
There was a problem hiding this comment.
IMO remove the if statement and return the image.. if the caller is expecting image then great otherwise it will be ignored
There was a problem hiding this comment.
Makes sense. Changed in the latest commit.
…emory cache even if "PreCache" is requested. GetOrPreCacheAsync now creates a new "Get task" only if current "PreCache" task did't return an image.
|
Congratulations once again |
Implements item 4 in #233