这是indexloc提供的服务,不要输入任何密码
Skip to content

Migrate codebase to Navigation 3 #1902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dturner
Copy link
Collaborator

@dturner dturner commented Jul 15, 2025

[WIP] This PR migrates the codebase to using Jetpack Navigation 3.

More details to follow...

Source code is still left in api module. impl module is empty at this point.
implementation(projects.feature.topic)
implementation(projects.feature.search)
implementation(projects.feature.settings)
implementation(projects.feature.interests.api)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a shame we have to have both api and impl dependencies for every feature module. I understand why this is the case (api contains the public facing routes and impl contains the entryproviders), however, it would be good if we could avoid this somehow.

object NiaAppNavigation {
@Provides
@Singleton
fun provideBackStack(backStackFactory: BackStackFactory) : SnapshotStateList<Any> =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about introducing a stronger type, say NiaRoute or NiaNavKey, for the back stack items? This would:

  • Make navigation keys more obvious
  • Protect against accidentally adding the wrong type to the back stack

Copy link

Choose a reason for hiding this comment

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

Added marker interface NiaBackStackKey

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like the explicit association with BackStack because the route is more than that. Would prefer NiaRoute or NiaNavKey (I think this is ok, even though it doesn't extend NavKey)

Copy link

Choose a reason for hiding this comment

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

Nav3 doesn't really use the term route - its more of a nav2 convention so I don't think NiaRoute is a good idea.

the route is more than that
I think that used to be the case, but contentKey has since then replaced all usages of key in NavEntryDecorators and scene keys. So at this point key is really just a backStack key - its only used to map backstack items back to a NavEntry.

wdyt?

Copy link

Choose a reason for hiding this comment

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

Which on a side note, with introduction of contentKey, i'm even not so sure about the official NavKey interface anymore. Is it super clear that it should be used just for key and not other keys? Maybe? Maybe not?

): EntryProviderBuilder<Any>.() -> @JvmSuppressWildcards Unit = {
entry<SearchRouteNav3> { key ->
SearchRoute(
onBackClick = backStack::removeLast,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required because SearchScreen displays a back arrow (do we need it?). Is it always the case that we just pop the last back stack item when it's clicked? Need to think about this more.

Copy link

Choose a reason for hiding this comment

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

I think it's needed. The search screen covers the entire screen so there's really no convenient way to dismiss/go back from search screen without it. Users could use system back, but then this would apply to all back arrows in general - I believe the added convenience from back arrow has its value.

Or in other words, is there any harm to having it? So far the back behavior of removeLost() seems natural to me as I play around with search. Unless you wanted to make search into a list/detail screen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, ok we definitely need it then. My issue is that we only know that removeLast is the right thing to do here because we know that SearchRoute is only ever displayed in a single pane, which may not always be the case.

If we didn't know the number of panes being displayed, what should the back behavior be? Pop everything up to and including SearchRoute? If so, we should do that (maybe add a backStack.popUpTo(route, inclusive = true) method). Or maybe we should try to call NavDisplay.onBack.

@claraf3 claraf3 force-pushed the cleanup-new-code branch 2 times, most recently from bd250e5 to 9b9cfe5 Compare July 16, 2025 02:57
@claraf3 claraf3 force-pushed the cleanup-new-code branch from 9b9cfe5 to b85823d Compare July 16, 2025 03:30
onBackClick = backStack::removeLast,
onInterestsClick = key.onInterestsClick,
onTopicClick = backStack::navigateToInterests,
)
Copy link

Choose a reason for hiding this comment

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

There's a bug here on main branch - filed #1906

@claraf3 claraf3 force-pushed the cleanup-new-code branch from b85823d to ce87666 Compare July 16, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants