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

Updating dependencies, SDKs, and more #368

Merged
merged 12 commits into from
Jul 26, 2023
Merged

Conversation

stevenzeck
Copy link
Contributor

@stevenzeck stevenzeck commented Jul 24, 2023

What's been updated

  1. Update all dependencies to latest versions.
  2. Update compile and target SDKs to 34.
  3. Update Gradle to 8.0, and AGP to 8.1.0.
  4. Use Java 17 (required by AGP update).

Important notes

  1. KSP has replaced Kapt for Room. There is an issue with Room 2.5.2 and Kotlin 1.9.0 that won't be fixed until 2.5.3. One of the workarounds in the issue is to use KSP instead of Kapt. Kapt is in maintenance mode and KSP offers significant performance improvements.
  2. With Kotlin 1.8, we don't need to declare kotlin-stdlib-jdk7 or 8 as they have been merged into a straightforward kotlin-stdlib. https://kotlinlang.org/docs/whatsnew18.html#updated-jvm-compilation-target.
  3. android.nonTransitiveRClass defaults to true with the new AGP version. Because of this, when accessing IDs from external libraries, you have to prefix it with the namespace. For example, R.id.search_close_btn is now androidx.appcompat.R.id.search_close_btn.
  4. If BuildConfig is needed for individual modules, buildConfig = true must be added to the buildFeatures block in build.gradle.kts for that module.

What else changed

  1. When targeting Android SDK 34 you must add android.permission.FOREGROUND_SERVICE_MEDIA_PLAYBACK in order for TTS or Audiobook playback to work.

@stevenzeck stevenzeck marked this pull request as ready for review July 25, 2023 02:01
@stevenzeck
Copy link
Contributor Author

@mickael-menu @qnga Please review, especially the Kotlin file changes.

I tested everything except for LCP. But please test with your devices too.

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stevenzeck, I tested again the various formats with LCP too.

test-app/src/main/AndroidManifest.xml Show resolved Hide resolved
readium/navigator/build.gradle.kts Outdated Show resolved Hide resolved
settings.gradle.kts Show resolved Hide resolved
Comment on lines +195 to +197
com.pspdfkit.R.id.pspdf__text_selection_toolbar_item_share,
com.pspdfkit.R.id.pspdf__text_selection_toolbar_item_copy,
com.pspdfkit.R.id.pspdf__text_selection_toolbar_item_speak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change have consequences? Maybe these resources cannot be overridden any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually because android.nonTransitiveRClass defaults to true with the updated AGP version.

Another fix would be to set that to false in gradle.properties, then we can continue using R.id.something instead of package.namespace.R.id.something. There is only one other place this was necessary and it was in the testapp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that we can still override PSPDFKit strings with these changes, from an app's module. I tried with this:

<string name="pspdf__action_menu_copy">Test</string>

@stevenzeck
Copy link
Contributor Author

Looks to be an issue with ExoPlayer in which the test app requires android.permission.POST_NOTIFICATIONS. google/ExoPlayer#10884

@mickael-menu
Copy link
Member

@stevenzeck The ExoPlayer DownloadService is not used in the toolkit, so it should not be required. Did you encounter an issue with the Test App?

@mickael-menu
Copy link
Member

So apparently we need this permission even without using the DownloadService, because otherwise the linting step fails. See this comment: google/ExoPlayer#10884 (comment)

@mickael-menu mickael-menu merged commit 0cb5961 into readium:v3 Jul 26, 2023
3 checks passed
@mickael-menu mickael-menu deleted the upgrades branch July 26, 2023 14:34
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