KMP migration - #5067
Open
MilosKozak wants to merge 136 commits into
Open
Conversation
Event.toString() used commons-lang3 ReflectionToStringBuilder, which was the only thing keeping the base class on Android. The 23 events that carry constructor parameters become data classes, so they print their own values. The 41 that carry none keep the base toString, which is now just the class name - the same thing reflection produced for a class with no fields. commons-lang3 is gone from :core:interfaces. Nothing depended on event identity, so the change from identity to content equality is safe: no distinctUntilChanged on a bus stream, no event in a Set or Map, no event compared with ==. The dedup key documented on EventShowSnackbar is not actually implemented in the host, so it does not rely on equality either. None of the 23 was open or subclassed, so all could take data. EventMobileToWearWatchface holds a ByteArray, so its generated toString prints the reference rather than the bytes reflection used to dump. For a watchface payload that is an improvement. RxBus is now one step from commonMain: Class<T> in toFlow is all that is left.
Class<T> in toFlow was the last thing keeping RxBus on Android. KClass is multiplatform and isInstance works the same, so the signature becomes toFlow(eventType: KClass<T>) and 208 call sites drop one token: ::class.java becomes ::class. Three places pass the type in a variable rather than a literal and change with it: RxHelper in androidTest, SWEventListener, and the two test fakes of RxBus. RxHelper's maps and its waitFor and resetState take KClass now too, so the file is consistent, and its callers plus SWDefinition follow. With that and Event moving in the previous commit, the fixpoint jumps from 156 files in commonMain to 204, leaving 56. Six rounds again: 31 files fail on their own imports, then 11, 10, 3 and 1. Sensitivity, IobCobCalculator, AutosensDataStore and the rest of that chain came across. What is left is genuinely platform bound or waits on ResourceHelper: SP, Storage, FileListProvider, the Dagger qualifiers, PasswordCheck, BlePreCheck, RfcommTransport, Sms, AapsSchedulers, and the plugin and pump interfaces that name ResourceHelper in a signature. Reified would read better than KClass at the call site. It can come later on its own - it is an inline extension, so it changes the shape of every call rather than one token.
Same change as RxBus, and it finishes something that was already half done here: observeAnyChange has always returned Flow<Set<KClass<*>>>, and a reified observeChanges() extension already existed, it just unwrapped to Class internally. 89 call sites drop .java. The when in PersistenceLayerImpl matches on KClass now, the test stubs match any<KClass<*>> instead of any<Class<*>>, and one list of types in OverviewDataCacheImpl needed converting by hand because it is built with listOf rather than passed inline. KClass.simpleName is nullable where Class.simpleName is not, which the default argument of awaitDbChange in androidTest relied on. This moves PersistenceLayer to commonMain and nothing else: everything that references it is held up by something else, mostly ResourceHelper. 205 files in commonMain now, 55 left.
remove, putBoolean, putDouble, putLong, putInt and putString taking a @stringres id existed on SP.Editor and nowhere else did anything call them, except SPImplTest. The test drives them through a `someResource` variable, so a search for edit blocks containing R.string found nothing and reported them as dead. They are dead in production; the test assertions go with them. SP itself stays in androidMain with its own resource id overloads, which wear uses. That is where it belongs: Preferences in :core:keys is the platform neutral API and is already in commonMain without referencing SP, and SP is the Android SharedPreferences layer that the phone's PreferencesImpl is built on. SP is not a blocker either - nothing in :core:interfaces references it.
Same change as DateUtil, for the files that named ResourceHelper only to pass it along. Callers are untouched, because a ResourceHelper is a TextResolver. InsulinType was already halfway there: its labels are TextRefs, so rh.gs(label) was a TextResolver call already and only the parameter type had to change. It moves to commonMain and takes InsulinManager with it. Profile names it in five signatures and never calls it. The work is in ProfileSealed, which does: 17 rh.gs(R.string.x) become rh.gs(TextRef.AndroidRes(R.string.x)), including two multi line calls and one that picks the id with an if. PumpSync has three of the same in its own default methods. Profile and PumpSync still cannot move: Profile names Pump, and PumpSync names Profile. Pump itself has no platform import and no ResourceHelper, so what holds that group is worth a separate look. ProfileSealedTest stubbed rh.gs with a raw id and now stubs the TextRef, which is what turned "mmol/L/U" into null. 207 files in commonMain, 53 left.
ImageVector was never blocking PluginDescription. Compose Multiplatform is already a commonMain dependency of this module, so it resolves there, and moving the file proves it. I had tested that earlier and then repeated the wrong reason twice. What actually held this group: - Pump waited on PumpProfile, which waited on Profile, which is the TextResolver change from the previous commit. Nothing platform bound anywhere in the chain. - PumpSync named R.string directly, so wrapping the ids in TextRef.AndroidRes did not help - AndroidRes still needs the R class. Its three strings exist in the generated common table, so they become InterfacesStrings.temp_basal_* , which is TextRef.Named and needs no R. This is the pattern InsulinType already used. PluginDescription still cannot move, but not for the reason I gave: it takes a (PluginBase) -> Any provider, and PluginBase stays behind for two small JVM things - an org.jetbrains @testonly annotation, and pluginId reading javaClass.simpleName. pluginId is deliberately left alone. Its KDoc says it matches the legacy RunningConfiguration encoding so the active-plugin sync keeps dual-writing the same value, and this::class.simpleName is nullable where javaClass.simpleName is not. Changing a persisted identity needs a decision, not a sweep. 211 files in commonMain, 49 left.
javaClass.simpleName becomes this::class.simpleName, which is the same string: a plugin is always a named class, as you said. It is not defaulted - simpleName is only null for an anonymous class, and syncing an empty plugin id would be worse than failing, since the value is persisted and mirrored to the client. The org.jetbrains @testonly annotation goes too. It is JVM only and carries no runtime behaviour; the KDoc above the function already says it is for tests. That removes both JVM references from PluginBase. It still does not move, and I tried: making its rh a TextResolver breaks 30 plugin files that call rh.gs(R.string.x) through the inherited property. That is churn in plugin and pump code which is not going multiplatform, so it waits for the ResourceHelper migration rather than being forced now. PluginDescription is behind PluginBase either way - it takes a (PluginBase) -> Any provider.
pluginName, shortName and description were @stringres ids with -1 meaning unset. They are TextRef? now, so a plugin names itself with something that carries no Android resource id, and "unset" is null rather than a sentinel. 64 files pass TextRef.AndroidRes(R.string.x) at the builder. That is fine from a plain Android module: TextRef lives in :core:keys commonMain, and a KMP module publishes an ordinary Android artifact, so pump drivers and plugins use it with no KMP plugin of their own - :pump:danar already had 43 uses before this. PreferenceSubScreenDef gains a TextRef primary constructor and keeps the resource id one as a secondary, so its 285 call sites are untouched while the three BG source plugins can pass a plugin's own name straight through. It already derived `title: TextRef` from the id internally, so this only exposes what it was doing. Readers lose their sentinel checks: SearchIndexBuilder compares against null, SearchableItem and QuickLaunchResolver pass the ref along instead of rewrapping it, and ConfigBuilderImpl uses PluginBase.name where it wanted the localized form. PluginBase keeps rh: ResourceHelper. Narrowing it to TextResolver is what would let PluginBase and PluginDescription reach commonMain, but it breaks plugins that hand the inherited rh to other ResourceHelper-typed APIs - VirtualPumpPlugin does this five times - so that belongs with the ResourceHelper migration, not here.
PluginBase held rh: ResourceHelper, but it only ever calls gs(TextRef), for name, nameShort and description. ResourceHelper adds the Android resource id overloads, which mean something only where Android resources exist, so that one field kept PluginBase, PluginDescription and ActivePlugin in androidMain. rh is now TextResolver, the platform neutral half. Plugins that use the inherited rh with Android resource ids narrow it back with "override val rh: ResourceHelper" - 31 one line changes. The alternative was to wrap every call as rh.gs(TextRef.AndroidRes(R.string.x)), which measured 366 call sites and would also mix the TextRef migration of individual plugins into a change that is about platform neutrality. That migration stays a separate job. Plugins behave exactly as before. Other platform specific parts of these types: - ActivePlugin.getSpecificPluginsListByInterface and CommandQueue.isCustomCommand* took Class<*>. They take KClass now, so call sites pass X::class. - ActivePlugin.collectMissingPermissions and collectAllPermissions need a Context and the Android permission model. They moved to a new PluginPermissions interface in androidMain; PluginStore implements both. - UiInteraction.mainActivity and errorHelperActivity are KClass, so the nine Intent call sites add .java. - Callback implemented java.lang.Runnable. Nothing ever handed a Callback to something that wanted a Runnable, so it declares its own run(). This also fixed two "cannot infer type parameter" errors in Command, where ?.run() resolved to Kotlin's scope function instead. Moved to commonMain: ActivePlugin, PluginBase, PluginBaseWithPreferences, PluginDescription, APS, Sensitivity, IobCobCalculator, PumpWithConcentration, Callback, Command, CommandQueue, EffectiveProfile, UiInteraction. :core:interfaces goes from 211/49 to 224/37 commonMain/androidMain. Dead code found on the way: - CommandQueueImplementation.removeAllCustomCommands tested "command is CustomCommand" on a Command from the queue, so it never matched. It was unreachable anyway: the line above returns early whenever a command of the same type is already queued. - OmnipodDashPumpPlugin built a bolus progress string and threw it away. Omnipod Eros keeps rh.gq for its pod alert plural. Plurals have no TextRef form yet, so that plugin still needs the Android ResourceHelper.
Same four plugin recipe as :core:interfaces - kotlin("multiplatform") plus the
AGP multiplatform library, the Compose compiler and Compose Multiplatform.
src/main becomes src/androidMain and src/test becomes src/androidHostTest.
166 of 438 files reach commonMain, 0 before. 131 of them are icon definitions,
moved untouched: CMP reuses the androidx package names, so not one import
changed. The rest came from compiling for iosArm64 and moving back whatever
failed, three rounds to a fixpoint.
What stays on Android, and why:
- 174 *Previews.kt files. androidx.compose.ui.tooling.preview.Preview is Android
only and CMP keeps its own in a separate artifact. Splitting each icon from its
preview harness costs nothing, because androidMain sees commonMain.
- ~95 files that name an Android resource id. R appears 75 times in the root
blocker list, ResourceHelper.gs 30 more. UiStrings is already generated for
this module, so the way out is TextRef.Named, the same conversion :core:keys
and PluginDescription went through. Not started here.
- 16 files that use a real Android API - Context, LocalContext, fromHtml,
FragmentActivity, window and requestedOrientation.
Restated by hand, because android-module-dependencies, test-module-dependencies,
compose-test-module-dependencies and jacoco-module-dependencies all apply
com.android.library and a multiplatform module cannot:
- lint MissingTranslation / ExtraTranslation off, or the incomplete locale files
would fail a release build for the first time here.
- withHostTest { isIncludeAndroidResources = true; isReturnDefaultValues = true }.
This one is load bearing: without it 93 of the 115 Robolectric Compose tests
fail, because createComposeRule() has no merged resource table and no manifest
holding the activity it launches.
- The JaCoCo isIncludeNoLocationClasses flag. Robolectric rewrites bytecode in its
own classloader, so without it the agent records nothing for the Compose screens
the tests drive. Coverage data for this module goes from 191 KB to 655 KB with
it back. The jacoco plugin itself is applied to every project by the root build
file, and jacocoAllDebugReport already knows how to read a multiplatform module,
so the aggregate report keeps working.
androidx-compose-ui-tooling moves from debugImplementation to implementation: the
AGP multiplatform library target has no build types, so there is no debug only
configuration. R8 in the app module drops it from a release build.
Note for anyone verifying locally: testFullDebugUnitTest does not exist for a
multiplatform module. Use testAndroidHostTest, as CircleCI already does.
Also in here, because the KMP move rewrote every path in this module and the two
cannot be separated into buildable commits: display formatting leaves
:core:objects.
TrendArrow.directionToIcon(), CobInfo.generateCOBString()/displayText() and the
TT display helpers now live in :core:ui, next to the strings and icons they use.
A domain module was reaching into the UI module to pick a picture for an enum and
to name a format string. TT.target() stays behind, because it is arithmetic.
This does not remove the :core:objects -> :core:ui dependency. TB.toStringFull and
EB.toStringFull call domain helpers used across plugins/aps, implementation and
pump/*, so moving them would need :core:ui -> :core:objects, which cycles while
ProfileSealed, BolusWizard and RunningModeGuard still name core.ui strings. Those
three have to be cleaned first.
Two halves. They cannot be split into buildable commits, because the source-set
sweep rewrote the path of nearly every file the first half edits.
## HTML
Only <b> and <br> appear anywhere - 36 and 4 across every English string
resource, nothing else - so a 60 line htmlToAnnotatedString() in commonMain
replaces AnnotatedString.fromHtml, which is Android only and was what kept the
dialogs out of commonMain. 11 unit tests cover it, including the cases the
platform parser gets wrong: an unknown tag and a bare < are kept rather than
swallowed, and an unbalanced <b> still terminates.
The rule applied to every other site: markup is built where the text is
generated, not parsed back out before it is shown.
- AutomationRuntime.executionLog is List<AnnotatedString>. It built
"<b>${event.title}:</b>" per entry, so making the screen render plain text
would have shown a literal <b> - the log panel is the one place formatting had
to move to the generator rather than simply disappear.
- MaintenanceDialogs, NSClientComposeContent and InsulinManagementScreen build
their bold headers with buildAnnotatedString and use the AnnotatedString
overload the dialogs already had.
- AppRepository.cleanupDatabase joins with a newline instead of <br>.
- NSDeviceStatusHandler wrote "<b>key:</b> value<br>" and the overview stripped
every tag straight back out before displaying it. It now writes plain lines.
Nothing AAPS uploads contains markup - the Nightscout pump.extended object is
built from versions, dates and rates - so no outbound formatting was lost.
Inbound is different: pump.extended read FROM Nightscout is written by whatever
uploaded it, so the flattening the overview used to do is kept, moved to the
point the foreign data arrives.
Deleted as dead: HtmlHelper (its only callers were two Insight sites, and the
Insight alert strings contain no markup at all), EventTidepoolStatus
.toPreparedHtml, and PumpEnactResult.toHtml with its tests - no production caller
for any of them. Also an unclosed <b> in an Omnipod Dash notification, which was
rendering literally because notification text is drawn by a plain Text.
## commonMain
166 -> 348 of 442 files.
- stringResource(TextRef) is expect/actual. The Android actual is the previous
code unchanged; iOS gets a documented placeholder, because expect needs an
actual on every target and that is what keeps the iOS compile honest. Every
screen funnels through this, so nothing else could move until it did.
- 174 @Preview files use org.jetbrains.compose ... .Preview. It accepts
showBackground, name and widthDp, which is every parameter used here - verified
by compiling for iOS, with a bogus parameter as a negative control. 148 moved;
the other 26 preview components that are still Android bound.
- 49 dead `import app.aaps.core.ui.R` lines. Files already converted to UiStrings
kept the import. Invisible in an Android module, a hard error in commonMain.
- SearchableItem used javaClass.simpleName; pluginId is the documented stable
identity and defaults to exactly that string. No plugin overrides it.
- includeFontPadding is an Android text-layout quirk, now a one line expect.
24 root blockers remain, 44 more files wait only on those. Six need
ResourceHelper narrowed to TextResolver, two carry @stringres, two still call the
platform stringResource; the rest are Activity, Context, Locale, permissions or
java.math.BigDecimal and want expect/actual rather than migration.
426 of 444 files, up from 348. Three of the 18 left are .android.kt actuals, so 15 are real Android code. ## What moved, and why it was stuck ProfileUtil was the cheapest win in the whole exercise: the interface imports nothing but :core:data, so it was simply in the wrong source set. Moving it also cleared the ProfileUtil error out of AapsTheme, PreferenceState and TemporaryTargetDisplay. AapsTheme looked like the hard one and was not. Three lines of 317 were Android: the system-bar icon SideEffect, LocalConfiguration for tablet detection, and one includeFontPadding. Two small expects - SystemBarAppearance and smallestScreenWidthDp - and the theme itself moved, pulling 26 files with it because most of the UI sits under it. BackHandler needed no BOM bump. androidx compose-ui has no backhandler package at any version we resolve (checked 1.10.0, 1.11.2, 1.11.4). It is a separate CMP artifact, org.jetbrains.compose.ui:ui-backhandler, which ships its own Android variant. It is still experimental, hence the OptIn at the two call sites. Six files narrowed from ResourceHelper to TextResolver, four more carried dead Android imports - @stringres on files already returning TextRef, and androidx.compose.ui.res on files already passing one. ## Rounding AdaptiveUnitDoublePreference and PreferenceState used BigDecimal(v).setScale(n, HALF_UP). The multiplatform NumberFormat is half-even, so switching to it would have quietly changed a displayed glucose value. The two modes can only disagree on an exact tie. At one decimal - mmol/L - a tie would have to be (2k+1)/20, and the factor of 5 means no Double ever lands there, so they cannot disagree at all. At zero decimals - mg/dL - a tie is k + 0.5, which IS exactly representable, and there half-even gives 4.5 -> 4. Half-even is banker's rounding: it cancels bias when many values are summed, which is not what a single number on a preference screen does. So half-up stays, and NumberFormat learned the mode instead: a NumberRounding enum, a rounding field defaulting to HALF_EVEN so nothing else changes, wired through all three actuals (DecimalFormat.roundingMode, NSNumberFormatterRoundHalfUp, and the mingw tie branch). SEPARATOR_DOT at the call sites, because toPlainString was always dot-separated and the text is parsed back when edited. ## The regression the emulator caught Naming the shared builders' strings with UiStrings broke them. ResourceHelper lives in :core:interfaces and cannot see UiStringIds - :core:ui depends on it, not the reverse - so a `ui` name read outside a Composable fell back to printing its own name, and the overview showed "format_carbs" instead of "58 g". The KDoc on keysIdOf had called this exactly: "If a non-Compose caller ever needs a `ui` name, this is the place that has to learn about it - at that point a registry is probably better than a third branch." So: TextRefIdRegistry, consulted for any owner keysIdOf does not know directly, with ResourceHelperImpl registering "ui" in its init - that class is downstream of every string-owning module and is built before anything can ask it for text. Verified on emulator: the row reads "58 g", and Treatments and Manage carry no stray names either. Four pump/source tests needed both gs overloads stubbed, not one swapped for the other: the shared builders call gs(TextRef) while the plugins' own code still calls gs(Int). Not verified by hand: the rounding change. It only differs on exact .5 ties, which is the behaviour deliberately preserved, but it is a glucose display.
Loop, ConfigBuilder, ConstraintsChecker, PluginConstraints, ProfileFunction, TemporaryBasalStorage, CalculationSignals and CalculationWorkflow import nothing from Android, Java, RxJava or Dagger. They were in androidMain only because that is where the module started, so moving them is a no-op for every caller. Found by listing androidMain files with no platform import at all - the same check that turned up ProfileUtil. Five other candidates from that list did NOT move and stay put: Maintenance, Prefs and PrefsFile need ExportResult and PrefsMetadataKey, SmsCommunicator needs Sms, and MidnightTime uses synchronized. 233 commonMain / 29 androidMain.
:core:objects goes from 5 files needing :core:ui down to 3. The cycle that made this awkward is gone: the formatting now lives in :core:ui, and :core:ui still has zero references to :core:objects. The blocker was never the formatting itself, it was three helpers it called - TB.getPassedDurationToTimeInMinutes, EB.getPassedDurationToTimeInMinutes and TB.durationInMinutes. They are used across plugins/aps, implementation and the androidTest adapters, so they could not simply travel with the formatting, and while they sat in :core:objects nothing in :core:ui could call them. They read nothing but the record's own timestamp and duration, so they moved DOWN to :core:data instead, next to the models. With them there, TB.toStringFull / toStringShort and EB.toStringFull / toStringMedium moved up to :core:ui with netExtendedRate, and take TextResolver rather than ResourceHelper. durationInMinutes stays Long. T.mins() returns Long and AutotuneIob assigns it straight to a val, so narrowing it to Int would have changed a caller's type. TizenPluginTest needed the gs(TextRef, vararg) overload stubbed as well as gs(TextRef). Mockito does not run default interface methods, so the vararg form answered null, toStringFull produced null and the bundle key vanished - the test failed on a missing key rather than on wrong text, which is a slow way to find out. Still holding :core:objects to :core:ui: ProfileSealed, BolusWizard and RunningModeGuard. Those are domain classes choosing display strings, so they need the strings relocated or the text moved out - not a file move.
…m :core:ui :core:objects no longer depends on :core:ui at all. That edge was backwards - a domain module reaching into the UI module - and it is what blocked :core:objects, and through it :core:graph and the 282-file :ui, from becoming multiplatform. ## The strings ProfileSealed, BolusWizard and RunningModeGuard between them name 33 strings that lived in :core:ui: the value+unit templates (format_carbs, format_insulin_units, confirmation_line, mins), the profile validation messages (value_out_of_hard_limits, basalprofilenotaligned, profile_*) and the wizard's confirmation text. They moved to :core:interfaces, which every one of these modules already depends on and which already owns a strings.xml and an InterfacesStrings generator. No name collided. 795 elements moved - 33 names across the 24 locales that actually had a translation for them. The total element count across both modules is unchanged at 21000, and none of the 33 is left anywhere in :core:ui. The move was line-exact rather than XML-rewritten, so the `comment=` translator notes and every byte of the translated text survive untouched. ## The call sites 138 files. Four different shapes had to be handled, and only the first was obvious: - app.aaps.core.ui.R.string.X, fully qualified - UiStrings.X, the generated TextRef form - a bare R.string.X, from `import app.aaps.core.ui.R` - an aliased CoreUiR/UiR/CoreR.string.X For a file whose R import pointed only at moved strings the import was swapped. Where a file mixes moved and unmoved names - TranslatorImpl uses 4 moved out of 205 - it gained `import app.aaps.core.interfaces.R as InterfacesR` and only the moved references were rewritten. ## What this does not fix The three domain classes still choose display strings; they just choose them from a module below rather than above. Making BolusWizard return structured data instead of built text is still the right change, and is now a refactor that can be done on its own rather than a prerequisite for the module graph.
exportSharedPreferences(FragmentActivity) had no caller anywhere. Its only use was ImportExportPrefsImpl's own private exportSharedPreferencesLegacy, which was in turn the sole route into exportToBoth, exportToLocal, doExportToLocal, exportToCloud and doExportToCloud - all FragmentActivity shaped, all unreachable. 234 lines. exportSharedPreferencesNonInteractive is a different method and stays: Automation calls it through ActionSettingsExport. That FragmentActivity was the only Android type in the ImportExportPrefs interface, so removing it took the whole maintenance package to commonMain: ImportExportPrefs, Prefs, PrefsFile, PrefsMetadataKey and Maintenance. One thing was left in the way. PrefsMetadataKey.formatForDisplay took a Context, and used it for nothing but context.getString. Rather than swap Context for a TextResolver parameter, it now returns a TextRef and the caller resolves it with stringResource - the layer that draws decides the language, and no resolver has to be threaded through the Composable. :core:interfaces 238 commonMain / 24 androidMain. Worth remembering: this is the second file in this migration that looked blocked on an Android type and was really blocked on dead code - the 49 unused `import app.aaps.core.ui.R` lines in :core:ui were the first. Check whether the code runs at all before designing an expect/actual for it.
The whole module, charts included. Nothing left in androidMain but the manifest. ## Vico ships Apple targets This was the open question and the answer is yes. com.patrykandpatrick.vico:compose 3.2.3 publishes iOS artifacts, so the charts are shared rather than reimplemented. Verified by resolving it from a commonMain source set and compiling for iosArm64 - the first probe even failed usefully, with "overload resolution ambiguity" on rememberCartesianChart, which only happens once the symbol has been found. Vico is the only third-party UI library in commonMain. ## What blocked the flip A multiplatform module cannot consume a FLAVOURED Android library: :core:objects carries the five product flavours from android-module-dependencies, so Gradle could not pick a variant for a consumer that asks for none. Only one file needed it - InsulinGraphCompose calls BS.iobCalc - and that function reads nothing but :core:data types, because the work is done by ICfg.iobCalcForTreatment which already lives there. So it moved down to :core:data, the same fix as the duration helpers, and the dependency went away instead of being worked around. :core:objects keeps its own TB.iobCalc and EB.iobCalc; those need Profile and EffectiveProfile. Files using both now import both, which Kotlin resolves by receiver. ## Strings :core:graph owns no strings, it borrowed them from :core:ui and :core:interfaces. Converted to UiStrings / InterfacesStrings with the module's own stringResource(TextRef), previews to the CMP annotation, ResourceHelper to TextResolver. One regex mangled a fully qualified app.aaps.core.interfaces.R.string.shorthour into app.aaps.core.interfaces.UiStrings.shorthour, because the bare-R rule matched inside the qualified name. The compiler caught it; a rule that rewrites `R.string` has to run after the qualified forms, not before.
The blocker for :core:objects was never org.json, it was Dagger. Dagger emits Java, and the AGP multiplatform library target has NO Java compilation step - :core:objects:tasks --all lists no *JavaWithJavac task at all, and build/classes/ holds only kotlin/. So KSP generated CoreModule's hilt_aggregated_deps and CoreModule_SmsManagerFactory correctly, they were never compiled, and the app failed with MissingBinding for SmsManager. That is not fixable with a Gradle setting. The fix is to stop generating. Five classes - CryptoUtil, RunningModeGuard, QuickWizard, QuickWizardEntry and BolusWizard - lose @Inject/@singleton and become plain constructors. A new CoreObjectsModule in :implementation constructs them with @provides, which keeps the graph identical: same scopes, same instances. This is the trade already made for BolusProgressData, and its comment says why - javax.inject cannot be used from code meant to reach commonMain. javax.inject.Provider is JVM only too, so QuickWizard and QuickWizardEntry take `() -> T` instead. Dagger still supplies them from the Android side, as a lambda over Provider.get(). Same pattern as Command.pumpEnactResultProvider. CoreModule itself moved rather than converted: a @module inside a multiplatform module would be generated and silently dropped. Its two @ContributesAndroidInjector entries were dead - nothing member-injects BolusWizard or QuickWizardEntry - so only the SmsManager @provides came across. :core:objects now runs no annotation processor: the ksp plugin and all three Dagger processors are gone from its build file, along with kotlin-parcelize, which was applied to a module containing no @parcelize at all. This does not flip the module. org.json still holds ~10 files and has to be dealt with separately. What it removes is the reason the flip could not work.
:app is the only module that can never become multiplatform - it is the Android application itself. Wiring put there never has to move again, whereas :implementation carries no such guarantee. It also lands among its peers: app/aaps/di/ already holds AppModule, PluginsListModule and ReceiversModule, and :app already depends on :core:objects. Hilt aggregates @Installin(SingletonComponent) modules in :app regardless of which module declares them, so this changes where the source lives and nothing about the graph. Worth recording the counter-argument, because it may matter later: :implementation is probably safe too. It is the ANDROID implementation of :core:interfaces - 42 of its 116 files import android/androidx directly, including 20 of the 51 *Impl classes - so iOS would get its own implementation module beside it rather than sharing this one. But that is a judgement about the future and :app needs no judgement. Verified on a cold-booted emulator (-no-snapshot-load): clean start, no MissingBinding, and the bolus wizard calculates 20 g -> 3.10 U at 10.5 mmol/L, which exercises BolusWizard through the hand-written provider including the correction path. No ANR on a quiet machine - the onStartJob ANRs seen earlier were the build host at load 6.68, not the app.
:core:objects is now a Kotlin Multiplatform module. 17 of 29 files are in commonMain. The DI lift to :app in the previous commit removed the Dagger codegen that blocked the flip, and two more files moved once their org.json boundary was split out: - BlockExtension and InsulinExtension keep only the kotlinx logic and move to commonMain. Their org.json entry points go to new androidMain files BlockJsonAdapters and InsulinJsonAdapters. These were already written as thin adapters, so nothing else changed. - ProfileSealed and RunningModeGuard ask for strings by InterfacesStrings instead of R.string, and RunningModeGuard takes the common TextResolver instead of ResourceHelper. - System.currentTimeMillis becomes Clock.System, and javaClass.simpleName becomes ::class.simpleName. Still Android only: CryptoUtil (javax.crypto), LoggingWorker (WorkManager), the two json adapter files, and 8 files that need org.json. Two Compose deprecations are also cleared: - compose.components.uiToolingPreview is replaced by org.jetbrains.compose.ui:ui-tooling-preview. The new artifact declares the annotation under the androidx package name, so all 177 preview files share one import with the Android only modules. - androidx.compose.ui.backhandler.BackHandler is replaced by NavigationBackHandler. Note the coordinate: androidx.navigationevent has no iOS targets, so this uses the JetBrains republish org.jetbrains.androidx.navigationevent:navigationevent-compose. Two test stubs asked for strings by id while the code moved to TextRef. SmsCommunicatorPluginTest was a real catch: the guard stopped rejecting and a bolus went through where the test expected "Pump suspended". Verified on emulator: profile viewer shows "7.7 g/U" and "8.2 mmol/L/U", the constraint log keeps its "[Safety]" source tag, and 17 captured screens contain no unresolved string keys.
The kotlinx halves of our JSON helpers were not equal to the org.json halves
they sit next to. org.json coerces on read and kotlinx throws, so a stored "36"
(a quoted number, which real Nightscout documents contain) read back as 36
through org.json and as the DEFAULT through kotlinx, with nothing logged.
New JsonLenientRead in :core:utils commonMain copies Android's JSON.toInteger /
toLong / toDouble / toBoolean / toString rules, and both sides now share it.
Fixed:
- ICfg.fromJsonObject read insulinEndTime with longOrNull, which answers null for
1.8E7 and 18000000.5 and fell back to 0. insulinEndTime 0 means DIA 0, so IOB
decays at once and the loop believes there is no insulin on board. It is
reachable from InsulinImpl and from BatchActionMapping (client control sync).
- All twelve kotlinx JsonHelper twins had the same coercion gap. The two string
overloads also had a dead guard: the JsonNull check was overwritten by the next
line, and because JsonNull is a JsonPrimitive holding "null" they returned that
text instead of the default.
- SceneSerializer used map with a throwing getString("id"), so ONE damaged scene
reached the outer catch and returned an empty list, wiping the whole scene
catalogue. Now the bad entry is skipped, like an unknown action type already
was.
- QuickWizard.addOrUpdate passed a stale position to JSONArray.put(index, value),
which pads the list with nulls and makes the readers throw at the next app
start. remove() had no bounds check and still saved, pushing an unchanged list
through the sync channel.
- IobTotal.json and determineBasalJson shared one swallowing try, so a NaN iob
skipped every later put and returned {}, losing time as well.
- QuickWizardEntry left its lateinit storage unassigned on a parse failure, so it
failed later as UninitializedPropertyAccessException far from the cause.
Removed as dead code:
- The IobTotal.copy() extension. IobTotal is a data class whose fields are all
constructor parameters, so the generated member copy() wins at every call site
and nothing imports the extension.
- QuickWizardEntry.usePercentage(), the DEFAULT/CUSTOM constants and the seeded
"usePercentage": "default" template entry. Its only writer was deleted in
ccf0fe3 (2023-10-14), so the DEFAULT branch has been unreachable for over
two years on master as well. IntKey.OverviewBolusPercentage stays - wear still
uses it.
Split so far: GlucoseValueExtension keeps its logic in commonMain as
toJsonObject, and toJson stays in androidMain as a reparse delegate. Going back
through the text is what keeps the bytes identical, because org.json renders a
whole numbered double as an integer and kotlinx does not.
:core:objects is now 25 files in commonMain and 8 in androidMain. org.json no longer blocks anything: of the 8 left, 6 are the deliberate boundary adapters that exist so the ~199 org.json files elsewhere keep compiling, and 2 are real platform code (CryptoUtil, LoggingWorker). Four independent files converted, each keeping its logic in commonMain on kotlinx types and leaving a thin org.json delegate behind: - IobTotalExtension: plus/round/combine are pure and moved as is; json and determineBasalJson became kotlinx builders. The NaN guard is restated for kotlinx, which has the opposite failure mode - org.json refuses a non finite double, kotlinx accepts it and writes the bare token NaN, which is not valid JSON and would turn an uploaded device status into null. - JSONObjectExt: a clean split, its kotlinx twins already existed. Its commonMain store() had the same coercion bug as JsonHelper (strict raw.int / raw.double), so it now reads leniently too. Only tests call it today. - SceneSerializer: its API is String based, so the whole file moved and no adapter was needed. - ProfileSwitchExtension: the units rule is preserved exactly - absent or explicitly null falls back to defaultUnits and only a still missing value rejects the profile. That rejection matters because GlucoseUnit.fromText never throws, it answers MGDL, so a null slipping through would read an mmol/L profile as mg/dL and put every target, ISF and correction out by 18x. QuickWizardEntry, QuickWizard and BolusWizard had to land together because they depend on each other. The entry now holds plain data instead of a live JSONObject, and all 23 accessors kept their names so every read call site is unchanged. Removing the JSONObject removed the aliasing three things relied on: - setGuidsForOldEntries assigned a guid and never saved. It only persisted because the entry WAS the element inside the stored array, so the guid rode along on some later unrelated save. If none happened, a different UUID was generated on every app start and nothing could resolve a legacy entry by guid across restarts. It saves now. - markAsUsed writes back explicitly through addOrUpdate. - QuickWizard parses into a list and skips an unreadable element, where a single damaged entry used to be a ClassCastException at construction, i.e. the app did not start. The editor's 46 storage.put calls became three typed copy() blocks. Clone now explicitly keeps its own guid and resets lastUsed instead of relying on which keys the old code happened to copy. Last blockers were small: kotlin.concurrent.Volatile, kotlin.uuid.Uuid, and in BolusWizard the 22 R.string refs to InterfacesStrings plus ResourceHelper to TextResolver and Calendar to Clock. The bolus calculator is common code now. Verified on emulator: a created preset persists all 23 keys with the right defaults (validTo 86340, useBG 0, useCOB 1, percentage 100), clone produces a second entry with a DIFFERENT guid and lastUsed reset, and both survive a force stop and restart.
InsulinJsonAdapters is deleted. ICfg.toJson and ICfg.Companion.fromJson had no references left anywhere but their own declarations - ProfileSwitchExtension was the last user and moved to fromJsonObject. Checked the neighbours rather than assuming: determineBasalJson is used by IobTotalTest and IobTotal.json by LoopPlugin, so IobTotalJsonAdapters stays. pureProfileFromJson gains a String entry point in commonMain. Four callers held the profile as text - out of the database or off the Nightscout wire - and built an org.json document only to hand it straight over. That hop is gone, and two Nightscout sync files no longer touch org.json at all. New PureProfileFromJsonParityTest pins the three entry points (String, kotlinx JsonObject, org.json adapter) against each other, so the remaining callers can be moved without re-arguing that the profile they read is identical. It also pins the rules a conversion is most likely to drop quietly: - a profile without units is REJECTED. This is the one that matters: GlucoseUnit.fromText never throws, it answers MGDL, so if a missing unit stopped rejecting, an mmol/L profile would be read as mg/dL and every target, ISF and correction would be out by 18x. - values quoted as strings are still read as numbers, which real Nightscout documents rely on. - a missing schedule, and text that is not JSON at all, give an invalid profile rather than an exception. - an unknown timezone falls back to UTC. The org.json call sites inside core:objects' own tests are left alone on purpose - they are the only coverage the adapter has.
pureProfileFromJson now has no production caller left on its org.json overload -
only tests use it, and they are the adapter's only coverage.
Build-then-parse removed. DefaultProfile and DefaultProfileDPV assembled a JSON
document only to parse it straight back into a PureProfile, so every value made a
round trip through text. They build the blocks directly now and neither file
contains a single org.json reference. This works because blockFromJson derived
each Block's DURATION from consecutive timeAsSeconds fields, so the helpers that
emitted {time, value, timeAsSeconds} arrays become functions returning
List<Block> with the same arithmetic. Three fields went with the document -
dia, carbs_hr and delay were written but never read back, because none is part of
a PureProfile.
ATProfile.data() and AutotunePlugin.saveLastRun had the same wasted hop:
toPureNsJson already answers a kotlinx document and they rendered it to text just
to reparse it as org.json. Both stay on kotlinx now. AutotunePlugin.loadLastRun
uses the lenient kotlinx readers, so a document written by an older build - where
a whole double was stored as a bare integer - still reads back the same.
ProfileStoreObject is kotlinx throughout. Its own comment said it was waiting for
JsonHelper and pureProfileFromJson to speak kotlinx; both do, so two text round
trips are gone - with() no longer re-serialises the incoming document and
getData() no longer re-parses it on every call.
Found by review of this change:
- ATProfile.data() lost a fail-safe. org.json refused NaN and Infinity outright,
so a non finite tuned value aborted the document and the method answered null.
kotlinx writes the bare token NaN and the lenient reader parses it back, which
would put a NaN into a dosing profile. It now rejects the profile explicitly.
- ProfileStoreObject.getStore() stopped logging a malformed `store`. The old code
threw out of getJSONObject and logged; answering null silently would hide a
broken document.
- The getData KDoc still described the old copy-on-read behaviour, and the
getDefaultProfileName comment stated the old optString rule wrongly - it
answered the literal text "null" for an explicit null, not "".
Tests: new PureProfileFromJsonParityTest pins the three entry points against each
other, including that a profile without units is REJECTED - GlucoseUnit.fromText
never throws, it answers MGDL, so losing that rejection would read an mmol/L
profile as mg/dL. DefaultProfileTest gains block-boundary assertions, because the
existing midnight-only checks cannot see a duration error, and a case for age 0,
which the old `age > 18` branch never covered.
Verified on emulator: the profile store loads after a force stop and the viewer is
unchanged (24.48 U, Fiasp, IC 7.7 g/U, ISF 8.2 mmol/L/U), and the profile helper
recomputes. Autotune is disabled there with no stored last run, so loadLastRun has
no runtime coverage.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.