compileSdkVersion 33; androidx.activity/appcompat upgrades; transitives #944
Conversation
| @@ -2,14 +2,16 @@ | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
| xmlns:tools="http://schemas.android.com/tools"> | |||
|
|
|||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS"/> | |||
There was a problem hiding this comment.
New requirement to request this permission if your app posts any notifications. Good point that I didn't really look into why this was being linted and if there was a way of specifying that we don't post notifications. I kind of thought that, hey, might as well just include it. Too messy?
There was a problem hiding this comment.
So this is a branch new permission that only applies to API 33+ installs. On those you need to request the permission at runtime. There will be follow-on work for this. Alternative would be to leave it out and disable the Android Lint setting for this? I think this is likely the least intrusive way forward?
What will catch us is when we start running on API 33 devices. I will create an issue to track that.
There was a problem hiding this comment.
If the lint setting is as stupid as "always request this permission," then I'd be inclined to disable it. In samples I think we should work pretty hard only to include things we're certain we're actually using.
There was a problem hiding this comment.
It looks for code that uses Notifications, but this can be in classes from dependent libraries - classes which aren't necessarily used. This is only a warning, but since we have warning=error it fails the build.
I'm going to take these out and take a lint warnings baseline, then use that going forward.
There was a problem hiding this comment.
Turns out I don't think we need POST_NOTIFICATIONS once i finished updating some transitive dependency libraries (this isn't in the lint baseline). Obviously one of the libraries had to update for Android 13 as well.
| @@ -29,7 +31,15 @@ internal data class ViewStateFrame( | |||
| companion object CREATOR : Creator<ViewStateFrame> { | |||
| override fun createFromParcel(parcel: Parcel): ViewStateFrame { | |||
| val key = parcel.readString()!! | |||
| val viewState = parcel.readSparseArray<Parcelable>(ViewStateFrame::class.java.classLoader)!! | |||
| val viewState = if (VERSION.SDK_INT >= VERSION_CODES.TIRAMISU) { | |||
There was a problem hiding this comment.
I'm kind of amazed they didn't give us compat wrappers to do all this version checking
There was a problem hiding this comment.
Should we make our own? We already have a module of Androidx support methods.
There was a problem hiding this comment.
Could consider it for sure.
There was a problem hiding this comment.
Did not do this as I think this type of branching is pretty standard practice and I'm ok with it as it makes it clear what can happen down the road rather than hiding that behind some syntactic sugar.
steve-the-edwards
force-pushed
the
sedwards/compile-sdk-33
branch
5 times, most recently
from
e5b2ffa to
55f5d80
Compare
steve-the-edwards
requested review from
a team and
zach-klippenstein
as code owners
steve-the-edwards
force-pushed
the
sedwards/compile-sdk-33
branch
from
55f5d80 to
3c1e69f
Compare
gradle/libs.versions.toml
Outdated
| androidx-compose-compiler = "1.3.2" | ||
| androidx-compose-foundation = "1.3.1" | ||
| androidx-compose-material = "1.3.1" | ||
| androidx-compose-runtime = "1.3.3" | ||
| androidx-compose-ui = "1.3.3" |
There was a problem hiding this comment.
steve-the-edwards
force-pushed
the
sedwards/compile-sdk-33
branch
from
3c1e69f to
4226443
Compare
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/RenderAsState.kt
Outdated
Show resolved
Hide resolved
...pose-samples/src/androidTest/java/com/squareup/sample/compose/launcher/SampleLauncherTest.kt
Outdated
Show resolved
Hide resolved
steve-the-edwards
force-pushed
the
sedwards/compile-sdk-33
branch
2 times, most recently
from
10e1725 to
787fa0e
Compare
Fix RenderAsState which was exposed by test as state was not set initially always.
steve-the-edwards
force-pushed
the
sedwards/compile-sdk-33
branch
from
787fa0e to
9381294
Compare
| configure<BaseAppModuleExtension> { | ||
| lint { | ||
| baseline = file("lint-baseline.xml") | ||
| } | ||
| } |
There was a problem hiding this comment.
Added this to baseline android lint warnings at this snapshot.
| workflowScope.launch( | ||
| start = UNDISPATCHED, | ||
| context = Dispatchers.Unconfined | ||
| ) { |
There was a problem hiding this comment.
moved from shuttling these renderings and snapshot into compose state in an onEach started by a launchIn on Dispatchers.Unconfined to launching for the collection with an CoroutineStart.UNDISPATCHED coroutine and this fixes the timing issue we saw come up in the Compose sample test while scrolling the LazyColumn.
I discussed this with @zach-klippenstein . He thinks there may still be some Compose bug here but this is making the behavior more reliable for us for now and while it makes this PR messy I'm inclined to merge it.
There was a problem hiding this comment.
Following up here - Kotlin/kotlinx.coroutines#1326 (comment)
There was a problem hiding this comment.
This could be a big win. The problem your describing sounds very similar to one that has been plaguing a lot of our Compose tests.
There was a problem hiding this comment.
Oh, but we don't use renderAsState :(
With upgrading of Compose, we revealed a tricky bug in
renderAsStatecaught by theSampleLauncherTestwhile scrolling aLazyColumnto bring aComposableinto view whose state was driven byrenderAsState. The fix was to launch a coroutine to transfer the state from Workflow into Compose state via anUNDISPATCHEDlaunched coroutine rather than anonEachon the flow.