Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ViewBinding support #325

Merged
merged 20 commits into from
Mar 31, 2020
Merged

ViewBinding support #325

merged 20 commits into from
Mar 31, 2020

Conversation

nashcft
Copy link
Contributor

@nashcft nashcft commented Feb 22, 2020

This pull request is for discussion in #322

  • Create library-viewbinding module for ViewBinding support
  • Create example-viewbinding module for example application using library-viewbinding

I created these modules based on library-databinding/example-databinding.

build.gradle Outdated
ext.sdkVersion = 28
ext.minimumSdkVersion = 14
ext.databinding_version = '3.2.0'
ext.databinding_version = '3.5.3'
ext.viewbinding_version = '3.6.0-rc03'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated library version for ViewBinding from one for DataBinding because DataBinding above 3.6.0 needs the dependency of ViewBinding library (androidx.databinding:viewbinding).
I think databinding_version and viewbinding_version can be merged when 3.6.0 stable has been released.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.6.0 has now been released

Copy link
Contributor Author

@nashcft nashcft Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up! I already updated the version of viewbinding at 1aace20.

I did not update the version of databinding because of the class declaration change of ViewDataBinding [1][2], but should I also update it to 3.6.0 and merge the variables databinding_version and viewdatabinding_version to one in this pull request?
When I briefly checked behavior of the example app with Android Gradle Plugin 3.5.3 using library-databinding with databinding libraries 3.6.0, no apparent problem was found.

[1] https://androidx.tech/artifacts/databinding/databinding-runtime/3.5.3-source/androidx/databinding/ViewDataBinding.java.html
[2] https://androidx.tech/artifacts/databinding/databinding-runtime/3.6.0-source/androidx/databinding/ViewDataBinding.java.html

*
* @param <T> The ViewBinding subclass associated with this Item.
*/
public abstract class BindableItem<T extends ViewBinding> extends Item<GroupieViewHolder<T>> {
Copy link
Contributor Author

@nashcft nashcft Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK that the names BindableItem and GroupieViewHolder is same as the corresponding classes in library-databinding because they have different package path. If Adding some prefix is preferable, it seems consistent to me that adding it to the classes in both modules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ViewBinding and DataBinding both are supposed to co-exist in a project.
There is a possibility of them being used in the same file (kotlin file with multiple Item class definition)
In such cases, one of the class usage will have to be fully qualified, because of the same name.
I also understand that renaming BindableItem is not a good idea for the existing users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review :)

As I commented below, BindableItem<T extends ViewBinding> should work not only with ViewBinding classes but also with DataBinding classes. So if an app which enables both DataBinding and ViewBinding want to use groupie with its extension library, depending only on library-viewbinding satisfies its demand and collision of class name will not happen.

binding library extension to be used
DataBinding only groupie-databinding
ViewBinding only groupie-viewbinding
Both DataBinding and ViewBinding groupie-viewbinding

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably work the other way around.

binding library extension to be used
ViewBinding only groupie-viewbinding
Both DataBinding and ViewBinding groupie-databinding

We will need to update BindableItem<T extends ViewDataBinding> to BindableItem<T extends ViewBinding> in the groupie-databinding artifact.

But we still need to change the abstract method

@NonNull
    protected abstract T initializeViewBinding(@NonNull View view);

to

    @NonNull
    protected T initializeBinding(@NonNull View view) {
        return DataBindingUtil.bind(view)
    }

This should be overridden with custom implementation when using ViewBinding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to update BindableItem<T extends ViewDataBinding> to BindableItem<T extends ViewBinding> in the groupie-databinding artifact.

This requires the users of groupie-databinding to update Android Gradle Plugin >= 3.6.0 (alpha11) after the update of groupie including ViewBinding support, but I think this may be OK if announcing that the update includes breaking change 👀
In this case, we don't have to add another module, and what to do is to change groupie-databinding to support all of above use-case.

But we still need to change the abstract method ... This should be overridden with custom implementation when using ViewBinding.

This change cannot pass compile because T does not always extends ViewDataBinding, so the user should always override initializeViewBinding even if they use only DataBinding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change cannot pass compile because T does not always extends ViewDataBinding, so the user should always override initializeViewBinding even if they use only DataBinding.

In a project with hundreds of Item classes, It will be a nightmare.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so I don't think adding the consideration to ViewBinding to existing groupie-databinding is good idea and I'd like to add new library (= groupie-viewbinding in this pull request) for the support to both ViewBinding and DataBinding because new library with new feature will not cause breaking change for users of the existing library.
If you are also concerned with the migration from DataBinding-only to both DataBinding and ViewBinding, and you don't want to change existing Item classes extending current BindableItem, then we have no other choice than to use reflection to get bind method from type parameter T. I want to avoid using reflection if possible.

Comment on lines +33 to +34
@NonNull
protected abstract T initializeViewBinding(@NonNull View view);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an abstract method for providing ViewBinding instance as @lkishor suggested in #322 .

@nashcft
Copy link
Contributor Author

nashcft commented Feb 22, 2020

I realized that library-viewbinding should support DataBinding as well as ViewBinding, because from 3.6.0 ViewDataBinding implements ViewBinding so subclasses of ViewDataBinding are assignable to the type parameter of public abstract class BindableItem<T extends ViewBinding> ....

Current library-viewbinding does not work well with DataBinding because I removed a method call of ViewDataBinding#executePendingBindings from BindableItem. I will fix this soon.

Comment on lines 71 to 73
if (binding instanceof ViewDataBinding) {
((ViewDataBinding) binding).executePendingBindings();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will certainly help users who want to use both DataBinding and ViewBinding.
The only downside I see here is, even though the user wants to only use ViewBinding, he will still have to add data-binding as a dependency for this code to even compile.
What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't check that case. Thank you for the pointing :)
Hmm... how about changing dependency declaration for databinding-runtime and databinding-common from compileOnly to implementation? This allows the user to use groupie-viewbinding artifact without enabling DataBinding in their build.gradle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to implementation could solve the compile issue.
But this still adds two extra dependencies databinding-runtime, databinding-common (for users only using ViewBinding), just because we library authors chose to implement it this way.
And no one loves extra dependency, It will come up as another issue later:

Why the heck did I get DataBinding when I add library-viewbinding to my project?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for late reply.

Indeed the dependencies to data binding libraries are redundant for the users only using View Binding, but these are necessary for this library to care about all possible use cases for the safety.
As I commented above, <T extends ViewBinding> accepts the classes generated by Data Binding, which are subclasses of ViewDataBinding, because ViewDataBinding implements ViewBinding since 3.6.0. This means even if we don't intend to support Data Binding in groupie-viewbinding and don't add the implementation pointed in this comment thread, users CAN use this library with Data Binding, and sometimes it doesn't work (for detail, see the pages listing below).

I prefer that the APIs of libraries are designed to prevent wrong usage because API definition is the only way to force its usage, and it is impossible to implement to exclude ViewDataBinding and its subclasses in compile time when considering the implementers of ViewBinding, so I added the consideration for Data Binding in the new library to support Data Binding as a special variation of View Binding.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather we do not force databinding on users who want to use viewbinding. I appreciate you wanting to code the library defensively but I think it's more accurate to have most users of this library not get an extra dependency just because some users might need it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nashcft I still think this should be removed in order to merge this PR.

DataBinding APIs are needed even when the user enables only ViewBinding
@aoben10
Copy link

aoben10 commented Mar 23, 2020

@ValCanBuild Do you know when this will be merged?

Copy link
Collaborator

@ValCanBuild ValCanBuild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off - massive thanks for your work on this @nashcft. Apologies for the late review, but the time I can spend on Groupie has sadly been reduced this year.

Now, for the review. Apologies, I am asking for quite a bit.

  • Can you please update the Readme with an example of how to use Groupie with ViewBinding?
  • In view of modernizing the examples could you convert the example-viewbinding project to Kotlin?

It seems to me that with ViewBinding 3.6.1 we would no longer need the databinding support would we? Anyone who uses databinding 3.6.0+ in their project should just end up using the viewbinding variant instead?

  • If the above is true, then please deprecate all classes in library-databinding with comments for people to switch to viewbinding and we'll end up removing it in a few versions.

build.gradle Outdated
@@ -7,10 +7,11 @@ def testResultsDir = "${rootProject.buildDir}/test-results"
buildscript {

ext.kotlin_version = '1.3.60'
ext.android_plugin_version = '3.5.2'
ext.android_plugin_version = '3.6.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this 3.6.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udpated 🚀 (1c9774e)

build.gradle Outdated
ext.sdkVersion = 28
ext.minimumSdkVersion = 14
ext.databinding_version = '3.2.0'
ext.databinding_version = '3.5.3'
ext.viewbinding_version = '3.6.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this to 3.6.1 too please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udpated 🚀 (1c9774e)

dependencies {
implementation project(':library-viewbinding')
implementation project(':example-shared')
implementation "androidx.appcompat:appcompat:1.0.2"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this to be a more up to date example, please use all latest library versions here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated ✅(98caac2)

Comment on lines 71 to 73
if (binding instanceof ViewDataBinding) {
((ViewDataBinding) binding).executePendingBindings();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather we do not force databinding on users who want to use viewbinding. I appreciate you wanting to code the library defensively but I think it's more accurate to have most users of this library not get an extra dependency just because some users might need it.

@NitroG42
Copy link

NitroG42 commented Mar 26, 2020

Would there be any way to drop ?

    override fun getLayout(): Int {
        return R.layout.mylayout
    }

and use instead:
MyLayoutBinding.inflate(view)

I know that is probably difficult but I think this is wht makes the viewbinding library great.

@nashcft
Copy link
Contributor Author

nashcft commented Mar 28, 2020

Welcome back and thanks for your review, @ValCanBuild :)

It seems to me that with ViewBinding 3.6.1 we would no longer need the databinding support would we? Anyone who uses databinding 3.6.0+ in their project should just end up using the viewbinding variant instead?

DataBinding can be used with the ViewBinding support if keeping the implementation we are discussing on this thread. In example-viewbinding we use both DataBinding and ViewBinding, and it works well. Of course using only DataBinding with the ViewBinding support also works.
But, I'm not sure we have to deprecate library-databinding APIs and remove DataBinding support in the future version. Maybe the discussion from this comment is related.

@nashcft
Copy link
Contributor Author

nashcft commented Mar 29, 2020

@NitroG42
Thanks for your suggestion, but it seems better to discuss it as another topic because dropping Item#getLayout means breaking change of the existing API in core module and it affects other classes 👀

Would you mind creating a new issue about the suggestion?

README.md Outdated
```

Groupie also has a support module for Android's [view binding](https://developer.android.com/topic/libraries/view-binding). This module also supports Android data binding, so if your project uses both data vinding and view binding, you don't have to add the dependency on the data binding support module. [Setup here.](#view-binding)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling correction "uses both data vinding and view binding".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See, the way it's laid out here basically means that you no longer need to use the data-binding module if using gradle plugin 3.6.0. This is why I'm saying we should deprecate it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now I got your point. Thanks for your patience.

README.md Outdated
Groupie also has a support module for Android's [view binding](https://developer.android.com/topic/libraries/view-binding). This module also supports Android data binding, so if your project uses both data vinding and view binding, you don't have to add the dependency on the data binding support module. [Setup here.](#view-binding)

### Note:
`groupie-viewbinding` can also be used for the project using only data binding, but in this case the version of Android Gradle Plugin must be 3.6.0 or more.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct to:
groupie-viewbinding can also be used for projects using only data binding when using Android Gradle Plugin 3.6.0 or higher

```

Groupie includes a module for Kotlin and Kotlin Android extensions. Never write a ViewHolder again—Kotlin generates view references and Groupie uses a generic holder. [Setup here.](#kotlin)

```gradle
implementation "com.xwray:groupie:2.7.0"
implementation "com.xwray:groupie-kotlin-android-extensions:2.7.0"
implementation "com.xwray:groupie:$groupie_version"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah - thats sensible. Should've done it ages ago rather than manually updating it 😂

README.md Outdated
}
```

Because ViewBinding does not have the util class that can generate an arbitrary bindng like `DataBindingUtil` for DataBinding, you need to override ` initializeViewBinding` to generate the instance of specified binding:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bindng -> binding

README.md Outdated
```

Groupie also has a support module for Android's [view binding](https://developer.android.com/topic/libraries/view-binding). This module also supports Android data binding, so if your project uses both data vinding and view binding, you don't have to add the dependency on the data binding support module. [Setup here.](#view-binding)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See, the way it's laid out here basically means that you no longer need to use the data-binding module if using gradle plugin 3.6.0. This is why I'm saying we should deprecate it soon.

private val carouselItem: CarouselItem

init {
this.adapter = adapter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please reformat this whole example project to use an indentation of 4 to keep it in line with the other ones?

Comment on lines 71 to 73
if (binding instanceof ViewDataBinding) {
((ViewDataBinding) binding).executePendingBindings();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nashcft I still think this should be removed in order to merge this PR.

Copy link
Collaborator

@ValCanBuild ValCanBuild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, I'm not sure we have to deprecate library-databinding APIs and remove DataBinding support in the future version

We don't have to but we should since library-databinding will no longer be necessary since library-viewbinding will end up being all a databinding or viewbinding user needs to use. I rather we don't support something that is no longer required or best practise to use.

Please remove the databinding dependency of library-viewbinding and I can approve. Few other minor comments as well - mostly on Readme

@ValCanBuild ValCanBuild merged commit 4b970b0 into lisawray:master Mar 31, 2020
@ValCanBuild
Copy link
Collaborator

ValCanBuild commented Mar 31, 2020

Massive thanks again for the quick work on the feedback @nashcft . I've now merged this and I'll create a new release later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants