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 7 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...

@claraf3 claraf3 force-pushed the cleanup-new-code branch 3 times, most recently from 9b9cfe5 to b85823d Compare July 16, 2025 03:30
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 think this is redundant can be deleted. IIRC when I ran the dependency graph generation script I excluded the benchmarks module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the command I use:

./generateModuleGraphs.sh --exclude-module :benchmarks --exclude-module :lint --exclude-module :ui-test-hilt-manifest

Copy link
Contributor

Choose a reason for hiding this comment

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

@dturner these graphs are becoming harder to read with this new api/impl split of module 🙈

Remember the discussions about dependency graphs in #1554 ?
I feel like this could be a great opportunity to showcase the api/impl splits.

Copy link
Collaborator

@claraf3 claraf3 Aug 12, 2025

Choose a reason for hiding this comment

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

I did run that particular command but it still generated this. Tried rerunning again after rebasing to main and im getting these errors

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'nowinandroid'.
> Could not create task ':generateModulesGraphvizText'.
   > 'org.gradle.api.Project org.gradle.api.artifacts.ProjectDependency.getDependencyProject()'

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to generate a Build Scan (Powered by Develocity).
> Get more help at https://help.gradle.org.

BUILD FAILED in 983ms
4 actionable tasks: 4 up-to-date
Configuration cache entry stored.
Error: dot: can't open /tmp/dep_graph_core_common.gv: No such file or directory
Done in 1 ms!
0 KiB - NaN% = 0 KiB
rm: /tmp/dep_graph_core_common.gv: No such file or directory
Parallel Configuration Cache is an incubating feature.
Calculating task graph as no cached configuration is available for tasks: generateModulesGraphvizText
Type-safe project accessors is an incubating feature.

The newly generated svg graphs are not showing properly as a result. Any idea what may be causing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can exclude this one

apply(plugin = "org.jetbrains.kotlin.plugin.serialization")

dependencies {
"api"(project(":core:navigation"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
"api"(project(":core:navigation"))
"implementation"(project(":core:navigation"))

Copy link
Collaborator

Choose a reason for hiding this comment

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

all impls need it as well, so api can just expose it to them


dependencies {
"api"(project(":core:navigation"))
"implementation"(project(":core:data"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the reason for this dependency? I thought the api modules only needed to depend on :core:navigation for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed this dependency after moving TopicScreen/TopicViewModel to impl.

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 am not sure why this needs to be in the api module - it shouldn't be used by any other module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moved to impl. It was in api due to the old ListDetailPane implementation which has since been replaced with the ListDetailScene.

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 am not sure why this needs to be in the api module - it shouldn't be used by any other module.

Copy link
Collaborator

@claraf3 claraf3 Aug 12, 2025

Choose a reason for hiding this comment

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

Moved to impl. It was in api due to the old ListDetailPane implementation which has since been replaced with the ListDetailScene. Good catch.

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 understand why this was moved into the api module - so that interests can depend on it, however, this raises an interesting question: should the list own its placeholder?

In this case, I would say yes, because the Interests module was originally designed to contain more than one type of list item (the 2 being "authors" which was deprecated, and the still current "topics".

Let's move TopicDetailPlaceholder into the Interests module and rename it to InterestsListPlaceholder.

This should simplify the dependency graphs, and keeps the :feature:api modules consistently simple, containing only the navigation routes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, Interests could be merged into the Topics module. I've been meaning to do this since we removed Authors, however, I don't want to add more complexity to an already complex PR. Up to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moved to Interests impl and renamed to InterestsDetailPlaceholder. Also moved the resources (drawable & string) referenced by InterestsDetailPlaceholder to Interests api res.

@claraf3 claraf3 force-pushed the cleanup-new-code branch 2 times, most recently from 73fc68b to f87e507 Compare August 12, 2025 21:32
@claraf3
Copy link
Collaborator

claraf3 commented Aug 12, 2025

Is there a lint task that auto formats all the files? ./gradlew lint just checks for errors but doesn't auto format.

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.

3 participants