Migration to Compose/modern app architecture

[emilymclean]

This discussion serves as a starting point to plan a long term improvement in the app architecture as well as a full migration to compose, continuing on from discussions in #866.

[emilymclean]

Essentially, I think it is worth using the announcement that traditional Views are in maintenance mode as an opportunity to refactor and improve the app architecture with a view towards migrating completely to compose. This would be a long term project and far beyond the scope of a single pull request, so I think it would be valuable to put some effort into planning the transition and the ultimate end product. Hopefully in the end we can have a semi-formal plan + architecture we can point to for future contributions.

Problems

Admittedly, I'm not yet fully across the codebase of this app, so I think other contributions would be valuable here, but one of the most obviously problematic components I can identify is the LocalBroadcastManager. The library itself has been deprecated since 2022, and I think their reasons for doing so apply to us as well:

LocalBroadcastManager is an application-wide event bus and embraces layer violations in your app; any component may listen to events from any other component.

I've modeled a real example of this use of the library to violate layering in the current codebase:

sequenceDiagram 
participant UI as UI (Note/Main ViewModel) 
participant UC as BookSparseTreeForNote (UseCase) 
participant DR as DataRepository
participant LBM as LocalBroadcastManager 
participant MA as MainActivity 
participant DM as DisplayManager
Note over UI, UC: Triggered by user or link
UI->>UC: UseCaseRunner.run(BookSparseTreeForNote)
UC->>DR: openBookForNote(noteId, sparseTree=true)

Note over DR, LBM: Communication
DR->>LBM: sendBroadcast(ACTION_OPEN_BOOK)

Note over LBM, MA: Delivery
LBM-->>MA: onReceive(intent) [LocalBroadcastReceiver]

Note over MA, DM: Navigation
MA->>DM: displayBook(bookId, noteId)

Note over DM: UI Update
DM-->>MA: Load BookFragment with focus on note

Loading

As you can see in this particular example, the data layer directly triggers a page change when that very explicitly should not be its concern. In my opinion, as recommended in the previously recommended deprecation notice, we should be replacing uses of the LocalBroadcastManager with either (when appropriate) more direct invocations of functionality or shared reactive state. In the case of this particular example, opening the BookFragment should be moved to the Fragment call site after the actual DB transaction performed by the DataRepository function is completed. In it's usage in the RefileFragment I would add an EventFlow which is triggered after UseCaseRunner.run(BookScrollToNote(noteId)) completes which the fragment subscribes to and actions, ala:

sealed interface RefileEvent {
  data class NavigateToNote(val noteId: Boolean): RefileEvent
}

// ...

class RefileViewModel {
  private val _events = EventFlow<RefileEvent>()
  val events = _events.asFlow(viewModelScope)

  fun goTo(noteId: Long) {
    viewModelScope.launch(Dispatchers.IO) {
      UseCaseRunner.run(BookScrollToNote(noteId))
      _events.send(RefileEvent.NavigateToNote(noteId)
    }
  }
}

// ...

class RefileFragment {
  viewModel.events.collectWithLifecycle { /* Do navigation here */ }
}

Furthermore, the use of MainActivity as an orchestrator is a decision I disagree with. I'm of the opinion that whatever is done there should either be pushed up into shared state or done in a more direct manner. Likewise, I don't like how the DataRepository is a mega class that represents all kinds of transformations and actions that are performed on data. For readability sake I would split it into more specialized repositories that represents distinct data groups in the app. There is also code in there that would be better suited for a UseCase. While I'm on the subject, UseCases are also structured opposite to what I would expect, which is that there is a "run" or "invoke" function which accepts parameters and a constructor which accepts dependencies.

Where I would start

Firstly, as a matter of policy, I would decide that we are migrating to a shared state model as well as modern "jetpack" solutions - this includes migrating to Coroutines and Flow. To enable contributions while migration is underway, the best bet would be to write wrappers for existing legacy code we identify as problematic, such as I have done with the display manager with Navigator. For the LocalBroadcastManager specifically, can design components that purport to represent "best practice" architecture but are actually implemented using the library under the hood until we get around to fixing it.

I would slowly split out repositories from the DataRepository and at the same time take it as an opportunity to return data as a Flow/use suspend functions. This might also be an appropriate place to use a facade so new contributions work with the eventual structure we would like. I would further move the current logic handled by UseCaseRunner into separate use cases for sync, local data modification, and list widget updates. That would eliminate the need for the harness altogether and allow new use cases to be written with a more conventional structure (+ old ones migrated over time) and using coroutines + flow.

Placing data logic into the ViewModel and business logic in UseCases would be enforced and migrated towards. Considering best practices re: ViewModels and Compose, all should eventually have a single state object Flow which fully represents UI state and a Flow which represents events. My work in SavedStateViewModel demonstrates a way to do this.

That's a lot of it, I would be very interested in hearing feedback, disagreement or other suggestions. Obviously I'm quite new to the project and don't have as in-depth knowledge of the codebase as I'm sure other contributors may have, so I'm very flexible re: my suggestions.

[emilymclean]

Another major improvement we could make is finally migrating from the retained SyncFragment to a ViewModel. This appears to have been planned since 2018 judging by the comment, finally doing this would be a major improvement and eliminate most of the need to use MainActivity as an orchestrator.

Individual views can instead interact with an activity scoped ViewModel or (my preferred solution) we create a near Application lifetime supervisor scope that can be used (indirectly and in a very limited way) by ViewModels or other tasks to perform long running tasks. Considering the current SyncFragment is essentially retained for the entire application lifecycle, I don't think there is much of a practical difference between our current setup and an application scope besides a cleaner architecture - however both have the same downsides, namely that a task executed by them could cause memory leaks if they never terminate. If we do go this route, we should be careful to ensure that the scope is only used when absolutely necessary and maybe even the scope is cancelled in low memory conditions.

[sudhirkhanger]

Thank you for the write up. Sadly, am not familiar with the structure of the project. But as a user of the project and as an Android developer I have a few thoughts.

1. Support for Compose Multi-platform.
2. Multi-module setup.
3. AI agent readiness.
4. UI refresh
5. Dependency Injection

At an architectural level we should be aware of where do we want this project to be. And how do we enable future contributions based on the new architecture.

With that I will go back and try to read the project further.

[emilymclean]

(I'm not back yet, but I thought it was worth responding to this comment regardless)

These are some good points, so I wanted to add my two cents.

1. Compose Multi-Platform support would be amazing, but I would expect that to be a very distant goal. Currently, a large portion of the codebase is still written in Java, and I would expect any migration to be gradual as views are rewritten. Likewise, many of the libraries currently in use are Android only, notably org-java, but also Dagger, juniversalchardet, all the git features, and documentfile. With the exception of Dagger (which has Koin), I don't think these have multiplatform alternatives. It might be worth investigating alternative ways of replicating the functionality in the other libraries but, personally, while I think we should keep multiplatform in mind while making these plans, I wouldn't make it a primary goal.
2. Multi-module would be a great add and allow us to neatly separate API/implementation + even use dynamic features in the future.
3. Frankly AI agent readiness this isn't something that I am particularly concerned with nor am I particularly versed in achieving, so some more in-depth input from anyone who does find that important would be valuable. Would this mainly involve setting up some AGENTS files?
4. A UI refresh would be excellent, and it is something I would very much like to aim for. I also suspect it will start more than a few fights, especially since I am generally against supporting old UIs since that more than doubles the maintenance burden in my experience. I think it would be good to start a (separate??) discussion where we can design new screens with community input.
5. Dependency injection is an interesting one, because it is already set up but a bunch of places in the codebase just doesn't use it for some reason. Consistently using it is an "easy" win (it isn't that easy because many places just instantiate DI components inline where Dagger can't inject).

In terms of architecture, at least in my opinion this app would have a multi-module structure. We would have a layer of "base" configuration modules, these configure individual libraries and such. So in our case, that would entail at least one Compose module, maybe even more, where we define the theme and widgets. From there we would have various feature modules which implement various domain areas, having a strict :api and :impl separation. This would probably have a core :feature:book feature, with further :feature:calendar, :feature:reminder, :feature:search, :feature:sync. The repos would be split off into :repo:directory, :repo:git, etc modules. Finally, I would have a shared :database module just for the sake of having nice ACID and single SoT, but that would only expose minimal implementation details (i.e. just the DAOs).

Internally, each module would have a full data, domain, and (if necessary) presentation layer. They would fully use coroutines and flows to cooperatively multitask and passively subscribe to state changes. I like to maintain a repository/client/persistence split when I code apps, but some people have different preferences. All these modules would expose interfaces/contracts through their :api module, which are then fulfilled by the :impl and all tied together with dagger.

Obviously this would be a pretty radical change that would break every in-progress PR, which is why I suggest a more staged transition. If we split the DataRepository up into smaller domain separated ones that double it, deprecate it, migrate existing usage, then remove it, we can more gracefully prepare for the change. This would also force us to migrate all the UseCases. Same for the UI, if we gradually bring it in-line with best practice it should be fairly easy to split out once we're ready.

[emilymclean]

Oh also another thing we should do is, once we're done with this discussion here, just create a branch for the future next version of Orgzly (2.0) and make substantial changes there. The main branch is still the default target and we just periodically merge it into the 2.0 branch until we're ready to make 2.0 live. That means we can make changes specifically just for 2.0 without breaking development of the existing app.