Conversation
c9a8a77 to
53b4b10
Compare
Pururun
left a comment
There was a problem hiding this comment.
@Pururun partially reviewed 204 files and all commit messages, and made 12 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Rawa).
android/gradle/libs.versions.toml line 74 at r1 (raw file):
turbine = "1.2.1" annotation-jvm = "1.9.1" junit-version = "4.13.2"
Do we need to add junit4?
android/gradle/libs.versions.toml line 170 at r1 (raw file):
protobuf-kotlin-lite = { module = "com.google.protobuf:protobuf-kotlin-lite", version.ref = "protobuf" } turbine = { module = "app.cash.turbine:turbine", version.ref = "turbine" } junit = { group = "junit", name = "junit", version.ref = "junit-version" }
See above
android/gradle/libs.versions.toml line 171 at r1 (raw file):
turbine = { module = "app.cash.turbine:turbine", version.ref = "turbine" } junit = { group = "junit", name = "junit", version.ref = "junit-version" } material = { group = "com.google.android.material", name = "material", version.ref = "material" }
Do we need to use ordinary material since we have material3?
android/lib/navigation/src/main/java/net/mullvad/mullvadvpn/core/animation/SlideInFromRightTransition.kt line 26 at r2 (raw file):
AnimatedContentTransitionScope<NavBackStackEntry>.() -> ExitTransition = { ExitTransition.Companion.None
Remove companion
android/gradle/build-logic/src/main/kotlin/AndroidLibraryFeatureImplPlugin.kt line 27 at r2 (raw file):
// Fixes packaging error caused by: // androidx.compose.ui:ui-test-junit4 "META-INF/AL2.0",
Probably don't need all of them
android/lib/navigation/src/test/java/net/mullvad/mullvadvpn/core/ExampleUnitTest.kt line 11 at r2 (raw file):
* See [testing documentation](http://d.android.com/tools/testing). */ class ExampleUnitTest {
Remove file
android/lib/feature/daita/impl/build.gradle.kts line 16 at r2 (raw file):
} junitPlatform {
This could maybe be moved into the compose plugin. (Memory note)
android/lib/navigation/.gitignore line 1 at r1 (raw file):
/build
Newline, maybe remove
android/lib/screen-test/.gitignore line 1 at r2 (raw file):
/build
Remove?
android/lib/navigation/src/androidTest/java/net/mullvad/mullvadvpn/core/ExampleInstrumentedTest.kt line 15 at r2 (raw file):
*/ @RunWith(AndroidJUnit4::class) class ExampleInstrumentedTest {
Remove file
android/lib/feature/daita/impl/src/main/java/net/mullvad/mullvadvpn/feature/daita/impl/DaitaScreen.kt line 3 at r2 (raw file):
package net.mullvad.mullvadvpn.feature.daita.impl // import
Remove
android/lib/feature/daita/impl/src/androidTest/java/net/mullvad/mullvadvpn/feature/daita/impl/DaitaScreenTest.kt line 27 at r2 (raw file):
private fun ComposeContext.initScreen( state: Lc<Boolean, DaitaUiState> = DaitaUiState(daitaEnabled = false, directOnly = false).toLc(),
Loading should probably be loading state
|
Tests in the feature module are not run on CI, we should look into a scalable way to do this. |
Pururun
left a comment
There was a problem hiding this comment.
@Pururun made 10 comments and resolved 10 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa).
android/gradle/libs.versions.toml line 74 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Do we need to add junit4?
Fixed
android/gradle/libs.versions.toml line 170 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
See above
Fixed
android/gradle/libs.versions.toml line 171 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Do we need to use ordinary material since we have material3?
Fixed
android/gradle/build-logic/src/main/kotlin/AndroidLibraryFeatureImplPlugin.kt line 27 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Probably don't need all of them
Fixed
android/lib/feature/daita/impl/src/androidTest/java/net/mullvad/mullvadvpn/feature/daita/impl/DaitaScreenTest.kt line 27 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Loading should probably be loading state
Fixed
android/lib/navigation/.gitignore line 1 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Newline, maybe remove
Fixed
android/lib/navigation/src/androidTest/java/net/mullvad/mullvadvpn/core/ExampleInstrumentedTest.kt line 15 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Remove file
Fixed
android/lib/navigation/src/test/java/net/mullvad/mullvadvpn/core/ExampleUnitTest.kt line 11 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Remove file
Fixed
android/lib/feature/daita/impl/src/main/java/net/mullvad/mullvadvpn/feature/daita/impl/DaitaScreen.kt line 3 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Remove
Fixed
android/lib/navigation/src/main/java/net/mullvad/mullvadvpn/core/animation/SlideInFromRightTransition.kt line 26 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Remove companion
Fixed
Pururun
left a comment
There was a problem hiding this comment.
@Pururun partially reviewed 9 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa).
android/lib/screen-test/.gitignore line 1 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Remove?
Done
1f648f2 to
b79db4e
Compare
Pururun
left a comment
There was a problem hiding this comment.
@Pururun partially reviewed 56 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
Rawa
left a comment
There was a problem hiding this comment.
@Rawa reviewed 189 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions.
android/gradle/libs.versions.toml line 194 at r6 (raw file):
mullvad-android-library-feature-impl = { id = "mullvad.android-library-feature-impl" } mullvad-android-library-compose = { id = "mullvad.android-library-compose" } mullvad-android-library-unit-test = { id = "mullvad.android-library-unit-test" }
Is this still dependent on Android Library? If so fix app otherwise adjust name
android/gradle/build-logic/src/main/kotlin/MullvadUnitTestPlugin.kt line 8 at r6 (raw file):
import utilities.libs class MullvadUnitTestPlugin : Plugin<Project> {
Remove mullvad from name
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new "daita" feature module and performs significant refactoring of the Android build configuration and codebase structure.
Changes:
- Introduces a new feature module architecture with
lib/feature/daita/impl - Refactors build configuration to use convention plugins instead of inline gradle scripts
- Migrates utility classes from
net.mullvad.mullvadvpn.utiltonet.mullvad.mullvadvpn.lib.common - Creates new shared modules for navigation and screen testing
- Updates navigation graph from
RootGraphtoMainGraphwith external module support
Reviewed changes
Copilot reviewed 233 out of 233 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| android/gradle/build-logic/* | New convention plugins for library, compose, testing configurations |
| android/lib/feature/daita/impl/* | New DAITA feature implementation with ViewModel, UI state, dialogs, and tests |
| android/lib/navigation/* | New navigation utilities module |
| android/lib/screen-test/* | New shared test utilities module |
| android/lib/common/* | Package refactoring for Lc/Lce utilities |
| android/app/* | Navigation graph updates and import fixes |
| Multiple build.gradle.kts | Migration to convention plugins |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0f0f1ea to
46ec3e2
Compare
Add new DAITA feature module along with convention plugins for: - New features - Unit tests - Instrumented tests These makes it easier to get make a new feature module along with simplifying the use of tests in submodules.
8de0327 to
a68719d
Compare
kl
left a comment
There was a problem hiding this comment.
@kl partially reviewed 237 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions.
This change is