jmartinesp / SwissKnife

A multi-purpose library containing view injection and threading for Android using annotations
Apache License 2.0
254 stars 24 forks source link

injectViews method should call super.injectViews method if the superclass contains @InjectView annotation. #30

Closed s0nerik closed 9 years ago

s0nerik commented 9 years ago

I've noticed that injectViews method works not correctly when used the @InjectView annotation is used both in the actual class and some of it's superclasses. The easy way to solve it should be just to call the superclass's injectViews method if the superclass contains @InjectView annotation.

jmartinesp commented 9 years ago

Could you try with this branch? I've tested a few things and it seems to work well so far, but let us make sure.

s0nerik commented 9 years ago

Actually, it doesn't work the way I expect it to work. Having a structure like this causes a btn1 to be null while it should be present after injecting views into InheritedActivity.

BaseActivity:

@CompileStatic
class BaseActivity extends AppCompatActivity {

    @InjectView(R.id.btn1)
    Button btn1

}

InheritedActivity:

@CompileStatic
class InheritedActivity extends BaseActivity {

    @InjectView(R.id.btn2)
    Button btn2

    @Override
    protected void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState)
        contentView = R.layout.inject_inheritance_test
        SwissKnife.inject this
    }

    @Override
    protected void onPostCreate(@Nullable Bundle savedInstanceState) {
        super.onPostCreate(savedInstanceState)
        btn1.text = "Hello btn1"
        btn2.text = "Hello btn2"
    }
}

inject_inheritance_test.xml:

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:orientation="vertical" android:layout_width="match_parent"
    android:layout_height="match_parent">

    <Button
        android:id="@+id/btn1"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content" />

    <Button
        android:id="@+id/btn2"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content" />

</LinearLayout>

Also, I can create a pull request with the actual code that shows this issue.

Upd. To fix it I've implemented a method that looks for any occurrence of the field annotated with given annotation and used it to determine if any of superclasses contains members annotated with @InjectView or @InjectViews annotation.

So, replacing this method:

    public static MethodNode getInjectViewsMethod(ClassNode declaringClass) {
        Parameter[] parameters = [new Parameter(ClassHelper.make(Object.class), "view")]

        MethodNode injectMethod = declaringClass.getDeclaredMethod("injectViews", parameters)
        if (injectMethod == null) {
            injectMethod = createInjectMethod()
            // Some parent class has injectViews
            if (declaringClass.getMethod("injectViews", parameters)) {
                def block = injectMethod.getCode() as BlockStatement
                block.addStatement(stmt(callSuperX("injectViews", args(parameters))))
            }
            declaringClass.addMethod(injectMethod)
        }

        return injectMethod
    }

with this:

    public static MethodNode getInjectViewsMethod(ClassNode declaringClass) {
        Parameter[] parameters = [new Parameter(ClassHelper.make(Object.class), "view")]

        MethodNode injectMethod = declaringClass.getDeclaredMethod("injectViews", parameters)
        if (injectMethod == null) {
            injectMethod = createInjectMethod()
            // Some parent class has injectViews
            if (superClassHasFieldWithAnnotation(declaringClass, ClassHelper.make(InjectView))
                || superClassHasFieldWithAnnotation(declaringClass, ClassHelper.make(InjectViews))) {
                def block = injectMethod.getCode() as BlockStatement
                block.addStatement(stmt(callSuperX("injectViews", args(parameters))))
            }
            declaringClass.addMethod(injectMethod)
        }

        return injectMethod
    }

    @Memoized
    static boolean superClassHasFieldWithAnnotation(ClassNode thisClass, ClassNode annotationClass) {
        def c = thisClass.superClass
        while (c) {
            if (classHasFieldWithAnnotation(c, annotationClass)) {
                return true
            }
            c = c.superClass
        }
        return false
    }

    static boolean classHasFieldWithAnnotation(ClassNode thisClass, ClassNode annotationClass) {
        if (thisClass.fields.find { it.annotations.find {it.classNode == annotationClass} }) {
            return true
        }
        return false
    }

did the trick and the example at the top worked as it should. Should I make a pull request with these changes I made to overcome an issue?

jmartinesp commented 9 years ago

Actually, what I do is check if any of the parent classes has an injectViews method. If it didn't work for you, maybe this way is flawed because at the moment InheritedActivity is being processed, BaseActivity hasn't been processed yet and that method isn't there.

But looking for fields only covers the view injection part, not the listener injection. If you have a class which only has an @OnClick on a button and you extend it, this new class wouldn't have that listener injected as no super.injectViews(...) would be called.

s0nerik commented 9 years ago

You're most probably right about the BaseActivity not being processed at the moment InheritedActivity is processing. But as far as I remember, there's no way to ensure the order of classes being processed by the same AST transformation. So, basically, I see two ways to overcome the issue with injections other than those for Views. First of all, we can look for occurrences of methods/fields annotated with all the other needed annotations in superclasses just like I've done with @InjectView and @InjectViews. The other possible way I can imagine is to create a new annotation that will be applied by the user to the base classes, and we will need just to determine if any of superclasses is annotated with it to know if we need to super.injectViews() method. IMHO the first method is better, `cause it won't force user to annotate base classes with another annotation.