Repository navigation
Provide constructor injection and setter injection (?) #204
Description
Activity
I vote for it. Great idea.
Le 25 mai 2012 16:54, "Pierre-Yves Ricau" <
reply@reply.github.com>
a écrit :We currently force the users to have at least package private (default)
scopes for their fields. Some users might not appreciate that. We could
allow constructor injection / setter injection, this way :@EBean public class MyBean { private View myView; private MyOtherBean myOtherBean; MyBean(@Bean MyOtherBean myOtherBean) { this.myOtherBean = myOtherBean; } void setMyView(@ViewById View myView) { this.myView = myView; } }
Reply to this email directly or view it on GitHub:
#204Note: I'm note sure what's best, the setter annotation could also be directly on the setter instead of being on the argument :
@ViewById void setMyView(View myView) { this.myView = myView; }
The latter is more common, but the former gives more flexibility.
Any progress on this isue?
Hi @giejay,
This feature is not scheduled for 3.0, so I guess not any work has been started on this.
However, you're really welcome to contribute :)
i had some thoughts about this issue.
setter
- having the annotation on a setter should be no big deal.
- adding the annotation on the parameter is not a good idea (see the discussion about parameter annotations in Add @Result annotation for @OnActivityResult #1058)
- additionally there should be only one bean/view/whatever set by one setter (with the usual void return)
- potential cause of unexpected NPE: not having an ensured injection order might be a source of bugs if one does more than setting a value in an injection method (e.g. accessing another, not yet set, value)
constructor
- might be a really nice feature as it would allow to have
finalbeans - throws the issue related to parameter annotation extraction right in our face (see Add @Result annotation for @OnActivityResult #1058)
- has to be limited to injections that don't require any contextual work. like view injection requiring to have a view inflated.
- might hard to implement (instantiating 1-n objects before call to
super()might require static helper methods somewhere).
I think i'd give the setter injection via annotation on method a try with code like @pyricau suggested.
@ViewById void setMyView(View myView) { this.myView = myView; }
but i would not call it "setter injection" but rather "method injection" allowing any name for the method and use the parameter name as default for annotation values like
ViewById.resName(). this would allow usage as advanced@AfterXXXannotations as you could have a snippet like this@Extra("MY_EXTRA_KEY") void reactOnExtra(String extraValue) { // do stuff with extraValue }
that currently has to be
@Extra("MY_EXTRA_KEY") String extraValue; @AfterExtras void reactOnExtra() { // do stuff with extraValue }
while the code saved is minimal in this example the upper way would not pollute the class scope with a variable only used in one method.
but as mentioned above, this could also cause unexpected issues (mostly NullPointerExceptions) when not used carefully. one should only use this as setter or only work with the value that got injected or are otherwise ensured to be set.
8 months without feedback. I just started to play with method injection for
@Bean. I have a working sample. I also tested with multiple values to inject. while that worked, there are issues with the ability to provide a implementation class in the@Bean.value()attribute. so i think it is the best to stay with the one injected element per method approach mentioned above.Sorry, @dodgex i did not saw this issue.
adding the annotation on the parameter is not a good idea
Why? We already have multiple annotations working just fine, and i just added two more.
@WonderCsabo actualy i'm not sure why i wrote that. iirc there was the discussion if we want to have annotations on a parameter and we decided to have them after this comment.
btw: if you want to see the current implementation you can see it here: https://github.com/dodgex/androidannotations/tree/204_method_injection
@dodgex it is a good idea. :) I see no problems with it. Actually i think it is much more natural then the method annotation. This is used by every DI system.
@WonderCsabo uhm, i'm not sure if i understand your comment. what is more natural? annotation on param?
Yeah, the param, of course. Becase the value actually will be injected to the parameter. Just like with
@Receiver.Extra, or@Injectin guice. Also, this way you can inject several parameter to the same method even when you useBean.value().okay. shall i create something like
@Bean.Implas parameter annotation? that way i could implement multiple beans per method injection.What is the problem with this?
void setBeans(@Bean(MyBeanImpl.class) MyBean bean, @Bean(MyBeanOtherImpl.class) MyBean bean2) { this.bean = bean; this. bean2 = bean2; }
😕
4 remaining items
You can check out how
@ViewByIdand@Clickworks. Actually they are reusing the samefindViewById()call, if it is possible.they use the same call/return value. but in this case it is the other way. we would need to have 1 call with multiple parameters. but yeah, that should be doable in a similar way...
Yeah, but the idea is the same about lazy generating the method call itself.
BTW, you can check out google/dagger and transfuse projects to get some ideas.
hmm for some reason the annotations on param only get validated, but not processed. do you have an idea why?
I guess the problem that we already registered the same annotation for fields. For now create a new annotation (
@BeanSetter), we can solve the architecture issue later.i debugged it: the problem is that the ModelProcessor is currently not able to handle method parameters.
Eh, sorry. Our current paramter handlers do nothing, the "parent" method handler is doing the processing actually.
We should modify ModelProcessor. We should also modify it to be able to use the same annotation on different element types.
i quickly hacked it a bit and now it processes the annotations, regardless of thier location (field, method or param)
its working! :)
@Bean public EmptyDependency dependency; @Bean protected void injectDependency(EmptyDependency methodInjectedDependency) { } protected void injectDependency2(@Bean EmptyDependency dep) { }
this one works too:
protected void twoDependencies(@Bean EmptyDependency bean1, @Bean(SomeImplementation.class) SomeInterface bean2) { }
currently this generates one instance of the bean per injection...
Great work! 👍 Maybe this is the time to open a WIP PR, because we are not specifying the feature, but the implementation.
doing some rough cleanup before creating PR. some parts of the code are a bit hacky ;)
Setter injection is implemented. Constructor injection only makes sense in case of beans. If there is need for that, we will open a new issue.
We currently force the users to have at least package private (default) scopes for their fields. Some users might not appreciate that. We could allow constructor injection / setter injection, this way :