android
Add ViewGroup.inflate extension
Sorry for opening a PR straight away, but it was so simple I just wrote it.
This is a method I add to every one of my Kotlin projects. The easiest/simplest way is to use it in RecyclerView.Adapter.createViewHolder like so:
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): RecyclerView.ViewHolder {
return RecyclerView.ViewHolder(parent.inflate(R.layout.item, false))
}
but can be used anywhere.
The points I am not sure about:
- It doesn't handle the case where the
ViewGroupcan be null as it would require passing anotherContextas a parameter since we can't use the one from theViewGroup. But that's out of the scope-ish since it's an extension method forViewGroup? - Defaulting
attachToRoottotrue. Mainly to mirror the Android API but as I said the main use case is to use inside aRecyclerView.Adapterin which case it should befalse, but I don't think it's a good enough reason/too confusing.
Anyway thanks for the library, very cool. 👍🏾
Just for consideration. Methods that take a boolean as default are a bit confusing, because one never knows what's the default. This one is specially confusing, as the gut feeling is thinking that the default value is false (as it's mostly used in adapters), but sticking to Android SDK would make one think that the default is true.
What do you think about having two separate methods?
viewGroup.inflate(R.layout.item)
viewGroup.inflateAndAttach(R.layout.item)
The method name says what it's doing without having to go to the code / reference and check what's the default.
I don't see why a default value for a boolean is any different than for other types? The fact that the default matches Android's existing methods makes sense for consistency (and the default value should be documented).
Booleans tend to be confusing for method arguments in general, that was point of my suggestion. There's quite some literature about it: https://martinfowler.com/bliki/FlagArgument.html
But I also see your point, people used to the SDK methods will easily understand this too, and for this library consistency may be better.
I agree that booleans tend to be confusing but this particular API has been around since 1.0 so I think consistency wins here.
I agree that the attachToRoot parameter is the original method is not very clear, but having 2 different methods mean that documentation for these methods must be more throroughly written, whereas here a reference to LayoutInflater.inflate documentation is enough imho.
I think attachToRoot parameter should be false by default. As @antoniolg mentioned that would be mainly used in Adapters where we want false. Being forced to always add that extra parameter will just make me to add my own extension as for many people. This one might be consistent but not so much concise and pleasant as in project description. I think there is no harm with adding some extra documentation to explain why usability here should win over consistency here.
I really don't like having inflate methods use different default behaviors.
I Agree that we should keep constancy with the platform, attachToRoot is by default true if the viewGroup is not null. In the mean time we need to be able to override this behavior (especially for adapter). Actually, We should go deeper, we only need a context to inflate a view, so we should have three extensions like:
fun Context.inflate(@LayoutRes layoutId: Int, root: ViewGroup? = null, attachToRoot: Boolean = root != null): View = LayoutInflater.from(this).inflate(layoutId, root, attachToRoot)
fun View.inflate(@LayoutRes layoutId: Int, root: ViewGroup? = null, attachToRoot: Boolean = root != null): View = context.inflate(layoutId, root, attachToRoot)
fun ViewGroup.inflate(@LayoutRes layoutId: Int, attachToRoot: Boolean = true): View = inflate(layoutId, this, attachToRoot)
As @antoniolg mentioned that would be mainly used in Adapters where we want false
Must say that I use it a lot too in custom views, where the value is usually true.
An alternative is to automatically cast to the receiver type:
@Suppress("UNCHECKED_CAST")
fun <T : View> ViewGroup.inflate(@LayoutRes layoutRes: Int, attachToRoot: Boolean = true): T =
LayoutInflater.from(context).inflate(layoutRes, this, attachToRoot) as T
@tetedoie That sounds good! Could allow handling every case, one of the problems I talked about in my comment. But maybe in different PRs so that it can be discussed more easily?
@clhols Not sure if that's very useful, but I don't really see why not. @romainguy How does it look for you?
Not sure if that's very useful, but I don't really see why not
In my experience, it's helpful in adapters, where you need to use the result of the inflation (to add it to a VH for instance), but not so much in custom views. Kotlin forces you to specify the type, so if you're not assigning the result to a variabe, you need to make it explicit.
It would look like this on a custom view:
inflate<View>(R.layout.view_custom)
It would be consistent with what was done for findViewById: https://developer.android.com/about/versions/oreo/android-8.0-changes.html#fvbi-signature
