refactor: migrate core modules to Kotlin Multiplatform and consolidat…#4735
refactor: migrate core modules to Kotlin Multiplatform and consolidat…#4735jamesarich merged 2 commits intomainfrom
Conversation
…e DI - Convert `core:data`, `core:network`, `core:domain`, `core:di`, and `core:prefs` into Kotlin Multiplatform (KMP) modules. - Delete the `:core:analytics` module and move flavor-specific implementations (Google/Firebase and F-Droid) directly into the `:app` module. - Relocate the `PlatformAnalytics` interface and `DataPair` model to `core:repository`. - Move platform-specific dependency injection (Hilt) modules for network and analytics from library modules to the `:app` module. - Update `core:domain` use cases to use `okio` (`BufferedSink`, `BufferedSource`) instead of `java.io` to support non-Android targets. - Refactor `NodeManagerImpl` to use `atomicfu` and `kotlinx-collections-immutable` for concurrency in shared code. - Update `PacketHandlerImpl` and `MeshMessageProcessorImpl` to use `Mutex` and KMP-compatible collection types. - Migrate the application package from `com.geeksville.mesh` to `org.meshtastic.app`. - Introduce `expect`/`actual` patterns for `Location` and `LocationRepository` to support platform-independent location handling. - Update `ExportDataUseCase` to use `kotlinx-datetime` for timestamp formatting. Signed-off-by: James Rich <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR migrates core modules (core:data, core:network, core:domain, core:di, core:prefs) to Kotlin Multiplatform (KMP), deletes the core:analytics module (moving its interfaces to core:repository and implementations to the app module), migrates the application package from com.geeksville.mesh to org.meshtastic.app, and switches domain use cases from java.io to okio for cross-platform compatibility.
Changes:
- Converted core modules to KMP with
expect/actualpatterns (e.g.,Location), migrated tests from JUnit tokotlin.test, and replacedjava.iowithokioin domain use cases. - Deleted the
core:analyticsmodule, relocatedPlatformAnalyticsandDataPairtocore:repository, and moved flavor-specific analytics/network DI modules into theappmodule. - Migrated the application package from
com.geeksville.meshtoorg.meshtastic.appacross all app-level files including AndroidManifest.xml, services, receivers, navigation, and widgets.
Reviewed changes
Copilot reviewed 205 out of 265 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Removes :core:analytics module |
| gradle/libs.versions.toml | Adds kotlinx-atomicfu dependency |
| core/analytics/* | Deleted module (build.gradle.kts, README.md) |
| core/repository/src/commonMain/.../PlatformAnalytics.kt | Relocated from analytics, removed AddNavigationTrackingEffect |
| core/repository/src/commonMain/.../DataPair.kt | Relocated from analytics module |
| core/repository/src/commonMain/.../LocationRepository.kt | New expect/actual pattern for KMP location |
| core/repository/src/androidMain/.../Location.kt | Android actual typealias for Location |
| core/di/build.gradle.kts | Converted to KMP library |
| core/domain/build.gradle.kts | Converted to KMP library with okio/kotlinx-datetime |
| core/domain/src/commonMain/.../*.kt | New/migrated use cases using okio types |
| core/domain/src/commonTest/.../*.kt | Tests migrated from JUnit to kotlin.test |
| core/network/build.gradle.kts | Converted to KMP with marketplace attribute hack |
| core/network/src/main/AndroidManifest.xml | Deleted (INTERNET permission removed) |
| core/network/.../MQTTRepository.kt | New interface extracted from impl |
| core/network/.../MQTTRepositoryImpl.kt | Implements new MQTTRepository interface |
| core/prefs/build.gradle.kts | Converted to KMP library |
| core/prefs/src/commonMain/.../*.kt | New prefs implementations and DI qualifiers |
| core/data/src/commonMain/.../*.kt | Updated imports, new data sources & interfaces |
| feature/map/.../*.kt | Relocated CustomTileProvider, GoogleMapsPrefs to feature:map |
| feature/settings/.../RadioConfigViewModel.kt | Updated to use okio wrappers |
| feature/settings/.../SettingsViewModel.kt | Updated to use okio BufferedSink |
| app/build.gradle.kts | Updated namespace, added dependencies, removed analytics |
| app/src/main/AndroidManifest.xml | Updated all class references to new package |
| app/src/main/kotlin/org/meshtastic/app/**/*.kt | Package migration from com.geeksville.mesh |
| app/src/{google,fdroid}/.../*.kt | Moved analytics/network DI modules to app |
Comments suppressed due to low confidence (3)
core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ExportSecurityConfigUseCase.kt:48
- The
ExportSecurityConfigUseCasepreviously formatted the JSON output withJSON_INDENT_SPACES = 4(pretty printing). The new implementation usesbuildJsonObject { ... }.toString()which produces compact/minified JSON by default. This is a behavioral change that may affect readability of exported security config files for end users. If the pretty-printed format was intentional for user-facing exports, consider usingJson { prettyPrint = true }.encodeToString(JsonElement.serializer(), jsonObject)instead.
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PlatformAnalytics.kt:34 - The
AddNavigationTrackingEffectmethod was removed from thePlatformAnalyticsinterface and all its implementations, and the call was removed fromMainScreen. However, for the Google flavor, this means Firebase/Datadog screen tracking will no longer work. If navigation tracking was intentional for the Google flavor, this functionality should be re-implemented elsewhere (e.g., directly in theMainActivityor in a separate navigation tracking component).
feature/map/src/google/kotlin/org/meshtastic/feature/map/prefs/di/GoogleMapsModule.kt:43 - The
GoogleMapsDataStoreannotation was changed frominternalto public. While this may have been necessary to allow access from thefeature:mapmodule after relocation, it broadens the API surface unnecessarily. Consider whether a narrower visibility is possible, or document why this change was needed.
You can also share your feedback on Copilot code review. Take the survey.
| configurations.all { | ||
| if (name.contains("android", ignoreCase = true)) { | ||
| attributes.attribute(marketplaceAttr, "fdroid") | ||
| } | ||
| } |
There was a problem hiding this comment.
The core:network build.gradle.kts hardcodes the marketplace attribute to "fdroid" for all Android configurations. This means the Google flavor's dependencies (like DataDog OkHttp integration, which was previously added via googleImplementation) won't be picked up correctly for the network module. Since the DataDog OkHttp dependency has been moved to the app module, this may be intentional, but it could also cause issues if any network module code conditionally depends on flavor-specific behavior.
| val intent = | ||
| android.content.Intent().apply { | ||
| setClassName("com.geeksville.mesh", "com.geeksville.mesh.service.MeshService") | ||
| setClassName("com.geeksville.mesh", "org.meshtastic.app.service.MeshService") |
There was a problem hiding this comment.
The setClassName call uses the old application ID "com.geeksville.mesh" as the package name (first parameter), but the service class name has been updated to "org.meshtastic.app.service.MeshService". If the application ID has also been migrated to org.meshtastic.app (as suggested by app/build.gradle.kts setting namespace = "org.meshtastic.app"), then the first parameter should also be updated. However, the applicationId is set via config.properties and may still be com.geeksville.mesh for backward compatibility. Please verify that the applicationId (not namespace) still matches "com.geeksville.mesh" — otherwise this setClassName call will fail to find the service.
| } else { | ||
| Log.w(TAG, "No service found for action com.geeksville.mesh.Service. Falling back to default.") | ||
| intent.setClassName("com.geeksville.mesh", "com.geeksville.mesh.service.MeshService") | ||
| intent.setClassName("com.geeksville.mesh", "org.meshtastic.app.service.MeshService") |
There was a problem hiding this comment.
Same concern as AndroidRadioControllerImpl.kt: the first parameter "com.geeksville.mesh" is the application ID used to locate the service component. The service class has been renamed to "org.meshtastic.app.service.MeshService", but the package name must match the actual applicationId. If the applicationId is still com.geeksville.mesh, this is fine. If it has changed, this will break.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4735 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 3 3
Lines 231 231
Branches 34 34
=====================================
Misses 231 231 ☔ View full report in Codecov by Sentry. |
Update `AndroidRadioInterfaceService` to observe device address changes via a Flow, ensuring the radio interface restarts automatically when the address is updated. This change also removes emulator-specific logic that previously defaulted to a mock interface. - AndroidRadioInterfaceService.kt: Added flow collector for `radioPrefs.devAddr` in `initListeners` - Removed `shouldDefaultToMockInterface` and `BuildUtils` dependency - Simplified `getDeviceAddress()` to return the current value from `_currentDeviceAddressFlow`
…e DI
core:data,core:network,core:domain,core:di, andcore:prefsinto Kotlin Multiplatform (KMP) modules.:core:analyticsmodule and move flavor-specific implementations (Google/Firebase and F-Droid) directly into the:appmodule.PlatformAnalyticsinterface andDataPairmodel tocore:repository.:appmodule.core:domainuse cases to useokio(BufferedSink,BufferedSource) instead ofjava.ioto support non-Android targets.NodeManagerImplto useatomicfuandkotlinx-collections-immutablefor concurrency in shared code.PacketHandlerImplandMeshMessageProcessorImplto useMutexand KMP-compatible collection types.com.geeksville.meshtoorg.meshtastic.app.expect/actualpatterns forLocationandLocationRepositoryto support platform-independent location handling.ExportDataUseCaseto usekotlinx-datetimefor timestamp formatting.