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

[NO-TICKET] Update dependencies and sample project to use ViewBinding #2

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

Conversation

emeruvia
Copy link

@emeruvia emeruvia commented Apr 1, 2022

With Kotlin synthetics being deprecated, sooner or later we will want to update our projects to use ViewBinding instead.
This PR updates the sample project to use ViewBinding instead of Kotlin synthetics. It also updates the library dependencies.

@emeruvia emeruvia self-assigned this Apr 1, 2022
@flopshot
Copy link
Contributor

flopshot commented Apr 1, 2022

👋 @emeruvia Let me know if you need any help.

…ds a generic ViewBinding parameter for the user to inject the generated ViewBinding file. This gives the option for users wanting to use ViewBinding a failsafe way of doing so, while keeping the regular ComponentView class untouched.

- Create a example-viewbinding module, which showcases how to use ComponentViewBinding
- Remove kotlin-extensions. This turns off kotlin synthetics
- Update library compileSdkVersion 29 -> 31
- Update library buildToolsVersion 29.0.3 -> 30.0.3
- Update library dependencies, recyclerview updated 1.1.0 -> 1.2.1
- Remove JCenter in favor of mavenCentral
- Update gradle version 4.0.1 -> 4.0.2
@emeruvia emeruvia force-pushed the feature/viewbinding branch from f6e7e63 to ac33fe0 Compare April 29, 2022 21:47
@emeruvia
Copy link
Author

Hi @flopshot 👋
Sorry for the delay, I just got some free time to circle back to this update.
I've added a ComponentViewBinding class which implements ComponentView and adds a generic ViewBinding parameter. I chose this approach so that any user wanting to use ViewBinding is forced to add the XML generated class, and so that they can add its initialization and safely remove Kotlin synthetics from any component view.

With this PR, I've also an example-viewbinding module, so that we can keep the samples in their separate files without removing any existing code.

Let me know what you think!

Comment on lines +10 to +14
* This forces any class implementing the ComponentViewBinding to add the respective ViewBinding file.
*/
abstract class ComponentViewBinding<Component: com.seannajera.dkouple.Component, ViewBinding>(
view: View
) : ComponentView<Component>(view) {
Copy link
Contributor

@flopshot flopshot May 8, 2022

Choose a reason for hiding this comment

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

question Is there a reason the library would want to force someone to use ViewBinding over other view inflating methods?

import com.seannajera.dkouple.DKoupleView
import com.seannajera.dkouple.FactoryView

@SuppressLint("NonConstantResourceId")
Copy link
Contributor

@flopshot flopshot May 8, 2022

Choose a reason for hiding this comment

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

nice-to-have AGP 8 will break the code generation feature of DKouple (they keep delaying this change so they might delay it further, but one should assume this time it will happen). This won't break the reflection based layout id lookup from the annotation at runtime in the adapter nor will it break a non-generated factory.

I've never profiled the annotation reflection for performance, but any performance hit was necessary to make the code generation work as well as to make the api look nicer.

Now that annotations won't work for that use case, it would be nice to redesign DKouple to not use them. I'd love to explore using KSP instead which would be WAY easier to maintain than an annotation processor.

This could be a part of a larger/separate issue.

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.

2 participants