Initial checkin of the ViewPump lib #2
Conversation
| } | ||
|
|
||
| dependencies { | ||
| compile 'com.android.support:appcompat-v7:24.0.0' |
There was a problem hiding this comment.
We shouldn't make these required dependencies. I would prefer provided and your tests/demo app build it in.
There was a problem hiding this comment.
that's a good point. that should help avoid dependency messes
| return; | ||
| } | ||
|
|
||
| final Method setPrivateFactoryMethod = ReflectionUtils.getMethod(LayoutInflater.class, "setPrivateFactory"); |
There was a problem hiding this comment.
A TODO: we need to get this and wrap it if something has already set this.
There was a problem hiding this comment.
will add a TODO comment to cache the Method. please correct me if i've misunderstood
| protected View onCreateView(String name, AttributeSet attrs) throws ClassNotFoundException { | ||
| return ViewPump.get().inflate(InflateRequest.builder() | ||
| .name(name) | ||
| .context(getContext()) // TODO: is this OK? before was fallbackView.getContext() |
There was a problem hiding this comment.
What was the old code in Calligraphy? I have a feeling it should use it's parent view where possible, otherwise theme styles get lost.
There was a problem hiding this comment.
The code in Calligraphy was here. At this point, we haven't inflated the view yet, so we can't pass the view's context to the InflateRequest, we only have access to the layout inflater's context. is this ok? if not, what would be an example where something Calligraphy would not be styled properly?
There was a problem hiding this comment.
Actually, I looked into this further, and it appears as though previously, when LayoutInflater.createView(name, prefix, attrs) was called, it used the mContext from the LayoutInflater, so by using getContext(), which returns mContext, I am using the same Context [link to source]
| View view = null; | ||
| for (String prefix : sClassPrefixList) { | ||
| try { | ||
| view = inflater.createView(name, prefix, attrs); |
There was a problem hiding this comment.
this should short if view != null See: chrisjenx/Calligraphy#361
There was a problem hiding this comment.
oh good call, will fix
| <?xml version="1.0" encoding="utf-8"?> | ||
| <resources> | ||
| <item name="viewpump_tag_id" type="id"/> | ||
| </resources> |
There was a problem hiding this comment.
Sorry to be super picky, can you leave EOF whitespace?
There was a problem hiding this comment.
np at all, will add. we can get a basic checkstyle added in a future PR and run it on Travis
| buildToolsVersion "24.0.0" | ||
|
|
||
| defaultConfig { | ||
| minSdkVersion 7 |
There was a problem hiding this comment.
I think I mentioned in the other review, that we should probably support 14+ which will simplify the Inflater a little bit.
There was a problem hiding this comment.
sounds good to me. will bump to 14 and remove the Honeycomb-specific code
|
Addressed all comments |
Refactors Calligraphy's LayoutInflater and ContextWrapper into a new library and adds Inflate Request/Response notion plus an Interceptor API