Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Kotlin 2.0 and SDK 35 #525

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

stevenzeck
Copy link
Contributor

@stevenzeck stevenzeck commented May 28, 2024

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

  • Upgraded to Kotlin 2.0.x.
  • Updated Android SDK to 35.
  • KSP2 is not ready as it errors with Room. Leaving it commented out in gradle.properties. It won't be ready until Room is updated to 2.7.0 (currently in alpha), and a KSP2 issue with @TypeConverters.
  • The Compose compiler now ships with Kotlin, instead of relying on keeping that synchronized.
  • I used https://github.com/autonomousapps/dependency-analysis-gradle-plugin as a starting point to see which dependencies aren't needed in each module. I did remove all of the testImplementation and androidTestImplementation ones since they weren't being used. They can always be added back.
  • Added a flag to the compiler options for Navigator and LCP modules: -Xconsistent-data-class-copy-visibility. See KT-11914.

@stevenzeck
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

Fix will be in 2.0.20.

@stevenzeck
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

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.

@stevenzeck stevenzeck changed the title Update to Kotlin 2.0 Update to Kotlin 2.0 and SDK 35 Sep 11, 2024
@qnga
Copy link
Contributor

qnga commented Sep 26, 2024

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
Copy link
Contributor

qnga commented Sep 26, 2024

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
Copy link
Contributor Author

stevenzeck commented Sep 27, 2024

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
Copy link
Contributor

qnga commented Sep 27, 2024

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

@stevenzeck
Copy link
Contributor Author

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
Copy link
Contributor

qnga commented Oct 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants