Added method to clear model cache and force a model build (#583) #586
Conversation
|
Could you add a test for this? there are already a bunch for the paged controller |
|
Yeah, I'll do it in the next days or weekend. |
| (0 until modelCache.size).forEach { | ||
| modelCache[it] = null | ||
| } | ||
| rebuildCallback() |
There was a problem hiding this comment.
I don't know that this method should call rebuildCallback() - since the intention of the method is to clear everything. If you want to also rebuild them this could be named more like forceRebuildAllModels
However, since requestForcedModelBuild calls this clear method as well as requestModelBuild() then requestModelBuild() is basically being called twice which is unnecessary, so it seems fine to remove the building callback from clearModels
There was a problem hiding this comment.
You're right. I removed the callback call.
| @@ -236,6 +236,13 @@ class PagedListModelCacheTest { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| fun clear() { | |||
| pagedListModelCache.clearModels() | |||
There was a problem hiding this comment.
afaik this doesn't test that models are cleared, just that models are rebuilt. I think a good way to test the behavior is to check that all models are rebuilt - maybe by adding a counter in the modelBuilder callback similar to how rebuildCounter is used
There was a problem hiding this comment.
Fixed the test. The callback call in the previous message was hiding the fact that the cache didn't even have models.
| @@ -142,6 +142,12 @@ internal class PagedListModelCache<T>( | |||
| return modelCache as List<EpoxyModel<*>> | |||
| } | |||
|
|
|||
| fun clearModels() { | |||
| (0 until modelCache.size).forEach { | |||
There was a problem hiding this comment.
I think a simpler approach would be modelCache.fill(null)
|
Thanks for updating - this looks ready. I'm concerned that there could be race conditions by calling this clear function on a different thread at the same time models are being built. I think it is ok to address that in a follow up task though, since we need to think about synchronization of the whole class. |
|
I'm almost sure there is already a race condition in model building (see #567). That would have to be addressed as a whole. |
PR for #583