square / mortar

A simple library that makes it easy to pair thin views with dedicated controllers, isolated from most of the vagaries of the Activity life cycle.
Apache License 2.0
2.16k stars 156 forks source link

AppCompat v. 22.1.0 swaps PathContext with Activity's on API < 21 #145

Closed almozavr closed 8 years ago

almozavr commented 9 years ago

AppCompatActivity and AppCompatViewInflater swaps view's context its inflated with for its container activity. So view no longer can find proper services we put in Context with getSystemService() because its constructor receives wrong Context. There are 2 ways to overcome it:

  1. AppCompat's delegate (AppCompatDelegateImplV7) looks if view's parent has id != android.R.id.content to try swap context, so if you change PathContainer's id to android.R.id.content, it will work.
  2. AppCompat checks if underlying activity implements LayoutInflater.Factory to pass control to it. This implementation couldn't make a trick with context swaping.
rjrjr commented 9 years ago

Thanks for the report. v22 was just full of surprises, wasn't it? We're dealing with all kinds of unpleasantness in RecyclerView too.

Overloading android.R.id.content seems like a pretty dicey proposition. And I don't understand your second proposal, could you rephrase?

jrgonzalezg commented 9 years ago

I am using AppCompat v22.1.1 with mortar using Dagger 2 as a service and i have not noticed any issues. When / how does this fail exactly?

almozavr commented 9 years ago

Sure, android.R.id.content is very hacky, dirty and dicey – just a quick note for those, who need a quick fix.

My 2nd proposal it to use AppCompatDelegateImplv7's ability to use a custom LayoutInflater.Factory service and within it's onCreateView method provide proper mortar-scoped Context. There is a small discussion of caveats.

So LayoutInflater.Factory should be implemented by some BaseActivity to provide a proper Context to inflated Views.

Sorry, for writing comments and the code – will try during this week, but noting this if someone wanna get to it faster and should have a glue where to start from.

jrgonzalezg commented 9 years ago

As i said in my previous comment, i am retrieving Mortar services (currently Dagger 2) from the context passed to the view in the constructor with no issues. I think the context that the view received is exactly either activity or a child of the activity context that is created after the activity layout inflater is modified by AppCompat, so it retains all its properties. Be sure to use v22.1.1 instead of v22.1.0 since v22.1.0 had a bug in the layout inflation of views inside fragments, and that could be the reason for your issue. If you provide a working minimal example with the problem maybe we can find out why it fails in your case while it does work with my code.

almozavr commented 9 years ago

@jrgonzalezg I'm using own adoption of Martar+Flow example simplified with some sugar annotations. You can see a lib and sample here https://github.com/techery/presenta

Also, I checked 21.1.1 and see no change in code which results target views to receive parent Context instead of wrapper from PathContextFactory. If you look at AppCompatDelegateImplV7, after delegate checks if host Activity is a LayoutInflater.Factory, it tries to create view with own inflater like this:

    @Override
    public View createView(View parent, final String name, @NonNull Context context,
            @NonNull AttributeSet attrs) {
        final boolean isPre21 = Build.VERSION.SDK_INT < 21;

        if (mAppCompatViewInflater == null) {
            mAppCompatViewInflater = new AppCompatViewInflater(mContext);
        }

        // We only want the View to inherit it's context from the parent if it is from the
        // apps content, and not part of our sub-decor
        final boolean inheritContext = isPre21 && mSubDecorInstalled && parent != null
                && parent.getId() != android.R.id.content;

        return mAppCompatViewInflater.createView(parent, name, context, attrs,
                inheritContext, isPre21);
    }

As you can see inheritContext will be true. Let's go deeper to mAppCompatViewInflater.createView:

public final View createView(View parent, final String name, @NonNull Context context,
            @NonNull AttributeSet attrs, boolean inheritContext, boolean themeContext) {
...
        if (inheritContext && parent != null) {
            context = parent.getContext();
        }
...
        createViewFromTag(context, name, attrs)
...

so context will become parent's context which in our case is host Activity and not the wrapper (PathContext). This is it.

jrgonzalezg commented 9 years ago

Thanks for the explanation. It seems that your code is very similar to mine since i am also using AppCompat + Mortar + Flow. And i have traced the execution and indeed the inheritContext variable gets set to true on that first method. But then my code follows a different path than yours that it is quite interesting.

I have added a use of the Dagger service retrieved through Mortar directly on the constructor of my custom views. When the code reaches that line for the first time (more on this soon), the context is in effect the activity context. That causes the retrieved Dagger component to be the one in the activity and not the one it is intended to be retrieved in the view which causes a ClassCastException. That exception is caught by createViewFromTag() in AppCompatViewInflater and it simply returns null after that.

We then go back at lines 800-812 of AppCompatDelegateImplV7 that looks like this:

    /**
     * From {@link android.support.v4.view.LayoutInflaterFactory}
     */
    @Override
    public final View onCreateView(View parent, String name,
            Context context, AttributeSet attrs) {
        // First let the Activity's Factory try and inflate the view
        final View view = callActivityOnCreateView(parent, name, context, attrs);
        if (view != null) {
            return view;
        }

        // If the Factory didn't handle it, let our createView() method try
        return createView(parent, name, context, attrs);
    }

And since the exception made our view null, it does not return that but instead tries the second not-Factory route to create the view which uses the proper PathContext and everything works afterwards. So this may serve as another point for an idea on how to fix this issue for the general case.

almozavr commented 9 years ago

That's what I was talking about, thanks.

Another possible solution is to provide custom LayoutInflaterFactory for (Base)Activity, and it works:

public abstract class BaseActivity extends AppCompatActivity {

    @Override protected void onCreate(Bundle savedInstanceState) {
        LayoutInflaterCompat.setFactory(LayoutInflater.from(this), new CustomAppCompatViewInflater(this));
        super.onCreate(savedInstanceState);
    }

And LayoutInflaterFactory goes very similar to default AppCompatViewInflater from AppCompat v21.1.1

public class CustomAppCompatViewInflater implements LayoutInflaterFactory {

    static final Class<?>[] sConstructorSignature = new Class[]{
            Context.class, AttributeSet.class};

    private static final Map<String, Constructor<? extends View>> sConstructorMap = new HashMap<>();

    private final Context mContext;
    private final Object[] mConstructorArgs = new Object[2];

    public CustomAppCompatViewInflater(Context context) {
        mContext = context;
    }

    @Override
    public final View onCreateView(View parent, final String name, @NonNull Context context,
            @NonNull AttributeSet attrs) {
        final Context originalContext = context;

        /* Mortar modification START */

        // We can emulate Lollipop's android:theme attribute propagating down the view hierarchy
        // by using the parent's context
        //        if (inheritContext && parent != null) {
        //            context = parent.getContext();
        //        }

        // We then apply the theme on the context, if specified
        if (Build.VERSION.SDK_INT < 21) {
            context = ViewUtils.themifyContext(context, attrs, true, true);
        }

        /* Mortar modification END, the rest has no modification */

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

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

        return null;
    }

    private View createViewFromTag(Context context, String name, AttributeSet attrs) {
        if (name.equals("view")) {
            name = attrs.getAttributeValue(null, "class");
        }

        try {
            mConstructorArgs[0] = context;
            mConstructorArgs[1] = attrs;

            if (-1 == name.indexOf('.')) {
                // try the android.widget prefix first...
                return createView(name, "android.widget.");
            } else {
                return createView(name, null);
            }
        } catch (Exception e) {
            // We do not want to catch these, lets return null and let the actual LayoutInflater
            // try
            return null;
        } finally {
            // Don't retain static reference on context.
            mConstructorArgs[0] = null;
            mConstructorArgs[1] = null;
        }
    }

    private View createView(String name, String prefix)
            throws ClassNotFoundException, InflateException {
        Constructor<? extends View> constructor = sConstructorMap.get(name);

        try {
            if (constructor == null) {
                // Class not found in the cache, see if it's real, and try to add it
                Class<? extends View> clazz = mContext.getClassLoader().loadClass(
                        prefix != null ? (prefix + name) : name).asSubclass(View.class);

                constructor = clazz.getConstructor(sConstructorSignature);
                sConstructorMap.put(name, constructor);
            }
            constructor.setAccessible(true);
            return constructor.newInstance(mConstructorArgs);
        } catch (Exception e) {
            // We do not want to catch these, lets return null and let the actual LayoutInflater
            // try
            return null;
        }
    }

}
almozavr commented 9 years ago

Need someone to review above and think of caveats.

deleet commented 9 years ago

Other than the constructor not matching the class name in your code sample above, this seems to work well for me. I'll take it as a temporary solution until what I think is a bug in the support library is fixed.

almozavr commented 9 years ago

Above code should be a simple wrapper around original AppCompatViewInflater - will update in a couple of hours.

almozavr commented 9 years ago

The same approach, but cleaner:

public abstract class BaseActivity extends AppCompatActivity {

    @Override protected void onCreate(Bundle savedInstanceState) {
        LayoutInflaterCompat.setFactory(LayoutInflater.from(this), new AppCompatViewInflaterProxy(this));
        super.onCreate(savedInstanceState);
    }
...
}
/**
 * This class is responsible for manually inflating our tinted widgets which are used on devices
 * running {@link android.os.Build.VERSION_CODES#KITKAT KITKAT} or below. As such, this class
 * should only be used when running on those devices.
 * <p>This class two main responsibilities: the first is to 'inject' our tinted views in place of
 * the framework versions in layout inflation; the second is backport the {@code android:theme}
 * functionality for any inflated widgets. This include theme inheritance from it's parent.
 *
 * <p> Mortar Context is preserved.
 */
public class AppCompatViewInflaterProxy implements LayoutInflaterFactory {

    private final AppCompatViewInflater appCompatViewInflater;

    public AppCompatViewInflaterProxy(Context context) {
        appCompatViewInflater = new AppCompatViewInflater(context);
    }

    @Override
    public final View onCreateView(View parent, String name, @NonNull Context context, @NonNull AttributeSet attrs) {
        return appCompatViewInflater.createView(parent, name, context, attrs, false, Build.VERSION.SDK_INT < 21);
    }
}

@rjrjr if it's ok, I'll open a PR for mortar sample with it.

rjrjr commented 9 years ago

Not ignoring this, just starved for brainwidth. It's on the queue, and thanks so much for digging into it.

On Wed, May 6, 2015, 12:00 AM Aleksey Malevaniy notifications@github.com wrote:

The same approach, but cleaner:

public abstract class BaseActivity extends AppCompatActivity {

@Override protected void onCreate(Bundle savedInstanceState

) { LayoutInflaterCompat.setFactory(LayoutInflater.from(this), new AppCompatViewInflaterProxy(this)); super.onCreate(savedInstanceState); }... }

/* * This class is responsible for manually inflating our tinted widgets which are used on devices * running {@link android.os.Build.VERSION_CODES#KITKAT KITKAT} or below. As such, this class * should only be used when running on those devices. *

This class two main responsibilities: the first is to 'inject' our tinted views in place of * the framework versions in layout inflation; the second is backport the {@code android:theme} * functionality for any inflated widgets. This include theme inheritance from it's parent. * *

Mortar Context is preserved. /public class AppCompatViewInflaterProxy implements LayoutInflaterFactory {

private final AppCompatViewInflater appCompatViewInflater;

public AppCompatViewInflaterProxy(Context context) {
    appCompatViewInflater = new AppCompatViewInflater(context);
}

@Override
public final View onCreateView(View parent, String name, @NonNull Context context, @NonNull AttributeSet attrs) {
    return appCompatViewInflater.createView(parent, name, context, attrs, false, Build.VERSION.SDK_INT < 21);
}

}

@rjrjr https://github.com/rjrjr if it's ok, I'll open a PR for mortar sample with it.

— Reply to this email directly or view it on GitHub https://github.com/square/mortar/issues/145#issuecomment-99347340.

Lakedaemon commented 9 years ago

Has this been reported to Google so they fix their Appcompat library ?

deleet commented 9 years ago

With the new Support Library update released today (version 22.2.0), you must now pass the following params onto the AppCompatViewInflater.onCreateView method:

/**
 * This class is responsible for manually inflating our tinted widgets which are used on devices
 * running {@link android.os.Build.VERSION_CODES#KITKAT KITKAT} or below. As such, this class
 * should only be used when running on those devices.
 * <p>This class two main responsibilities: the first is to 'inject' our tinted views in place of
 * the framework versions in layout inflation; the second is backport the {@code android:theme}
 * functionality for any inflated widgets. This include theme inheritance from it's parent.
 *
 * <p> Mortar Context is preserved.
 */
public class AppCompatViewInflaterProxy implements LayoutInflaterFactory {

    private final AppCompatViewInflater appCompatViewInflater;

    private final boolean isPreL = Build.VERSION.SDK_INT < 21;

    public AppCompatViewInflaterProxy() {
        appCompatViewInflater = new AppCompatViewInflater();
    }

    @Override
    public final View onCreateView(View parent, String name, @NonNull Context context, @NonNull AttributeSet attrs) {
        return appCompatViewInflater.createView(parent, name, context, attrs,
            false, /* Don't traverse up the view hierarchy to obtain the root context since we're using Mortar's */
            isPreL, /* Only read android:theme pre-L (L+ handles this anyway) */
            true /* Read read app:theme as a fallback at all times for legacy reasons */
        );
    }
}
jrgonzalezg commented 9 years ago

Is this not needed with Mortar 0.18? It seems to be the case on my projects, but i do not know if there are still some edge cases where this patch is still needed.

almozavr commented 8 years ago

No longer needed (and won't work via AppCompatViewInflaterProxy) with new 23.1.1 support libs.