commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
988 stars 1.18k forks source link

Migrate to Android Architecture Components #3379

Open ShridharGoel opened 4 years ago

ShridharGoel commented 4 years ago

This is a suggestion to migrate the app to the Android Architecture Components which include ViewModels, LiveData and Data Binding. It is a part of Android Jetpack.

Migrating to MVVM has many advantages like it solves the issue of tight coupling as well as improves testability along with other benefits like the use of lifecycle aware components. MVVM is recommended by Google and is preferred over MVP.

https://android-developers.googleblog.com/2018/05/use-android-jetpack-to-accelerate-your.html

This can be included as a project for GSoC 2020.

Some reference links: https://developer.android.com/topic/libraries/architecture https://developer.android.com/topic/libraries/architecture/viewmodel https://developer.android.com/topic/libraries/architecture/livedata

It would be great if maintainers can share their views on this.

GearGit commented 4 years ago

I think the existing architecture is supporting the app well enough and there is no need to migrate the app to a whole new architecture. @nicolas-raoul @maskaravivek @ashishkumar468 @misaochan What's your take on this?

ShridharGoel commented 4 years ago

I think the existing architecture is supporting the app well enough and there is no need to migrate the app to a whole new architecture.

@GearGit You won't notice much difference in the working of the app even if we don't use any architecture pattern and put all the code into the activities without having presenters. But the actual reason why architecture patterns are used is to make the code more efficient and the development process better. That's why I think migrating to MVVM would be a good idea and would be helpful in the future too since most modern applications follow MVVM.

animeshk08 commented 4 years ago

I also believe the same. Even though Google promotes MVVM. They never suggested, it is the most suitable architecture for every android application. The current MVP architecture looks pretty robust and I dont think we really need to migrate from it.

animeshk08 commented 4 years ago

Your take is right @ShridharGoel . The entire point of an architecture is to improve scalability. That is why we are following MVP architecture. Unless there are specific cases which MVP fails to prove efficient, there should not be a need to switch to MVVM.

ShridharGoel commented 4 years ago

MVP fails to be efficient when testability is concerned. Since there is dependency of views in case of MVP, unit testing becomes difficult. Similarly, there are other things too where MVVM is much better like it solves the issue of tight coupling and the use of lifecycle aware components have their own advantage.

animeshk08 commented 4 years ago

Yes. MVVM does makes testability more effecticient and I think of the main goals of 2020 for commons app was to improve on the unit tests. I think there can be more discussions on this? @maskaravivek @ashishkumar468 @nicolas-raoul sir,please provide your views on this.

maskaravivek commented 4 years ago

@ShridharGoel Thank you for suggestion. We recently moved to MVP after a lot of discussion in #888. Still if you think some of the issues are not handled by our current architecture then it would be great if you take a look a look at our current codebase and list down specific examples where MVVP could have been more efficient.

macgills commented 4 years ago

This discussion should be revived, in my opinion.

MVVM is the standard recommended approach since 2017 and has fantastic support from the modern androidx libraries.

The MVP solution currently has a good few issues with logic not contained to presenters, boolean traps in the api, Presenters named as Listeners, views not passed to constructors and associated errors of persistence across config changes as well as confusing potential states from not being data driven UIs from observable sources.

It would not be necessary to completely remove MVP but a gradual shift and commitment to anything new being in MVVM has great potential for benefit to the project in my opinion.

ShridharGoel commented 4 years ago

@macgills If the team is interested, then I would like to take this up as a GSoC 2020 project. It would be great if this can be added as a project.