Implement View binding, replacing synthetics

[faranjit]

This pr has opened regarding the issue #628. This pr does not contains PhotoEditor.kt because that file has some logics.

[mzorz]

First of all thank you very much for your contribution here! We really appreciate it, and we apologize for the time that has passed. It's sometimes difficult to juggle priorities, but always happy to see contributions of this kind. Very helpful!

I was finally able to dedicate a good amount of time to analyzing your PR and the approach, I'm summarizing my thoughts here.

I changed the base branch to a feature branch we're going to utilize to build changes on it before we merge to develop.

Smoke test

I gave it a smoke test and it seems to be working well, except that the text color picker bottom sheet only shows the different colors when tapping on each color item in the selector.

Checked and spotted where this issue was coming from, fixed in 28f28f5 (going to open a different PR with that).

Plugin declarations / build.gradle

I checked the init declaration in gradle and thought of changing it to:

 buildFeatures {
        viewBinding = true
    }

as indicated in https://developer.android.com/topic/libraries/view-binding, but no big deal really. This can be changed later if needed 👍

2. I checked the parcelize mention here https://developer.android.com/topic/libraries/view-binding/migration#gradle

https://developer.android.com/kotlin/parcelize

If your app does not use Parcelize features, remove the line that enables Kotlin Android Extensions:

plugins {
    kotlin("android.extensions")
}

for our case I think it'd be enough to remove and add kotlin-parcelize. I tried this and realized that plugin is available since Kotlin plugin 1.4.20 (https://kotlinlang.org/docs/whatsnew1420.html#kotlin-android-extensions), but we're on 1.4.10 to match the version used in WordPress for Android. So, we won't be able to change it for the time being. I think it's fine, creating an issue to keep track of it here. #686

General code inspection

3. binding.root
   In https://github.com/Automattic/stories-android/pull/634/files#diff-fef1400542cc90dfd5ea00a0bebb866cbedfbbe92962212f9926ce38d3dd52caR41
   Nice touch here setting the insets on the root view 👍

4. ByFragmentViewBinding (FragmentViewBindingDelegate)

I can see you were using the boilerplate code from this gist which in turn is explained as part of this article here but turns out was not immune to how certain Fragments lifecycle is handled, and was updated here.

We could update the code to use observeForever and match it to the updated gist version, but this made me think a bit further: I think the view binding as it comes from jetpackX does not add too much boilerplate code as opposed to this abstract class. While I like the idea and how neat and clear the code looks when used in Activities or Fragments, I think following up the standard suggested pattern is also easier to read and maintain by other developers, should a bug occur (related or unrelated) in the future. In fact the delegate itself has a good level of complexity (one could argue that's exactly the beauty of it, that it hides complexity), but often times hiding / abstracting complexity also makes bugs potentially harder to find. In seeking a balance between readability, maintainability and the extent to which boilerplate code runs, I think it's better to give explicit meaning than implicit meaning.

An interesting discussion held internally about this shows that there is not a whole lot of agreement on this area, or to put it some way, a consensus is still in development. The discussion in this PoC PR also brought up the finding that the same updated gist as used in this PR works with using observeForever, overcoming the lifecycleOwner state when the observer gets added. (*)

What my thoughts are: using the viewDelegate makes coding faster, but finding a bug may become harder. I'd be up for using this in a smaller app, but I wonder whether we're willing to introduce this in such a large codebase as the WordPress for Android is. In the case of WordPress, the changes there are so huge and widespread that they need be done in partial ways and in a way which is easier to understand and is traceable. Hence the "first pass" on all applicable classes was done by means of applying the Google recipe first, with taking care of nullifying the var binding in onDestroy for Activities and in onDestroyView in the case of Fragments. This work can be seen in this list of PRs here.

Interestingly, this seems to be an ache for many developers. One of the problems outlined in omitting nullifying the binding field as expressed here woocommerce/woocommerce-android#3764 could be fixed by the delegate introduced here in this PR, if we also took care of changing the observer to observeForever.

5. ByActivityViewBinding (ViewBindingPropertyDelegate)
   https://github.com/Automattic/stories-android/pull/634/files#diff-5544b4433760a3fb190c372fae26d910ea8c16d53ee32e044a1d1862479cc3d4R1

Digging a bit more, I realized the FragmentViewBindingDelegate is in fact published as a library in https://github.com/Zhuinden/fragmentviewbindingdelegate-kt. I checked the license and we should be good with that in terms of license compatibility as well.

But, unfortunately ViewBindingActivity is not part of the same library. I wish this one was part of the same library, so we could use it homogeneously across codebases.

Finally, I'm now understanding a bit more how this PR here brings both these concepts to an upper level by implementing an homogeneous interface with these 2 classes:

• abstract class ViewBindingFragment
• abstract class ViewBindingActivity

That is neat! 🙌 !

6. Preliminary conclusion

So back to the discussion about the approach, certainly the Stories library is in a better spot to implement changes such as this one as it's not a huge codebase to work with in this regard.

In terms of how to integrate these solutions, I wish we could implement the library and also incorporate the gist https://gist.github.com/seanghay/0fd991d7a823815500557fe043b052ce#file-view-binding-ktx-kt

Setting that apart, in general I'd be up to go for it, with a few changes as explained above, plus the needed conflict resolution.

I'm taking care of that in a subsequent PR to make it easier 👍

(*) paqN3M-hV-p2

[mzorz]

I resolved the conflicts with the base branch, merging now 👍 (will test and fix any other issues from the view binding update against the feature branch)

[faranjit]

Hi @mzorz,
Thank you for your detailed review and I am so sorry for late response. I have just read all of your comments and any other mentions. These are very educational :) but unfortunately I am late to solve them because you already did. Thanks to you I noticed my mistakes. I am going to be more careful in next pull requests. See you then.

[mzorz]

Thank you @faranjit! The delay was on our part first so, no problem :). Looking forward to more contributions for sure, I always enjoy learning from reading and analyzing other people's approaches to things and yours was certainly carefully crafted and very tidy which I always appreciate. Also always happy to see the library is helping anyone out there who wants to use it 👍 🙇