Add inheritance support

[ZacSweers]

This should address #4. I was thinking I could get around this by using parameterized classes, and to my good fortune it turned out Butterknife had a pretty decent solution for this. @emilsjolander would you mind taking a look and see what you think? @denley you as well if you're interested, since you opened the issue.

• Adopt Butterknife's approach to making all generates classes implement a IBarbershopInterface, and do class lookups of this interface rather than method lookups. This makes this next part possible.
• Fix inheritance by determining if a generated class has a parent and then extending from the parent instead of implementing the IBarbershopInterface. This way, style() can just call super.style() to ensure that the entire hierarchy of style() methods run.

This functionally works, but I'm slightly concerned about one thing that I'm going to mull over tomorrow. Right now, it traverses the entire hierarchy. So if you have a parent with a child and grandchild, you conceivably could have the parent getting styled three times (that is, once for the parent's constructor, once for the child's, and one more for the grandchild). I'm not sure how to get around this yet though, if at all. I should write better tests.

Now, if you have a TestView and ChildTestView that extends it, Barber will generate code that looks like this:

public class TestView$$Barbershop<T extends io.sweers.barber.sample.TestView> implements Barber.IBarbershop<T> {
  @Override
  public void style(final T target, final AttributeSet set, final int[] attrs, final int defStyleAttr, final int defStyleRes) {
    if (set == null) {
      return;
    }
    TypedArray a = target.getContext().obtainStyledAttributes(set, attrs, defStyleAttr, defStyleRes);
    if (a.hasValue(0)) {
      target.testBoolean = a.getBoolean(0, false);
    }
    a.recycle();
  }
}

public class ChildTestView$$Barbershop<T extends io.sweers.barber.sample.ChildTestView> extends TestView$$Barbershop<T> {
  @Override
  public void style(final T target, final AttributeSet set, final int[] attrs, final int defStyleAttr, final int defStyleRes) {
    super.style(target, set, attrs, defStyleAttr, defStyleRes);
    if (set == null) {
      return;
    }
    TypedArray a = target.getContext().obtainStyledAttributes(set, attrs, defStyleAttr, defStyleRes);
    if (a.hasValue(0)) {
      target.coolNumber = a.getInt(0, -1);
    }
    a.recycle();
  }
}

[denley]

I'm not convinced that calling super.style(target, set, attrs, defStyleAttr, defStyleRes) will work properly. Specifically, the attrs array should be different for the superclass and the subclass.

Using the example in your BarberTest test case, the child class should receive R.styleable.ChildTestView, whereas the parent class should be seeing R.styleable.TestView.

When I have some time I'll try to adapt your test case to illustrate this (it's still speculation at this point).

[ZacSweers]

Hmm, you might be right. When I expanded the tests a bit, it now starts to fail. My guess is that the only solution here is that the generated class has to know the R.styleable.<whatever> int[] at compile time and just include that in the code generation. Unfortunately this means breaking the API right after release 😦.

Would really rather prefer to keep the API the same if at all possible right now, or at least have it just be deprecated for a couple releases.

I did look at adding class annotations to specify this up front, but Java doesn't like this because it doesn't consider R ints to be constant -_-.

Any ideas?

[denley]

I have an idea.

Since Barber.style must be called with the attrs, defStyleAttr, and defStyleRes parameters, we can assume that it will always be called from the custom View's constructor once for each level in it's hierarchy. We also know that Barber.style will be called by the parent class before the child class, since the constructors are always called in that order.

So going on that, we could make ChildTestView$$Barbershop look something like this:

public class ChildTestView$$Barbershop<T extends io.sweers.barber.sample.ChildTestView> extends TestView$$Barbershop<T> {

    Set<T> styledViews = new LinkedHashSet<>;

    protected boolean hasStyled(T target) {
        return styledViews.contains(target);
    }

    @Override
    public void style(final T target, final AttributeSet set, final int[] attrs, final int defStyleAttr, final int defStyleRes) {
        if (set == null) {
            return;
        }

        if(!super.hasStyled(target)) {
            super.style(target, set, attrs, defStyleAttr, defStyleRes);
         } else {
            styledViews.add(target);

            TypedArray a = target.getContext().obtainStyledAttributes(set, attrs, defStyleAttr, defStyleRes);
            if (a.hasValue(0)) {
                target.coolNumber = a.getInt(0, -1);
            }
            a.recycle();
        }
    }
}

This way, the Barbershop delegates the styling to its parent if it hasn't been styled yet.

[ZacSweers]

That could work, but it would come at the expense of the current caching
mechanism. Right now it caches and reuses every instance of the Barbershop
classes when it finds them via reflection. This could be changed, but it
would require creating a new instance every time wouldn't it? It's probably
worth it, I'm not sure if there'd be any noticeable performance difference
since the Class<?> found via reflection could still be cached at least.

On Wed, Mar 4, 2015 at 4:29 AM Denley Bihari notifications@github.com
wrote:

I have an idea.

Since Barber.style must be called with the attrs, defStyleAttr, and
defStyleRes parameters, we can assume that it will always be called from
the custom View's constructor once for each level in it's hierarchy. We
also know that Barber.style will be called by the parent class before the
child class, since the constructors are always called in that order.

So going on that, we could make ChildTestView look something like this:

public class ChildTestView$$Barbershop extends TestView$$Barbershop {

Set<T> styledViews = new LinkedHashSet<>;

protected boolean hasStyled(T target) {
    return styledViews.contains(target);
}

@Override
public void style(final T target, final AttributeSet set, final int[] attrs, final int defStyleAttr, final int defStyleRes) {
    if (set == null) {
        return;
    }

    if(!super.hasStyled(target)) {
        super.style(target, set, attrs, defStyleAttr, defStyleRes);
     } else {
        styledViews.add(target);

        TypedArray a = target.getContext().obtainStyledAttributes(set, attrs, defStyleAttr, defStyleRes);
        if (a.hasValue(0)) {
            target.coolNumber = a.getInt(0, -1);
        }
        a.recycle();
    }
}

}

This way, the Barbershop delegates the styling to its parent if it hasn't
been styled yet.

—
Reply to this email directly or view it on GitHub
#5 (comment).

[denley]

I did take into consideration that Barbershops are cached and reused. It's why I used a Set<T> to remember each target that has been styled rather than just a boolean flag.

Keep in mind that each target object can only be styled once in its entire existence, since it happens in the constructor. The Barbershop is reused for different T target objects but never for the same one.

There is a performance drawback, which is that it would hold a reference to every T target object so they can never be garbage collected. They will exist until the app's process is terminated. Maybe we could use WeakReference, or maybe we can get away with only remembering the most recently styled target.

[ZacSweers]

so the set would just continue to grow and grow through the app's lifecycle as more instances are used? I've been tinkering with this today, and there's kind of a catch 22 regarding the two approaches. One is to cache the class like I mentioned and create a new instance of it when called, but this results in all style calls creating a new instance every time, and subsequently not checking the same values in parent fields (either a boolean isStyled flag, or the set idea). The other with your idea, the hasStyled set would only work once, and subsequent calls from other views would break wouldn't they? Once one is in the set, it would permanently return true.

I'm leaning pretty hard toward doing something to include the R.styleable.<whatever> in the generated code. That to me at this point seems like still the best option.

[denley]

Once one is in the set, it would permanently return true.

This is correct, but in practice it can actually never be called again on the same target. The style method must be called from the target's constructor (since it needs the AttributeSet parameter) and as such it can only ever be called once for any target (well, once per class in it's hierarchy).

Based on this, we might even be able to get away with this instead:

T lastStyledTarget = null

protected boolean hasStyled(T target) {
    return lastStyledTarget!=null && lastStyledTarget==target;
}

While I think the above solution would work, I think you're probably right to lean toward having the user specify the R.styleable.<whatever> for the target class. I think it would keep the Barbershop class simpler and hence reduce the complexity of testing required.

The downside is that it means deprecating the style(View target, AttributeSet set, int[] attrs, int defStyleAttr, int defStyleRes) method in favour of style(View target, AttributeSet set, int defStyleAttr, int defStyleRes) and a @Styleable(int[] res) class annotation. The second one is not much more verbose but deprecation always causes confusion.

[ZacSweers]

Yeah, I'm going to give your suggestion a try before deciding anything.

Unfortunately, your @Styleable(int[] res) example wouldn't work since it's not a constant value. A couple ideas for this would be either to specify a resource name via String (groan) or to do conventional naming (i.e. if you name your class MyTextView, Barber will assume the resource name is R.styleable.MyTextView). I'm thinking a good approach would be to default to the latter, and offer the former for anyone that wants another convention.

[ZacSweers]

So that solution worked (including using weakreferences). Since the code is there and ready, I'd like to run with this if possible. It's not too much added work/code, but another issue is that it isn't thread-safe. This normally wouldn't be an issue for probably 95% of use cases since most drawing is done on the UI thread. There are cases though where you construct a view off of the UI thread though (such as pre-loading view or for taking snapshots). Going to think on this some more tonight.

[denley]

Hmm, you're right. It seems that array parameters for annotations must not just be constants, but they must actually be an array of constants declared inside the annotation. I guess it's because it has to be used at compile time, and even values inside array constants can be changed at runtime.

On that note, this complete joke of code will actually compile: R.styleable.CustomView[0] = 2;. I wonder if this could be used to hack a library/sdk project.

Anyway, back to the topic at hand.

For your latter approach, I think that you will have trouble determining the package name of R. I can't think of a way you would figure it out at compile time, since you cant just use the target class` package (it may be nested inside a sub-package). It could also potentially be inside a package that's named the same as a library package name (though probably a rare case).

At runtime, you could use context.getPackageName(), but then if it were inside a library project it would find the wrong R class.

Maybe the style method could just accept R as a parameter, though users may find it confusing because it's not immediately obvious why they need to pass it as a parameter.

The threading issue you mentioned is a very good point. Maybe using synchronized somewhere will solve this, though blocking the UI thread is bad. This definitely needs some thought.

[ZacSweers]

Well, the Barbershop does fortunately know the class name at compile time, so it wouldn't be too difficult. Basically just default to doing something like “R.styleable.” + className in the code generation. That said, it's not ideal. Friend of mine said he has an idea, so we're going to look more at it tomorrow.

[denley]

I meant that you won't be able to reference R since you won't know the package name of the project. i.e. you won't be able to figure out what to write in the line import xxx.xxx.R;

[ZacSweers]

Ah, yeah I see what you mean

[denley]

The package of the target class is not necessarily the same as the package of the Android project.

[ZacSweers]

Yeah I realized what you meant right after I posted, edited my comment already :P

[ZacSweers]

I think I got it working in multithreaded situations using a WeakHashSet implementation, similar to what you were describing earlier in the thread. It automatically prunes itself as well. @denley @emilsjolander please take a look if you have time, hopefully should be ready to merge at this point

[denley]

Nice work. I think that hits the nail on the head.

My only other thought it that it might be prudent to do some check to make sure that Barber.style is being called properly from each class in the hierarchy. If the user accidentally leaves one out it would be a confusing bug. I'm not sure if this check is even possible, but its worth a thought.

[ZacSweers]

Yeah I'm not sure how that implementation would work off the top of my head. I think I'm just going to merge this as-is for now and can revisit that if anyone actually runs into that issue.

[emilsjolander]

LGTM