openhab-android
openhab-android copied to clipboard
Use view binding to get rid of findViewById
Using findViewById is somewhat error prone due to the lack of a direct relationship between the XML description and the usage in code. View binding automatically generates code for creating that relationship.
I've sat on this for quite some time now, for two reasons:
- Ensuring type safety was the cause for larger refactoring in
WidgetAdapter, which caused that class to even grow (other code paths became smaller due to less internal member variables andfindViewByIdcalls) - Our duplicated widget list layouts (normal vs. compact) stretch the design of view binding quite a bit. I'm using the binding class for the normal layout for both cases, which has a consequence on layout compatibility: previously both layouts just needed to share view IDs, now the whole hierarchy needs to be the same (that is, the same classes, XML attributes obviously can differ). Whether that works or not is determined at runtime.
For the latter point, writing a unit test should be easy in theory (have a list of widget-layout-binding to compact-widget-layout-binding, inflate the former, bind the latter to the root view of the former), but this needs to be an instrumented Android unit test, and I'm not sure if and how this can be run in GH actions.
@mueller-ma Please let me know your thoughts on this.
TODO:
- [x]
PrimaryServerPreferencecurrently crashes on bind
Nice work! This is something I also wanted to do at some point. I'll have a look at running the emulator in CI, but for now you can add the test case to the repo and execute it locally.
Added the test, and it even found something ;-) Unfortunately it doesn't seem possible to exempt single views from being included in the binding.
@mueller-ma This conflicts with #3898, I guess check that one first? I'll rebase and undraft this one afterwards.
Still some findViewById left:
I rebased on main locally and the color temperature widget is completely empty:
Still some findViewById left:
I don't think we can do much there:
- In the various
PreferencesubclassesfindViewByIdis used to look up the view set aswidgetLayoutResource, which in turn is just a partial view hierarchy: thePreferencebase class expands it into its own view. I'm not aware of a way of creating a view binding for it. - For the
AbstractWidgetBottomSheet, we're peeking into the internals ofBottomSheetDialogFragmentthere, so we can't create a view binding for that either.
I rebased on main locally and the color temperature widget is completely empty:
I'll check this.
color temperature widget is completely empty
Fixed now.