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

View factories #93

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

View factories #93

wants to merge 5 commits into from

Conversation

leonrd
Copy link

@leonrd leonrd commented Jan 14, 2017

What this PR does:

  • Adds ability to instantiate views using a factory function, instead of using constructor reflection.
    This should also provide a bit more type safety. This is similar to how Anko works with { ctx -> View(ctx) }
  • Anvilgen now creates lazy factory functions for the above functionality.
  • Anvilgen will skip creation of abstract views, but still generates attrs for them. This is a side-effect of the above type safety: could not instantiate abstract views.
  • Anvilgen will skip creation of views which do not have constructor(Context). This is a side-effect of the above type safety: could not properly instantiate these views. In the past, constructor.newInstance(context) could have failed.

I haven't made any benchmarks yet, but I think this should provide some performance improvements versus viewClass.getConstructor(Context.class).newInstance(c);

Besides this, in Kotlin you get a rather nice syntax with v(MyCustomView(it)).

The only issue with this approach, that I could see, is that these functions should be cached, otherwise Anvil will reinstantiate the view on every render. Which is a pretty big deal.

Fortunately, Retrolambda and Kotlin caches lambdas, so this should work fine:
v((ctx) -> { new MyCustomView(ctx) })
and this
v({ ctx -> MyCustomView(ctx) })
and this
v({ MyCustomView(it) })

Kotlin (and maybe Retrolambda, didn't check), on the other hand, does not cache method references.
So while this is a pretty nice syntax:
v(::MyCustomView) { /* do something */ }
the problem is that it would instantiate the view on every render.

Any thoughts?

@dmitry-zaitsev
Copy link
Contributor

Fortunately, Retrolambda and Kotlin caches lambdas

Not sure where did this came from, but it is not true. If we'll call the factory function twice it will return a new object on each call (no matter if it's Kotlin, Retrolambda or Java 8)

@leonrd
Copy link
Author

leonrd commented Feb 4, 2017

Sorry for the confusion, I wasn't refering to that. What I meant was: kotlin, and possibly retrolambda, reuses the reference for a given lambda. So you can compare and see if the factory function changed.

@dmitry-zaitsev
Copy link
Contributor

Oh OK, now I get it - thanks for the explanation.

Some other feedback - code indentation is slightly off for some files (like Anvil.java).

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

Successfully merging this pull request may close these issues.

2 participants