Automattic / stories-android

Loop concept app - WP Stories library
GNU General Public License v2.0
17 stars 6 forks source link

Simplify view binding #698

Closed mzorz closed 3 years ago

mzorz commented 3 years ago

Follow up to #628, targets #688

After giving view binding a bit more thought and discussing different points of view, I arrived at the conclusion it'd be best if we kept the Stories library codebase aligned in terms of the usage as per in WPAndroid.

The core of the problem here is to provide an easy way in which Fragments / Activities can use binding without having to remember about dereferencing a class variable that holds beyond the Fragment/Activity lifecycle. Hence the simple to use, yet somewhat complexly crafted solution using lifecycle observers.

I wonder, why don't we get rid of the reference completely? we could do like so like it's done here: https://github.com/wordpress-mobile/WordPress-Android/pull/14596/files#diff-39225ce810fd1f96abbd79fe623a339e99c4bb826c38df330eeb18b36eee9a87R44-R47

       with(FragmentLoginNoSitesEmptyViewBinding.bind(view)) {
            initContentViews()
            initViewModel(savedInstanceState)
        }

And then using Kotlin extension methods as shown there (i.e. iniContentViews and initViewModel in the example above)

Another example can be found here https://github.com/wordpress-mobile/WordPress-Android/blob/70ce7de8817020d29768074bb408688436320220/WordPress/src/main/java/org/wordpress/android/ui/activitylog/list/ActivityLogListFragment.kt#L72-L103

Citing @renanferrari here (ref paqN3M-hV-p2#comment-510):

[...] we came to the conclusion that one of the main advantages of using ViewBindings is the fact that it gives us explicit nullability for our views, meaning we now have a much clearer idea of where exactly our views might be null and where they won’t. This is really good and can prevent a lot of unexpected headaches, while using a delegate would make this implicit again.

In the cases where we needed to hold the binding field, we would end up with something like this:

private var binding: FragmentMainBinding? = null

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
    super.onViewCreated(view, savedInstanceState)
    with(MyFragmentBinding.bind(view)) {
        binding = this
        myFirstField.text = ...
        setupViews()
        setupObservers()
    }
}

private fun MyFragmentBinding.setupViews() {
    mySecondField.text = ... // Explicit nullability using the non-null 'this'.
}

private fun onSomeKindOfCallback() {
    binding?.myThirdField.text = ... // Explicit nullability using the nullable field.
}

override fun onDestroyView() {
    super.onDestroyView()
    binding = null
}

There’s still some boilerplate as we need to set the field to null on onDestroyView() and we’re transforming binding-related functions into extension functions, but I believe this a fair price to pay for more null-safety.

I think we should be able to use it like that in Stories. And when that's not possible (i.e. we still need to keep a reference for other reasons), IMO it's still better to keep it explicit to avoid maintainability problems and / or being able to spot the potential issue in a quicker way.

To test

malinajirka commented 3 years ago

I reviewed the changes in this PR + I tested them in both the sample app and WPAndroid. All looks good to me (we might want to consider making changes in this comment, but we can do that later)