Application Architecture Modernization (Modern Android Development / MAD)

[mcumings]

The current application suffers from a lot of artificial complexity resulting from not having a well defined architecture. Additionally, there are some bad practices which have become prevalent and are impacting app performance. Some examples of this include:

• Blocking I/O on the main thread. Examples:
  • Database is being initialized on the main thread
  • SharedPreferences updates are using commit versus apply
• Lots of business logic in the view layer (which happens to primarily run on the main thread)
• Poor encapsulation. Examples:
  • mesh onConnected calback being used to set UiViewModel.meshService field
  • MeshService.kt has LOTS of different responsibilities (and is nearly 1900 lines of code!)
  • Individual files defining multiple/many classes
• State is being managed in a variety of locations:
  • Different node databases in UiViewModel and MeshService

This tends to make the app difficult to understand and more fragile when it comes time to make changes. By applying the architectural principles in Google's recommended application architecture (https://developer.android.com/topic/architecture) many of these complexities will be naturally eliminated. For example:

• Using dependency injection can allow portions of the app to remain decoupled from each other, allowing them to be more freely changed, independently of one another while hiding their implementation details.
• Driving the UI directly from data models can create a unidirectional data flow (e.g., radio -> database -> view model -> view) which can make it easier to debug the application as well as put all application state into a place which supports transactional change.
• Moving business logic out of the view layer and into domain and/or data layers allows for easier combination of discrete portions of the application and makes each component much more testable in isolation.

This issue is a proposal to shift the app closer to this recommended architecture in small, incremental steps.

[andrekir]

considering your review I would say database needs the most attention. MeshService is the heart of the project and basically shares code between all different platforms. so we might leave that for last.

if there are tasks we can split or anything I can help, let me know.

[mcumings]

Sounds good. The database (and state storage in general) is definitely a good place to start. It fans out pretty rapidly though. For instance, I started building up a SharedPreferences replacement in the database and found that I couldn't wire it up at all points of use until I take a step back and get those points of use natively injected, first. i.e., there's a lot of static coupling.

I'll definitely start there and keep pulling the thread to see how much unravels. 😄

[mcumings]

Ok, I think that starting with preferences is sub-optimal for one main reason: when we change the database schema we either need to blow away the existing data or we need to define migrations to mutate the database schema over time, properly.

Preferences are one of those things which should remain persistent, indicating that the database would need to be leveraging migrations.

I think it would be easier to work on the general database schema dealing only in terms of data which can be transparently discarded upon app upgrade, allowing us to avoid the cost of having to spin up migrations until later, at which point we can implement preferences.

i.e., as a practical way of avoiding the headache of proper data migrations until we absolutely have to

[pmarches]

Hello,

I think this merge broke HEAD. This is the stack trace I get when trying to start the app with a clean checkout. I have no idea how to resolve this, but I was able to run the application 2 weeks ago.

Let me know if I can help in some way

FATAL EXCEPTION: main
Process: com.geeksville.mesh, PID: 27030
java.lang.RuntimeException: Unable to start activity ComponentInfo{com.geeksville.mesh/com.geeksville.mesh.MainActivity}: java.lang.IllegalStateException: Scoped provider was invoked recursively returning different results: com.geeksville.mesh.repository.radio.RadioInterfaceService@7528dca & com.geeksville.mesh.repository.radio.RadioInterfaceService@9961b3b. This is likely due to a circular dependency.
    at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3449)
    at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3601)
    at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85)
    at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
    at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
    at android.os.Handler.dispatchMessage(Handler.java:106)
    at android.os.Looper.loop(Looper.java:223)
    at android.app.ActivityThread.main(ActivityThread.java:7656)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Caused by: java.lang.IllegalStateException: Scoped provider was invoked recursively returning different results: com.geeksville.mesh.repository.radio.RadioInterfaceService@7528dca & com.geeksville.mesh.repository.radio.RadioInterfaceService@9961b3b. This is likely due to a circular dependency.
    at dagger.internal.DoubleCheck.reentrantCheck(DoubleCheck.java:66)
    at dagger.internal.DoubleCheck.get(DoubleCheck.java:48)
    at dagger.internal.DelegateFactory.get(DelegateFactory.java:36)
    at com.geeksville.mesh.DaggerMeshUtilApplication_HiltComponents_SingletonC$ActivityCImpl.injectMainActivity2(DaggerMeshUtilApplication_HiltComponents_SingletonC.java:579)
    at com.geeksville.mesh.DaggerMeshUtilApplication_HiltComponents_SingletonC$ActivityCImpl.injectMainActivity(DaggerMeshUtilApplication_HiltComponents_SingletonC.java:548)
    at com.geeksville.mesh.Hilt_MainActivity.inject(Hilt_MainActivity.java:69)
    at com.geeksville.mesh.Hilt_MainActivity$1.onContextAvailable(Hilt_MainActivity.java:40)
    at androidx.activity.contextaware.ContextAwareHelper.dispatchOnContextAvailable(ContextAwareHelper.kt:84)
    at androidx.activity.ComponentActivity.onCreate(ComponentActivity.java:358)
    at androidx.fragment.app.FragmentActivity.onCreate(FragmentActivity.java:216)
    at com.geeksville.mesh.MainActivity.onCreate(MainActivity.kt:188)
    at android.app.Activity.performCreate(Activity.java:8000)
    at android.app.Activity.performCreate(Activity.java:7984)
    at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1309)
    at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3422)
    at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3601) 
    at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85) 
    at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) 
    at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) 
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066) 
    at android.os.Handler.dispatchMessage(Handler.java:106) 
    at android.os.Looper.loop(Looper.java:223) 
    at android.app.ActivityThread.main(ActivityThread.java:7656) 
    at java.lang.reflect.Method.invoke(Native Method) 
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947) 

[andrekir]

@pmarches thanks for reporting. not seeing any issues but there are more changes on the way, including RadioInterfaceService moving out of MainActivity (which should resolve your issue). hang tight.