ViewBindings on subclasses broken

[Dellkan]

ViewBindings doesn't seem to apply on sub-classes of views. As far as I can tell, this issue only affects custom viewbindings.

Example:

This one works

        <android.support.design.widget.TextInputLayout
            android:layout_width="match_parent"
            android:layout_height="wrap_content">
            <EditText
                android:layout_width="match_parent"
                android:layout_height="wrap_content"
                android:inputType="textEmailAddress"
                android:hint="@string/login_username_hint"
                tools:text="user@username.com"
                tools:inputType="textEmailAddress"
                bind:text="${username}"
                style="@style/editTextInput" />
        </android.support.design.widget.TextInputLayout>

While this one will not work.

        <android.support.design.widget.TextInputLayout
            android:layout_width="match_parent"
            android:layout_height="wrap_content">
            <android.support.design.widget.TextInputEditText
                android:layout_width="match_parent"
                android:layout_height="wrap_content"
                android:inputType="textEmailAddress"
                android:hint="@string/login_username_hint"
                tools:text="user@username.com"
                tools:inputType="textEmailAddress"
                bind:text="${username}"
                style="@style/editTextInput" />
        </android.support.design.widget.TextInputLayout>

The only difference between these two layouts are android.support.design.widget.TextInputEditText vs EditText. android.support.design.widget.TextInputEditText extends AppCompatEditText which in turn extends EditText.

The problem field is bind:text - there are no errors, but the binding is never applied to the view. Neither OneWay nor TwoWay bindings seems to have any effect. Please note that I've overwritten text with my own binding (see https://github.com/Dellkan/Robobinding-helpers/blob/master/robobinding-helpers-common/src/main/java/com/dellkan/robobinding/helpers/common/viewattributes/EditText/EditTextExtensions.java)

Also note that I've tried with WebView (to test the last pull request), and had the same issue when I tried to use a subclass of WebView. Neither bindings that should be inherited, nor bindings applied directly to the subClass was applied.

I've run some tests, and found that this issue was introduced in 0.8.14, while 0.8.13 works completely as expected (and inherits bindings correctly).

[weicheng113]

@Dellkan , Thanks for the issue report. As you are on it, is it possible for you to make a smallest sample project to demonstrate the issue? I will work on it as soon as I get time.

Meanwhile, if you like, you can try to debug to locate the problem. The issue should not be too difficult to solve.

Thanks, Cheng

[Dellkan]

@weicheng113 I'll try to get time to build a minimal sample tonight.

0.8.13 -> 0.8.14 touched a lot of files, and I'm not sure where to start looking for this kind of issue (maybe you can give me a couple of pointers on where I should start looking?). To the extent I've debug-stepped through the code so far, I've noticed the following:

1. CustomViewBinding#mapBindingAttributes is never called for subjected views
2. https://github.com/RoboBinding/RoboBinding/blob/develop/framework/src/main/java/org/robobinding/LayoutInflaterHoneyComb.java#L135 The delegate does not return an actual View on subjected views (it returns null)
3. The issue only seems to occur when another LayoutInflaterFactory is part of the chain. My projects utilize https://github.com/chrisjenx/Calligraphy which has its own LayoutInflater. Note however that this combination still worked fine in 0.8.13.

As an aside, these kind of deep dives into the source codes really make me wish the project had better javadocs comments (on class/method purpose, usage, etc). It's pretty hard to understand what each part in this library does from source code and class/method names alone.

[Dellkan]

Hi @weicheng113. Please find a minimal sample on https://github.com/Dellkan/RobobindingIssue257Demo. In it, I discovered that it doesn't matter whether the binding is custom or not. No bindings will be attached on some view elements. Again, this only occurs when using https://github.com/chrisjenx/Calligraphy (and possibly other libraries that modifies the LayoutInflaterFactory).

While debugging, I stumbled over android/support/v7/app/AppCompatViewInflater.java:101 Which has the following code:

        View view = null;

        // We need to 'inject' our tint aware Views in place of the standard framework versions
        switch (name) {
            case "TextView":
                view = new AppCompatTextView(context, attrs);
                break;
            case "ImageView":
                view = new AppCompatImageView(context, attrs);
                break;
            case "Button":
                view = new AppCompatButton(context, attrs);
                break;
            case "EditText":
                view = new AppCompatEditText(context, attrs);
                break;
            case "Spinner":
                view = new AppCompatSpinner(context, attrs);
                break;
            case "ImageButton":
                view = new AppCompatImageButton(context, attrs);
                break;
            case "CheckBox":
                view = new AppCompatCheckBox(context, attrs);
                break;
            case "RadioButton":
                view = new AppCompatRadioButton(context, attrs);
                break;
            case "CheckedTextView":
                view = new AppCompatCheckedTextView(context, attrs);
                break;
            case "AutoCompleteTextView":
                view = new AppCompatAutoCompleteTextView(context, attrs);
                break;
            case "MultiAutoCompleteTextView":
                view = new AppCompatMultiAutoCompleteTextView(context, attrs);
                break;
            case "RatingBar":
                view = new AppCompatRatingBar(context, attrs);
                break;
            case "SeekBar":
                view = new AppCompatSeekBar(context, attrs);
                break;
        }

        if (view == null && originalContext != context) {
            // If the original context does not equal our themed context, then we need to manually
            // inflate it using the name so that android:theme takes effect.
            view = createViewFromTag(context, name, attrs);
        }

        if (view != null) {
            // If we have created a view, check it's android:onClick
            checkOnClickListener(view, attrs);
        }

        return view;

Not sure if the previous snippet is relevant, but it does run through it (and returns null as view)

[Dellkan]

https://github.com/RoboBinding/RoboBinding/blob/develop/framework/src/main/java/org/robobinding/LayoutInflaterHoneyComb.java#L158 (WithViewCreationIfNull5_0#onCreateView) never runs on subjected views. It does however run on 0.8.13 (and on 0.8.14/15 when CalligraphyLayoutInflater isn't attached), and subjected views are created in it, and put up for binding from there on. I don't understand much of how LayoutInflater + factories work, but maybe wrapPrivateFactory2 isn't working like intended?

I see that 0.8.14 introduced some significant changes to how the factories are wrapped and packed

@weicheng113 I'm at the end of my rope. I understand far too little of how LayoutInflater with its factories work, or what considerations you have to make, or are trying to make. By debugging, it seems to me that it has something to do with how LayoutInflaterHoneyComb is setting privateFactory2, but I don't know know how I'm to fix that. I would appreciate it greatly if you could look into it

[weicheng113]

@Dellkan, Sorry to response late. This part is a bit complicated. I spent quite a lot of time on this before. I will find some time this weekend. Thanks a lot for the issue demo and your effort into the problem.

Once i have located the issue, i will get you back.

Thanks, Cheng

[Dellkan]

@weicheng113 Thank you for taking the time :)

Let me know if I can be of any help

[weicheng113]

@Dellkan , I think i located the issue. In the line 57 ~ 61 CalligraphyLayoutInflater, the code block blow:

@Override
    public View inflate(XmlPullParser parser, ViewGroup root, boolean attachToRoot) {
        setPrivateFactoryInternal();
        return super.inflate(parser, root, attachToRoot);
    }

The method setPrivateFactoryInternal() will erase the privateFactory(erased the privateFactory from RoboBinding as well), which i think it is not right thing or safe thing to do.

In RoboBinding v0.8.13, we took the same approach as Calligraphy, which is subclassing of LayoutInflater(LayoutInflaterHoneyComb--subclassOf-->LayoutInflaterPreHoneyComb--subclassOf-->android.view.LayoutInflater). In this approach, CalligraphyLayoutInflater will be replaced by RoboBinding LayoutInflaterHoneyComb. That is why CalligraphyLayoutInflater will not affect RoboBinding LayoutInflaterHoneyComb. Whereas, since v0.8.14, we used a more elegant way, clone and wrap the existing LayoutInflater and we don't have any subclasses of android.view.LayoutInflater(LayoutInflaterHoneyComb and LayoutInflaterPreHoneyComb are no long subclasses of android.view.LayoutInflater). A cloned CalligraphyLayoutInflater will be used, which will erase any privateFactory previously set(in RoboBinding we never erase but wrap and delegate).

In this case, I would think what Calligraphy did was not a proper approach. What do you think @Dellkan ?

[Dellkan]

@weicheng113 Thank you very much for looking into this.

I debugged through, and saw the same thing. setPrivateFactoryInternal() does indeed overwrite the private factory set by Robobinding.

I'm still stumbling and struggling to understand how the mechanics of LayoutInflater works (with its factories, Factory2 and private factories, and I'm not sure what the road ahead would be. Forgive me the silly question, but is messing with mPrivateFactory the only way to achieve the goal of Robobinding and Calligraphy?

I've reported this to chrisjenx/Calligraphy#317, hoping we can get eyes on this from both ends.

Meanwhile, can you think of any workaround that either you or I can apply from Robobinding, or even in my own projects, until we get a more permanent fix? I'm using both Calligraphy and Robobinding in production settings, and can't really make do without either.

[Dellkan]

I've forked Calligraphy, and put in a hotfix. It seems to do the trick, but I'm not sure yet if there are any side-effects (there could very well be).

[weicheng113]

@Dellkan, I will give you my idea in a hour or so.

[weicheng113]

@Dellkan , I had a look of your hotfix. It looks good to me. What we need to do with privateFactory is if there is an existing privateFactory, we will wrap and delegate it.

I will try to explain to you my personal understanding on the mechanics of LayoutInflater here. I think the intention of these three factories: mFactory, mFactory2 and mPrivateFactory2, was to enable API consumer to plugin custom view creation abilities. mFactory is an old interface and is retained for back-compatibility.

The main logic of LayoutInflater is in the method of

LayoutInflater.createViewFromTag(View parent, String name, Context context, AttributeSet attrs,
boolean ignoreThemeAttr):View

If you browse through the code, it says it will try to createView

1. from mFactory2, or from mFactory if mFactory2 is null.
2. if 1 not successful, then try from mPrivateFactory.
   And so on.

Cheng

[Dellkan]

@weicheng113 Thank you for the description, and for taking a look at the hotfix.

So, essentially, all there is to it is:

1. mFactory2(Factory2) is the main handler, that creates the views
2. mFactory(Factory) is the old deprecated variant. It's there for compatibility, and if mFactory2 doesn't return/create the view
3. mPrivateFactory(Factory2) runs as a backup to the previous 2 steps, but doesn't have any additional purpose other then what mFactory2 had.

The rest of the code usually found then deals with compatibility layers, and for wrapping/delegating, since only one LayoutInflater can be set.

Robobinding and Calligraphy both have to deal with mPrivateFactory because both want to do something after the view has been created, which may not have happened before in step 3. They both want to read the xml attributes and apply them to the view, but don't actually need to create the view itself

Would that be a correct summary?

[weicheng113]

@Dellkan I think your understanding is correct in RoboBinding side. I am not sure about Calligraphy.

The feature #164 is a very important one to implement next. I believe It will be beneficial to RoboBinding users. If you get time and are interested, please I would like to invite you to have a look.

Thanks, Cheng

[Dellkan]

@weicheng113 I've already implemented something similar to #164 (I documented some of it in #228). This is of course in Robobinding-helpers, which in short is a layer of abstraction on top of Robobinding. I'd love to take a look at #164 to implement first-class support, but can't give any promises as to when I'd have time to do so.

[weicheng113]

@Dellkan , Thanks a lot. I will have a look your related addition when i get time. No problem, we can contribute when we have time. We open source to join all contributions to make progress and benefit other users.