readium / kotlin-toolkit

A toolkit for ebooks, audiobooks and comics written in Kotlin
https://readium.org/kotlin-toolkit
BSD 3-Clause "New" or "Revised" License
180 stars 77 forks source link

Update to Kotlin 2.0 and SDK 35 #525

Closed stevenzeck closed 1 day ago

stevenzeck commented 4 months ago

Do not merge unless ready to accept Kotlin 2.0. Opening this to make sure the checks pass.

stevenzeck commented 2 months ago

@mickael-menu This causes the app to crash with the below error when opening an epub. Declaring R2RTLViewPager as public fixes it. I don't know why, that old code just goes in a circle.

Possibly related to https://youtrack.jetbrains.com/issue/KT-63050/K1-K2-IllegalAccessError-when-accessing-a-private-field-isnt-detected-at-compile-time. Even though the OP there was using Kotlin 1.6.21 when reporting it.

java.lang.IllegalAccessError: Illegal class access: 'org.readium.r2.navigator.epub.EpubNavigatorFragment' attempting to access 'org.readium.r2.navigator.pager.R2RTLViewPager' (declaration of 'org.readium.r2.navigator.epub.EpubNavigatorFragment' appears in /data/app/~~-rjhsDrV6c9hFpC_VngmIw==/org.readium.r2reader-y1pHEqGxpJ6lP_pzngh9Iw==/base.apk!classes4.dex)
at org.readium.r2.navigator.epub.EpubNavigatorFragment.resetResourcePagerAdapter(EpubNavigatorFragment.kt:497)
at org.readium.r2.navigator.epub.EpubNavigatorFragment.resetResourcePager(EpubNavigatorFragment.kt:475)
at org.readium.r2.navigator.epub.EpubNavigatorFragment.onCreateView(EpubNavigatorFragment.kt:414)
mickael-menu commented 2 months ago

We could set it public with an @InternalReadiumApi annotation as a workaround. I wonder if this is related to the fact that R2RTLViewPager is a Java class. In which case we might have a similar problem when using the streaming ZIP container (e.g. when opening an EPUB from the shared storage).

stevenzeck commented 2 months ago

Fix will be in 2.0.20.

stevenzeck commented 1 month ago

@mickael-menu There is new behavior regarding non-public constructors of data classes and the copy() function, see KT-11914. I'm thinking we should annotate these with @ConsistentCopyVisibility; I don't think copy should be publicly visible in these cases.

Applicable to:

mickael-menu commented 1 month ago

@stevenzeck I agree, and that's a welcoming change!

Configuration has actually a public constructor, which delegates to the internal one. So copy should stay public there.

All the types under StateMachine are internal to the toolkit so I wouldn't bother annotating them as copy will never be part of the public API.

stevenzeck commented 1 month ago

I misread the phases. The @ConsistentCopyVisibility annotation (or -Xconsistent-data-class-copy-visibility flag) are only be necessary until Phase 3. I added the flag to the navigator and LCP gradle files.

qnga commented 1 week ago

What about Edge-to-edge content being the default with SDK 35 on Android 15? I think this should be tested on an actual Android 15 device as soon as we can.

qnga commented 1 week ago

We might need something like that on non-reading activities:

enableEdgeToEdge()
setContentView(R.layout.activity_main)
ViewCompat.setOnApplyWindowInsetsListener(findViewById(R.id.main)) { v, insets ->
    val systemBars = insets.getInsets(WindowInsetsCompat.Type.systemBars())
    v.setPadding(systemBars.left, systemBars.top, systemBars.right, systemBars.bottom)
    insets
}

That's what's AndroidStudio suggests now in new empty view projects.

stevenzeck commented 1 week ago

I tested it in the emulator, and yes padding was needed for the status bar. The BottomNavigationView handles that behavior change. enableEdgeToEdge() is only if the app should do edge-to-edge on older Android versions.

I did notice an issue in light mode when enableEdgeToEdge() is not in there in that the status bar text and icon colors don't contrast with the background. It's not specific to Readium though. Though it might be because there isn't much of a theme in there. I created a bug report.

qnga commented 1 week ago

In my understanding, if you don't call enableEdgeToEdge, you must apply padding only on Android 15. I didn't test it though.

stevenzeck commented 6 days ago

ViewCompat.setOnApplyWindowInsetsListener doesn't seem to be called in Android 14 and below if enableEdgeToEdge isn't in there. Up to you, but I think it's fine without it. I don't quite understand what they're doing with the status bar color when edge-to-edge is enabled. It doesn't use the dynamic colors.

qnga commented 4 days ago

ViewCompat.setOnApplyWindowInsetsListener doesn't seem to be called in Android 14 and below if enableEdgeToEdge isn't in there.

Sounds weird. That's something we've been using for a long time at different places. We'll see during tests.

stevenzeck commented 1 day ago

Closing in favor of splitting this out.