nextcloud / android

📱 Nextcloud Android app
https://play.google.com/store/apps/details?id=com.nextcloud.client
GNU General Public License v2.0
4.3k stars 1.77k forks source link

Make use of View Binding #6247

Open stefan-niedermann opened 4 years ago

stefan-niedermann commented 4 years ago

As an intermediate step to #3146 i propose to use View Binding (this is not Data Binding).

I think Data Binding is still interesting, but it requires huge efforts to make it work nicely everywhere, while View Binding is the "little brother" - better than the current state but way easier to switch compared to Data Binding.

I'd like to hear your opinions @tobiasKaminsky @AndyScherzinger @ezaquarii

If everyone agrees i could create a initial Pull Request to add support in a few cases to demonstrate it on a concrete sample.

AndyScherzinger commented 4 years ago

@stefan-niedermann afaik we already decided to go for view binding replace the now deprecated Butterknife usage 😉

cc @tobiasKaminsky @ezaquarii

AndyScherzinger commented 4 years ago

@stefan-niedermann I also think @tobiasKaminsky already implemented this in 1-2 cases in the app :)

tobiasKaminsky commented 4 years ago

Yes, we want to do this. But it is some sort of time tradeoff, so at least I want to do it only for new code or code that I touch. Changing existing code without enhancing anything, I do not have time for this.

But if you are up to this, feel free to do it :+1:

As this is discussed, I will close it.

ezaquarii commented 4 years ago

@stefan-niedermann CC @tobiasKaminsky

The value of migrating existing code to view bindings is IMO limited, as the view bindings solve a trivial problem. It buys minimal amount of code reduction for a price of extra dependency on non-trivial compiler tooling that can be poorly maintained, buggy or just abandoned by it's maintainers. Plus one needs to understand this piece of magic to work with it. Value null safety is a value-added proposition, but this class of bugs is not very pressing concern in that segment of code and are trivial to spot as well.

The platform's API findViewById(int) is simple, well understood (one doesn't sweat over a trivial getter) and doesn't require extra compiler toolchain to work.

Not that I want to be negative, but if you are looking for an area where you could have an impact, migration to some form of an observable ViewModel yelds immediate, massive improvement of overall maintainability. This can also be done in small steps, chipping out bigger problems over time - even doing it for a trivially small portion of UI establishes a good position to launch subsequent refactoring campaings.

Generally, when doing refactoring of big codebase, one would aim at doing structural changes enabling proper engineering solutions, rather than jumping on the corpse with a fancy mortuary makup kit.

I think Data Binding is still interesting, but it requires huge efforts to make it work nicely everywhere

Data binding is natural way of doing UI, but it becomes economical when you have observable state that one can project onto the screen. Most of Android applications generally do not meet this prerequisite, being usually a hairy mixture of business logic, distributed (!) state and UI update code bullet dancing shots from lifecycle callbacks. Data bindings will surely save few calls to getters and setters, but typing is not a bottleneck and the complexity is elsewhere.

If everyone agrees i could create a initial Pull Request to add support in a few cases to demonstrate it on a concrete sample.

I have nothing against it, given that view bindings are already used in our build process. My main point is that if you are looking for a positive impact, this is not the most economical use of your time. You can find really loud bang for small bucks in ViewModel migration and we have pretty good idea how this is done. Please feel invited. :)

stefan-niedermann commented 4 years ago

@tobiasKaminsky mentioned nearly the same points in a private chat with me and also suggested to fix some bugs instead 😄

The value of migrating existing code to view bindings is IMO limited, as the view bindings solve a trivial problem

The first PR with only one migrated file reduces the code base by 100 lines of code (and many methods).

for a price of extra dependency on non-trivial compiler tooling that can be poorly maintained, buggy or just abandoned by it's maintainers.

This is not ButterKnife but it is developed, maintained and recommended by Google directly. It's introduction a few months ago made ButterKnife deprecated because "there is finally a well supported official solution". Actually it's on the same level of support as View Binding. (see my link in the first comment).

The platform's API findViewById(int) is simple, well understood

It's more to write, needs casts and when you change the type e.g. from LinearLayout to RelativeLayout you will not notice this on compile time but (hopefully) only on runtime.

I agree that LiveDatas and Data Binding should be the long term goal, but i

So: Take it or leave it, but please tell me before i do it.

tobiasKaminsky commented 4 years ago

So: Take it or leave it, but please tell me before i do it.

Do it :heart: It might come the time to split it more up, to get it to a modern code base, but for now it does not hurt.