Performance improvements to homepage recyclers and coil #2125
Conversation
| homeViewModel.queryTextSubmit("") | ||
| } | ||
|
|
||
| homeMasterRecycler.setHasFixedSize(true) |
There was a problem hiding this comment.
Are you sure about this?
| expandCallback?.invoke(name) | ||
| val currentAdapter = (binding.homeChildRecyclerview.adapter as? HomeChildItemAdapter) | ||
| ?.apply { | ||
| if (isHorizontal != info.isHorizontalImages) isHorizontal = info.isHorizontalImages |
There was a problem hiding this comment.
isHorizontal and hasNext is not an expensive operation? Why are you doing this check?
There was a problem hiding this comment.
Because I was randomly trying things and it ended up in there when I opened the PR. That won't be in the final version.
| expandCount = count | ||
| expandCallback?.invoke(name) | ||
| val currentAdapter = (binding.homeChildRecyclerview.adapter as? HomeChildItemAdapter) | ||
| ?.apply { |
There was a problem hiding this comment.
You forgot to update the clickCallback, nextFocusUp and nextFocusDown. As sharedPool is shared between adapters this needs to always be set.
| ) { | ||
| // clear image to avoid loading & flickering issue at fast scrolling (e.g, an image recycler) | ||
| this.dispose() | ||
| if (imageData == null) { |
There was a problem hiding this comment.
What? dispose stops the current request and clears the image. I do not see what advantage this brings. Moreover, will this not break if we do loadImage(url); loadImage(null) ?
There was a problem hiding this comment.
Did you mean to write
this.dispose()
this.setImageDrawable(null)| } | ||
| } | ||
|
|
||
| binding.homeChildRecyclerview.setRecycledViewPool(HomeChildItemAdapter.sharedPool) |
There was a problem hiding this comment.
I believe that you should set the pool before attaching the adapter. I do not have a source for this, but I remember reading it.
| } | ||
|
|
||
| binding.homeChildRecyclerview.setRecycledViewPool(HomeChildItemAdapter.sharedPool) | ||
| binding.homeChildRecyclerview.setHasFixedSize(true) |
There was a problem hiding this comment.
Can you please double check this. I am unsure if we can make such assumptions if we recycle the entire homeChildRecyclerview, but I have not checked.
| companion object { | ||
| val sharedPool = | ||
| RecyclerView.RecycledViewPool().apply { this.setMaxRecycledViews(CONTENT, 4) } | ||
| RecyclerView.RecycledViewPool().apply { this.setMaxRecycledViews(CONTENT, 8) } |
There was a problem hiding this comment.
Any reason why this needs to be 8? This is the parent view for each row, not the items themself.
There was a problem hiding this comment.
Because sometimes there are many rows. That was the intent anyway.
| internal fun buildImageLoader(context: PlatformContext): ImageLoader = ImageLoader.Builder(context) | ||
| .crossfade(200) | ||
| .allowHardware(SDK_INT >= 28) // SDK_INT >= 28, cant use hardware bitmaps for Palette Builder | ||
| .allowHardware(SDK_INT >= 28) // SDK_INT < 28, can't use hardware bitmaps for Palette Builder |
There was a problem hiding this comment.
I think you misinterpreted the comment. The comment is "SDK_INT >= 28" and "can't use hardware bitmaps for Palette Builder", they are unrelated, and it does not make any sense to switch >= to <. See 3636d8e
There was a problem hiding this comment.
Oh that was my had yeah whoops.
| } | ||
|
|
||
| if(imageData == null) return // Just in case | ||
| // Only dispose if a different image is requested to avoid unnecessary reloads |
There was a problem hiding this comment.
Is this really a thing that happens? Does dispose really show up in a flamegraph?
There was a problem hiding this comment.
Well the idea was to not clear and reload the image every time but only if the actual source changes but its also possible I misinterpreted the intent behind dispose here.
| adapter.hasNext = item.hasNext | ||
| adapter.submitList(info.list) | ||
|
|
||
| binding.homeChildRecyclerview.addOnScrollListener(object : RecyclerView.OnScrollListener() { |
There was a problem hiding this comment.
Same problem as the You forgot to update the clickCallback
Thanks for the review alot I hadn't yet considered, but for the record I only opened this PR so that I could just put it out here, it was far from finished which is why I marked as a draft. I was trying to reduce the lag on the home page on some devices so I was trying things. Coil seems to be a major part of the expensive operations also, and setHasFixedSize can help also, but you also make a point if that is okay to do. I'm almost certain it should be fine on the master recycler, less so on the child ones. (or maybe I'm in reverse here and its the other way around). |
2 participants
No description provided.