Skip to content

arch: introduce 'Destination' pattern#21023

Open
david-allison wants to merge 4 commits into
ankidroid:mainfrom
david-allison:destination
Open

arch: introduce 'Destination' pattern#21023
david-allison wants to merge 4 commits into
ankidroid:mainfrom
david-allison:destination

Conversation

@david-allison
Copy link
Copy Markdown
Member

@david-allison david-allison commented May 11, 2026

Split into:

Note

Assisted-by: Claude Opus 4.7

Purpose / Description

When we move to feature modules, we need a way to navigate between features without introducing cross-module dependencies (as these will nullify the build time/parallelism goals we hope to coomplish). This PR introduces a Destination class and navigate(Destination)

Sample usage in an Activity/Fragment

// the object should be produced in the ViewModel
navigate(BrowserDestination.ScrollToCard(deckId = 1, cardId = 1))

Fixes

Approach

How Has This Been Tested?

Unit tested

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison david-allison marked this pull request as ready for review May 11, 2026 21:01
@david-allison david-allison force-pushed the destination branch 3 times, most recently from ee2bc18 to bb40df7 Compare May 11, 2026 22:23
Copy link
Copy Markdown
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest splitting the anki-common part from the destination part on different PRs

Currently, I'm still trying to understand the purpose/goal/usefulness of the destination module

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label May 12, 2026
@david-allison
Copy link
Copy Markdown
Member Author

david-allison commented May 12, 2026

I've done some digging and (surprisingly) I don't believe we have cycles in our navigation graph. I'll self-assign and clean up the README.

EDIT: README was clean; I may want a note in com.ichi2.anki.destinations - open to suggestions


When we move to feature modules [for build speed + architecture], we should aim to have no dependencies between feature modules.

  • Cyclic dependencies are not possible. Allowing cross-feature dependencies quickly leads to cycles and build errors
  • Cross-module dependencies slow the build:
    • Transitive dependencies define an ordering, which can't be compiled in parallel

'Destination/navigate' acts as a 'mediator' in Google's terminology, as this is the one place where there would need to be dependencies between features: https://developer.android.com/topic/modularization/patterns#communication

Once we've removed the bulk of cross-feature dependencies via destinations, we can pick higher yield modules to extract, rather than needing to work from the leaf nodes in the nav graph

Also see: https://github.com/android/nowinandroid/blob/main/docs/ModularizationLearningJourney.md#feature-modules


Just to note: a lot of what's in the two linked pages above is far too 'enterprise' for us. End goals:

  • Measurably faster compile + test times due to incremental builds + smaller modules
  • internal reduces public API surface area
  • Better architecture: no cycles, more obvious impact of changes, 'stable core'
  • jvm-library split - no Robolectric overhead
  • 'modules as milestones' to use [for a Compose migration]

This should be a responsibility of the browser, not the destination

Assisted-by: Claude Opus 4.7
* ToCard => ScrollToCard
* Move EXTRA_CARD_ID_KEY to ViewModel

This should be a responsibility of the browser, not the destination

Assisted-by: Claude Opus 4.7 - all
@david-allison david-allison changed the title arch: introduce 'anki-common' and 'Destination' arch: introduce 'Destination' pattern May 13, 2026
import net.ankiweb.rsdroid.RustCleanup
import net.ankiweb.rsdroid.exceptions.BackendNetworkException
import timber.log.Timber
import com.ichi2.anki.destinations.Destination as NavigateDestination
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of this, where we have to explicity handle the import, maybe rename the old one and mark that as deprecated?
i.e.

interface Destination {
    fun toIntent(context: Context): Intent
}

to

@Deprecated(
    "Use com.ichi2.anki.destinations.Destination + navigate(). See #20558.",
    ReplaceWith("com.ichi2.anki.destinations.Destination"),
)
interface IntentDestination {
    fun toIntent(context: Context): Intent
}

or anything better?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather handle this in a follow up. I've kept the original issue open, and "doing it right" in the next PR is better than a temporary rename

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a TODO, but not the rename, would rather churn the code once

Comment thread anki-common/src/main/java/com/ichi2/anki/destinations/Navigator.kt Outdated
@criticalAY criticalAY added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels May 14, 2026
Decouples the navigation contract from the app module. When we move to a
multi-module approach, we will not want feature modules depending
on other feature modules.

Moving navigation (and interface definitions for navigating to screens)
to a lower module means we can break cross-module dependencies/circular
dependencies.

* Destination will be subclassed, and these subclasses
define the requirements for types of navigation
  * Card Browser may take a CardId to scroll to
* Exposed publicly by com.ichi2.anki.destinations.navigate(Destination)
  extensions

Issue 20558

Assisted-by: Claude Opus 4.7
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels May 14, 2026
* AnkiDroidNavigator handles the top-level 'when(Destination)'
* Each package contains `FeatureDestination.toIntent(Context)`
  * When a feature module split occurs, the nav implementation lives
  with the feature

Issue 20558 - initial implementation of navigation pattern

Assisted-by: Claude Opus 4.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants