Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vinc3m1
Copy link
Contributor

@vinc3m1 vinc3m1 commented Jan 26, 2019

No description provided.

@vinc3m1
Copy link
Contributor Author

vinc3m1 commented Jan 26, 2019

Tests to come (probably next week) but let me know what you think about the API

@muraziz
Copy link
Contributor

muraziz commented Jan 28, 2019

@vinc3m1, is the purpose of this diff to avoid using ViewRenderInfo and instead use Adapter interface directly? If that's the only intention, I'm not sure whether the benefits outweigh the introduced complexity (with this we'll have three ways of handling views: components, ViewRenderInfo and AdapterProxy).
One thing that comes to my mind when using this approach is to avoid calling findViewById and use ViewHolders directly outside.

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) {
Copy link
Contributor

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!

@vinc3m1
Copy link
Contributor Author

vinc3m1 commented Jan 29, 2019

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.

@facebook-github-bot
Copy link
Contributor

@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.

@astreet
Copy link
Contributor

astreet commented Jul 10, 2019

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?

@vinc3m1
Copy link
Contributor Author

vinc3m1 commented Jul 10, 2019

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.

@astreet
Copy link
Contributor

astreet commented Jul 11, 2019

Can you maybe give a code example of how the API would be used? (in the Javadocs would be even better)

@mihaelao
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants