openhab-android icon indicating copy to clipboard operation
openhab-android copied to clipboard

Use view binding to get rid of findViewById

Open maniac103 opened this issue 7 months ago • 3 comments

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 and findViewById calls)
  • 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] PrimaryServerPreference currently crashes on bind

maniac103 avatar Apr 24 '25 12:04 maniac103

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.

mueller-ma avatar Apr 27 '25 14:04 mueller-ma

Added the test, and it even found something ;-) Unfortunately it doesn't seem possible to exempt single views from being included in the binding.

maniac103 avatar May 02 '25 16:05 maniac103

@mueller-ma This conflicts with #3898, I guess check that one first? I'll rebase and undraft this one afterwards.

maniac103 avatar May 06 '25 09:05 maniac103

Still some findViewById left:

grafik

I rebased on main locally and the color temperature widget is completely empty:

grafik

mueller-ma avatar Nov 19 '25 17:11 mueller-ma

Still some findViewById left:

I don't think we can do much there:

  • In the various Preference subclasses findViewById is used to look up the view set as widgetLayoutResource, which in turn is just a partial view hierarchy: the Preference base 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 of BottomSheetDialogFragment there, 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.

maniac103 avatar Nov 20 '25 08:11 maniac103

color temperature widget is completely empty

Fixed now.

maniac103 avatar Nov 20 '25 08:11 maniac103