Convert STAR sample to KMP #1103

ZacSweers · 2024-01-06T20:20:50Z

This PR converts the STAR sample into a multiplatform project!

The commits are a little winding, so I'd suggest looking at the files changed view for code and commits just for titles/story telling. The line diff is almost entirely just the updated baseline profile as I fixed the missing startup prof along the way.

Highlights

The STAR app now runs on Android and Desktop. This is just a starting point, support for others to continue later.
Update to Coil 3.0 alphas (with multiplatform support)
Revamped CoilRule project (which is now also multiplatform)
Using commonized annotations for both parcelize and dagger worked remarkably well, allowing almost all the impls to live in common sources.
Focused just on JVM for now for scoping reasons. Generified or expect/actual'd things where it made sense.
Expect/actual for overlay functions actually feels like a really slick indirection pattern, as it can let us use a bottom sheet on mobile and something more appropriate like a dialog for desktop.
The Platform.kt pattern works well here.
Multiplatform windowSizeClass works nicely: https://github.com/chrisbanes/material3-windowsizeclass-multiplatform
Supports dark mode via cmd+U. Quits with cmd+W.
Add STAR instrumentation tests to CI.
Switch to kotlinx-datetime for time APIs

Rough edges

Shared resources don't really have a story in compose multiplatform currently. There's some libraries and tools out there, but out of scope for this PR. Now we just have a Strings.kt file with our three strings.
Kapt cannot run on a KMP project with both Android and JVM targets, so we have to do annoying tedium to conditionally enable the JVM target only when requested. Ultimately the better solution would be to move to KSP or split them into separate subprojects.
I tried out kotlin-inject as a KMP solution and... there's a lot of missing features that I felt made it a little untenable. Might still be the only KMP solution available for the foreseeable future though.
- No modules support, everything is a component
- Obviously no anvil/contributing support
- Multibindings must be provided in the host component directly. Unclear how to provide lazy multibindings like Map<Class, Provider<Activity>>.
IDE completions often didn't know about common sources that could be imported, namely annotations.
KSP cannot see through typealias'd actual annotations 🤔. This resulted in my needing to support a lenient mode that allows for short name matching on the annotation rather than the fully qualified one. Filed KSP does not read actualized typealias declaration google/ksp#1676.
Desktop-specific - unclear where to store persistent data like a DB or token storage. Currently uses an in-memory DB and FakeFileSystem for token storage 🤷.
The IDE seems to use significantly more memory when doing a lot of KMP work. I was maxing out all 4gb of memory assigned to Android Studio in this branch.

TODO

Filter overlay on Desktop
Touch up info screen padding
Touch up landscape detail page
Escape key as a back nav event on desktop seems flaky at best, probably worth putting back the nav icon.
Finish getting this working in CI. It's annoying because check invokes the double kapt jvm/android issue. Probably need to just disable the desktop target by default and make it opt-in for building locally.

output.mp4

It seems anvil doesn't support intermediate source sets

ZacSweers · 2024-01-07T23:45:23Z

gradle.properties

@@ -31,11 +31,17 @@ org.gradle.parallel=true
 org.gradle.configureondemand=true
 org.gradle.caching=true

+# Disable noisy DAGP stability warning
+dependency.analysis.compatibility=NONE


Opportunistic noise cleanup

ZacSweers · 2024-01-07T23:45:47Z

gradle/libs.versions.toml

 kct = "0.4.0"
 kotlin = "1.9.22"
 kotlinpoet = "1.15.3"
 kotlinx-coroutines = "1.7.3"
 ksp = "1.9.22-1.0.16"
 ktfmt = "0.46"
+ktor = "2.3.7"


Required for Coil 3, backed by OkHttp on JVM

ZacSweers · 2024-01-07T23:46:40Z

gradle/libs.versions.toml

 leakcanary-android = { module = "com.squareup.leakcanary:leakcanary-android", version.ref = "leakcanary" }
 leakcanary-android-instrumentation = { module = "com.squareup.leakcanary:leakcanary-android-instrumentation", version.ref = "leakcanary" }

 lints-compose = "com.slack.lint.compose:compose-lint-checks:1.2.0"

+material = "com.google.android.material:material:1.11.0"


Required for M3 themes in themes.xml

ZacSweers · 2024-01-07T23:47:01Z

samples/counter/src/commonMain/kotlin/com/slack/circuit/sample/counter/Remove.kt

@@ -1,12 +1,25 @@
-// Copyright (C) 2022 Slack Technologies, LLC
-// SPDX-License-Identifier: Apache-2.0
+/*


Fixed this header while I was at it

ZacSweers · 2024-01-07T23:47:57Z

samples/star/apk/src/main/res/values-night/themes.xml

These theme files are just moved from the star library, unchanged

ZacSweers · 2024-01-07T23:52:29Z

samples/star/src/androidMain/kotlin/com/slack/circuit/star/data/InjectedTokenStorage.android.kt

+
+@ContributesBinding(AppScope::class)
+@SingleIn(AppScope::class)
+class InjectedTokenStorage @Inject constructor(storage: Storage<Preferences>) :


Unfortunately it's not enough to just have one contributed binding in commonJvm and instead needs to be duplicated in each target

ZacSweers · 2024-01-07T23:55:39Z

samples/star/src/commonMain/kotlin/com/slack/circuit/star/benchmark/ListBenchmarksScreen.kt

-fun ListBenchmarksItemPreview() {
-  Box { ListBenchmarksItem(ListBenchmarksItemScreen.State(0)) }
-}
+// @Preview


Previews are annoyingly not multiplatform compatible. Need to figure out a good solution for these

ZacSweers · 2024-01-07T23:56:46Z

samples/star/src/commonMain/kotlin/com/slack/circuit/star/home/AboutScreen.kt

 @CircuitInject(screen = AboutScreen::class, scope = AppScope::class)
 @Composable
 fun About(modifier: Modifier = Modifier) {
-  Scaffold(


The scaffold was redundant, removed opportunistically

ZacSweers · 2024-01-08T00:48:17Z

samples/star/src/commonMain/kotlin/com/slack/circuit/star/ui/HorizontalPagerIndicator.kt

+ */
+@OptIn(ExperimentalFoundationApi::class)
+@Composable
+fun HorizontalPagerIndicator(


Copied as accompanist isn't multiplatform

ZacSweers · 2024-01-08T00:49:09Z

samples/star/src/jvmMain/kotlin/com/slack/circuit/star/common/BackPressNavIcon.jvm.kt

+  iconButtonContent: @Composable () -> Unit,
+) {
+  // We do nothing on desktop
+  // TODO maybe we do escaping?


We'd need to do something that sends an escape event to the window, as that's sorta how I've implemented backstack popping here

Think we'll have to create a back press local so that it can get triggered correctly on the window 🤔

yeah, I think it's out of scope for this PR as a start anyway

samples/star/README.md

stagg · 2024-01-10T20:26:42Z

samples/star/src/commonMain/kotlin/com/slack/circuit/star/petdetail/PetPhotoCarouselScreen.kt

-      } ?: Modifier
+        } ?: Modifier
+      } else {
+        Modifier


Not sure whats needed but it'd be nice to swipe on the pager via mouse event, or have the little side arrows on desktop.

fun fact that's already implemented, works on android too :P. You can see it in action in the video when the images stop scrolling and start snapping

stagg · 2024-01-11T01:40:09Z

samples/star/src/jvmMain/kotlin/com/slack/circuit/star/common/BackPressNavIcon.jvm.kt

+  iconButtonContent: @Composable () -> Unit,
+) {
+  // We do nothing on desktop
+  // TODO maybe we do escaping?


Think we'll have to create a back press local so that it can get triggered correctly on the window 🤔

Co-authored-by: Josh Stagg <[email protected]>

ZacSweers added 30 commits October 7, 2023 13:42

Abstract android bits out of TokenStorage

a0254a3

Extract out android APIs in PetRepository

1510f37

Use compose key events in PetPhotoCarousel

41af3e6

Move preview out + use entries

f105cfd

Fix typo

364cd27

Switch out deps

b2a95f4

Merge branch 'main' into z/kmpStar

fc91c6a

Merge branch 'main' into z/kmpStar

7c2aca1

Add coil3 coordinates

fd6951f

More dep organization

1bc471c

Fix coil3 group

6bac40c

Finish build file setup

3b62dfc

Rename dirs to new structure

5896b7d

Initial pass at aligning sources

e7eaee9

KMP SqlDriverFactory

c1b9af5

KMP BackPressNavIcon

986a5eb

KMP ConditionalSystemUiColors

c182c2d

Introduce common parcelizing APIs

a2ac08c

Misc work and prep

908a873

Spotless pass

be41cf6

Android builds now

9135336

Update CoilRule to c3

27cdde7

STAR apk builds

7bc1b5e

Extract more things down to commonMain

4f3dda9

Extract ListBenchmarksScreen

27496b0

Push whole impls down into common using typealias trick

3c994bd

More extractions

3e81bf9

It seems anvil doesn't support intermediate source sets

Desktop is working!

ca26de7

Source jvmTarget from libs

461f53f

Verify star desktop on CI

d608b9f

ZacSweers added 3 commits January 7, 2024 19:40

Icon cleanup

4d85c60

Handle url clicks

a4bb63a

Get CI check working

f527798

ZacSweers commented Jan 8, 2024

View reviewed changes

ZacSweers added 5 commits January 7, 2024 20:38

Remove hamcrest

9ec3e30

Run star instrumentation tests on CI too

ebe86c1

Use kotlinx-datetime for multiplatform

2426c77

More time APIs

adbda8c

Use in-memory FS for token storage on desktop

b148272

ZacSweers mentioned this pull request Jan 8, 2024

KSP does not read actualized typealias declaration google/ksp#1676

Closed

Merge branch 'main' into z/kmpStar

a2ca059

This comment was marked as resolved.

Sign in to view

ZacSweers added 11 commits January 8, 2024 00:49

Try an explicitly ATD model

cd63a5f

Fix leakcanary config

eb4566f

Add missing rules

b3f1354

Update profile generation for AGP 8.2 + enable startup-prof

da0d732

Fix AboutScreen on android

4c2a768

Fix wrong platform type on android

3a2dbe8

Update EW config

30733a6

Spotless

1215b4c

Regenerate snapshots to make lfs happy?

d9a0bbc

Fix test

fa28fe3

More fixes

c3b34a7

stagg reviewed Jan 11, 2024

View reviewed changes

Update samples/star/README.md

b949d2a

Co-authored-by: Josh Stagg <[email protected]>

stagg approved these changes Jan 11, 2024

View reviewed changes

ZacSweers added this pull request to the merge queue Jan 11, 2024

Merged via the queue into main with commit ed96619 Jan 11, 2024
4 checks passed

ZacSweers deleted the z/kmpStar branch January 11, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert STAR sample to KMP #1103

Convert STAR sample to KMP #1103

ZacSweers commented Jan 6, 2024 •

edited

Loading

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 7, 2024

ZacSweers Jan 8, 2024

ZacSweers Jan 8, 2024

stagg Jan 11, 2024

ZacSweers Jan 11, 2024

This comment was marked as resolved.

stagg Jan 10, 2024

ZacSweers Jan 11, 2024

stagg Jan 11, 2024

Convert STAR sample to KMP #1103

Convert STAR sample to KMP #1103

Conversation

ZacSweers commented Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Jan 6, 2024 •

edited

Loading