-
Notifications
You must be signed in to change notification settings - Fork 766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create an AdapterProxy to directly call through to underlying Adapter #481
base: master
Are you sure you want to change the base?
Conversation
… with RecyclerBinder
Tests to come (probably next week) but let me know what you think about the API |
@vinc3m1, is the purpose of this diff to avoid using cc @pasqualeanatriello, thoughts? |
@@ -46,7 +46,7 @@ public RenderInfoViewCreatorController(boolean customViewTypeEnabled, int compon | |||
|
|||
@UiThread | |||
public void maybeTrackViewCreator(RenderInfo renderInfo) { | |||
if (!renderInfo.rendersView()) { | |||
if (!renderInfo.rendersView() || renderInfo == AdapterProxy.PROXY_RENDER_INFO) { |
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.
This seems weird! If AdapterProxy
is going to be used, maybe then RenderInfo
should have a method that will indicate whether it should be tracked, otherwise it risks being unmaintainable in case more RenderInfo
children will be added in the future!
We've definitely seen some metrics improvements and are exclusively using this over ViewRenderInfo actually. The improvements are reduced overhead, much simpler integration with existing RV Adapters, and fixed some bugs when dealing with insert/remove functions when proxying to an underlying adapter (due to positions no longer matching) The main issue with ViewRenderInfos is that the position isn't passed through its function calls so we can't directly proxy to the underlying adapter without tracking the positions and updating them on insert/remove, which gets quite complicated. |
@vinc3m1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
This seems interesting, can you explain more how it's used? Like, is this when embedding a RecyclerBinder within part of an existing RecyclerView? Or is it embedding an existing RV inside a RB? |
When adding RecyclerBinder to an existing RecyclerView + Adapter, this allows the adapter calls to go directly through to the underlying Adapter without the overhead/indirection/API differences of using a ViewRenderInfo for ever row. |
Can you maybe give a code example of how the API would be used? (in the Javadocs would be even better) |
I think this is an interesting idea, and it would be great to make RecyclerBinder more flexible in how it integrates with existing RecyclerViews. I think this API in this form might be complicating things a bit though; this is providing an Adapter proxy which is only capable of creating ViewHolders, but doesn't also handle operations. It's basically just a view creator&binder so calling it an adaptor might be a bit confusing. Would it make sense for this use case to pass a custom adapter to the RecyclerBinder instead? |
No description provided.