Skip to content

Conversation

@theimpulson
Copy link
Member

@theimpulson theimpulson commented Oct 27, 2025

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Converts the database package to Kotlin and adapts classes dependent upon it
  • Migrates from KAPT to KSP based on the Kotlin version

Closes #12752

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/giant PRs with more than 750 changed lines label Oct 27, 2025
@TobiGr TobiGr added codequality Improvements to the codebase to improve the code quality database Issue and PRs related to database operations dependency Issues and PRs related to dependencies labels Oct 27, 2025
@theimpulson
Copy link
Member Author

> Task :app:connectedDebugAndroidTest
org.schabi.newpipe.local.subscription.SubscriptionManagerTest > testInsert[emulator-5554 - 5.0.2] FAILED 
	org.schabi.newpipe.extractor.exceptions.ContentNotAvailableException: Got error:"INTERNAL": Internal error encountered.
	at org.schabi.newpipe.extractor.services.youtube.YoutubeChannelHelper.checkIfChannelResponseIsValid(YoutubeChannelHelper.java:249)
Tests on emulator-5554 - 5.0.2 failed: There was 1 failure(s).

@theimpulson theimpulson requested review from Stypox and TobiGr October 28, 2025 04:04
@theimpulson theimpulson marked this pull request as draft October 28, 2025 06:03
@theimpulson
Copy link
Member Author

theimpulson commented Oct 28, 2025

For some reason after using KSP, FeedDao has null errors with KSP on runtime but not with KAPT. It is further strange as classes related to it weren't changed. Trying to upgrade the kotlin and KSP version to see if that fixes it.

@theimpulson theimpulson marked this pull request as ready for review October 28, 2025 07:13
@theimpulson
Copy link
Member Author

I didn't updated room to the latest 2.8.x versions as that drops support for Android 5.x devices (AndroidX dropped support for them) as I assume we will want to do that with another update but not the next one.

@theimpulson
Copy link
Member Author

* What went wrong:
Execution failed for task ':app:lintAnalyzeDebug'.
> A failure occurred while executing com.android.build.gradle.internal.lint.AndroidLintWorkAction
   > Unexpected failure during lint analysis (this is a bug in lint or one of the libraries it depends on)
     
     Message: Unexpected failure during lint analysis (this is a bug in lint or one of the libraries it depends on)
     
     Message: \\\`findFirCompiledSymbol\\\` only works on compiled declarations, but the given declaration is not compiled.
     Stack: \`KotlinIllegalArgumentExceptionWithAttachments:LLResolutionFacade.findCompiledFirSymbol(LLResolutionFacade.kt:221)←LLResolutionFacade.resolveToFirSymbol$low_level_api_fir(LLResolutionFacade.kt:129)←LowLevelFirApiFacadeKt.resolveToFirSymbol(LowLevelFirApiFacade.kt:36)←KaFirScriptSymbol$special$$inlined$lazyFirSymbol$1.invoke(KaFirPsiSymbol.kt:327)←KaFirScriptSymbol$special$$inlined$lazyFirSymbol$1.invoke(KaFirPsiSymbol.kt:246)←SafePublicationLazyImpl.getValue(LazyJVM.kt:125)←KaFirPsiSymbol.getFirSymbol(KaFirPsiSymbol.kt:70)←KtSymbolUtilsKt.getFirSymbol(ktSymbolUtils.kt:26)←KaFirScopeProvider.getDeclaredMemberScope(KaFirScopeProvider.kt:123)←KaFirScopeProvider.getCombinedDeclaredMemberScope(KaFirScopeProvider.kt:105)←KaBaseSession.getCombinedDeclaredMemberScope(KaBaseSession.kt:-1)←SymbolLightClassUtilsKt.addPropertyBackingFields(symbolLightClassUtils.kt:429)←SymbolLightClassUtilsKt.addPropertyBackingFields$default(symbolLightClassUtils.kt:422)←SymbolLightClassForScript$getOwnFields$$inlined$cachedValue$1.compute(symbolLightUtils.kt:362)←CachedValuesManager$NonPhysicalPsiHandlerProvider.compute(CachedValuesManager.java:222)←CachedValuesManager$NonPhysicalPsiHandlerProvider.compute(CachedValuesManager.java:215)←PsiParameterizedCachedValue.doCompute(PsiParameterizedCachedValue.kt:24)←CachedValueBase.lambda$getValueWithLock$3(CachedValueBase.java:299)←CachedValueBase.computeData(CachedValueBase.java:37)←CachedValueBase.lambda$getValueWithLock$4(CachedValueBase.java:299)←RecursionManager$1.computePreventingRecursion(RecursionManager.java:113)←RecursionGuard.doPreventingRecursion(RecursionGuard.java:28)←RecursionManager.doPreventingRecursion(RecursionManager.java:68)←CachedValueBase.getValueWithLock(CachedValueBase.java:300)←PsiParameterizedCachedValue.getValue(PsiParameterizedCachedValue.kt:18)←CachedValuesManager.getParameterizedCachedValue(CachedValuesManager.java:97)←CachedValuesManager.getCachedValue(CachedValuesManager.java:212)←CachedValuesManager.getCachedValue(CachedValuesManager.java:136)←SymbolLightClassForScript.getOwnFields(SymbolLightClassForScript.kt:233)←SymbolLightClassBase.getFields(SymbolLightClassBase.kt:50)←UClass.getFields(UClass.kt:48)←KotlinScriptUClass.getFields(KotlinScriptUClass.kt:44)←AbstractKotlinUClass.getUastDeclarations(AbstractKotlinUClass.kt:34)←AbstractKotlinUClass.accept(AbstractKotlinUClass.kt:223)←ImplementationUtilsKt.acceptList(implementationUtils.kt:15)←UFile.accept(UFile.kt:89)←UastLintUtilsKt.acceptSourceFile(UastLintUtils.kt:1026)←UastGradleVisitor.visitBuildScript(UastGradleVisitor.kt:42)←LintDriver$checkBuildScripts$3.run(LintDriver.kt:1469)←LintClient.runReadAction(LintClient.kt:1738)←LintCliClient.runReadAction(LintCliClient.kt:231)←LintDriver$LintClientWrapper.runReadAction(LintDriver.kt:2826)←LintDriver.checkBuildScripts(LintDriver.kt:1443)←LintDriver.runFileDetectors(LintDriver.kt:1306)←LintDriver.checkProject(LintDriver.kt:1072)←LintDriver.checkProjectRoot(LintDriver.kt:624)←LintDriver.access$checkProjectRoot(LintDriver.kt:176)←LintDriver$analyzeOnly$1.invoke(LintDriver.kt:447)←LintDriver$analyzeOnly$1.invoke(LintDriver.kt:444)←LintDriver.doAnalyze(LintDriver.kt:503)←LintDriver.analyzeOnly(LintDriver.kt:444)←LintCliClient$analyzeOnly$1.invoke(LintCliClient.kt:276)←LintCliClient$analyzeOnly$1.invoke(LintCliClient.kt:273)←LintCliClient.run(LintCliClient.kt:330)←LintCliClient.analyzeOnly(LintCliClient.kt:273)←Main.run(Main.java:1773)←Main.run(Main.java:283)←DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)←Method.invoke(Method.java:580)←AndroidLintWorkAction.invokeLintMainRunMethod(AndroidLintWorkAction.kt:103)←AndroidLintWorkAction.runLint(AndroidLintWorkAction.kt:90)←AndroidLintWorkAction.execute(AndroidLintWorkAction.kt:64)←DefaultWorkerServer.execute(DefaultWorkerServer.java:63)←NoIsolationWorkerFactory$1$1.create(NoIsolationWorkerFactory.java:66)←NoIsolationWorkerFactory$1$1.create(NoIsolationWorkerFactory.java:62)←ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100)←NoIsolationWorkerFactory$1.lambda$execute$0(NoIsolationWorkerFactory.java:62)←AbstractWorker$1.call(AbstractWorker.java:44)←AbstractWorker$1.call(AbstractWorker.java:41)←DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:210)←DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:205)←DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:67)←DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:60)←DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:167)←DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:60)←DefaultBuildOperationRunner.call(DefaultBuildOperationRunner.java:54)←AbstractWorker.executeWrappedInBuildOperation(AbstractWorker.java:41)←NoIsolationWorkerFactory$1.execute(NoIsolationWorkerFactory.java:59)←DefaultWorkerExecutor.lambda$submitWork$0(DefaultWorkerExecutor.java:174)←FutureTask.run(FutureTask.java:317)←DefaultConditionalExecutionQueue$ExecutionRunner.runExecution(DefaultConditionalExecutionQueue.java:194)←DefaultConditionalExecutionQueue$ExecutionRunner.access$700(DefaultConditionalExecutionQueue.java:127)←DefaultConditionalExecutionQueue$ExecutionRunner$1.run(DefaultConditionalExecutionQueue.java:169)←Factories$1.create(Factories.java:31)←DefaultWorkerLeaseService.withLocks(DefaultWorkerLeaseService.java:263)←DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:127)←DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:132)←DefaultConditionalExecutionQueue$ExecutionRunner.runBatch(DefaultConditionalExecutionQueue.java:164)←DefaultConditionalExecutionQueue$ExecutionRunner.run(DefaultConditionalExecutionQueue.java:133)←Executors$RunnableAdapter.call(Executors.java:572)←FutureTask.run(FutureTask.java:317)←ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)←AbstractManagedExecutor$1.run(AbstractManagedExecutor.java:48)←ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)←ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)←Thread.run(Thread.java:1583)\`
     
     You can run with --stacktrace or set environment variable \`LINT_PRINT_STACKTRACE=true\` to dump a full stacktrace to stdout.
     Stack: `RuntimeException:LintDriver$Companion.handleDetectorError(LintDriver.kt:3880)←LintDriver$Companion.handleDetectorError$default(LintDriver.kt:3739)←LintDriver$Companion.handleDetectorError(LintDriver.kt:3736)←LintDriver$analyzeOnly$1.invoke(LintDriver.kt:449)←LintDriver$analyzeOnly$1.invoke(LintDriver.kt:444)←LintDriver.doAnalyze(LintDriver.kt:503)←LintDriver.analyzeOnly(LintDriver.kt:444)←LintCliClient$analyzeOnly$1.invoke(LintCliClient.kt:276)←LintCliClient$analyzeOnly$1.invoke(LintCliClient.kt:273)←LintCliClient.run(LintCliClient.kt:330)←LintCliClient.analyzeOnly(LintCliClient.kt:273)←Main.run(Main.java:1773)←Main.run(Main.java:283)←DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)←Method.invoke(Method.java:580)←AndroidLintWorkAction.invokeLintMainRunMethod(AndroidLintWorkAction.kt:103)←AndroidLintWorkAction.runLint(AndroidLintWorkAction.kt:90)←AndroidLintWorkAction.execute(AndroidLintWorkAction.kt:64)←DefaultWorkerServer.execute(DefaultWorkerServer.java:63)←NoIsolationWorkerFactory$1$1.create(NoIsolationWorkerFactory.java:66)←NoIsolationWorkerFactory$1$1.create(NoIsolationWorkerFactory.java:62)←ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100)←NoIsolationWorkerFactory$1.lambda$execute$0(NoIsolationWorkerFactory.java:62)←AbstractWorker$1.call(AbstractWorker.java:44)←AbstractWorker$1.call(AbstractWorker.java:41)←DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:210)←DefaultBuildOperationRunner$CallableBuildOperationWorker.execute(DefaultBuildOperationRunner.java:205)←DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:67)←DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:60)←DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:167)←DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:60)←DefaultBuildOperationRunner.call(DefaultBuildOperationRunner.java:54)←AbstractWorker.executeWrappedInBuildOperation(AbstractWorker.java:41)←NoIsolationWorkerFactory$1.execute(NoIsolationWorkerFactory.java:59)←DefaultWorkerExecutor.lambda$submitWork$0(DefaultWorkerExecutor.java:174)←FutureTask.run(FutureTask.java:317)←DefaultConditionalExecutionQueue$ExecutionRunner.runExecution(DefaultConditionalExecutionQueue.java:194)←DefaultConditionalExecutionQueue$ExecutionRunner.access$700(DefaultConditionalExecutionQueue.java:127)←DefaultConditionalExecutionQueue$ExecutionRunner$1.run(DefaultConditionalExecutionQueue.java:169)←Factories$1.create(Factories.java:31)←DefaultWorkerLeaseService.withLocks(DefaultWorkerLeaseService.java:263)←DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:127)←DefaultWorkerLeaseService.runAsWorkerThread(DefaultWorkerLeaseService.java:132)←DefaultConditionalExecutionQueue$ExecutionRunner.runBatch(DefaultConditionalExecutionQueue.java:164)←DefaultConditionalExecutionQueue$ExecutionRunner.run(DefaultConditionalExecutionQueue.java:133)←Executors$RunnableAdapter.call(Executors.java:572)←FutureTask.run(FutureTask.java:317)←ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)←AbstractManagedExecutor$1.run(AbstractManagedExecutor.java:48)←ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)←ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)←Thread.run(Thread.java:1583)`
     
     You can run with --stacktrace or set environment variable `LINT_PRINT_STACKTRACE=true` to dump a full stacktrace to stdout.

@theimpulson
Copy link
Member Author

Downgraded back to current versions as that seems to be a task for another PR. All seems to be good so far.

@Stypox
Copy link
Member

Stypox commented Oct 28, 2025

I didn't updated room to the latest 2.8.x versions as that drops support for Android 5.x devices (AndroidX dropped support for them) as I assume we will want to do that with another update but not the next one.

Since keeping Android 5 around doesn't seem to create as many problems as Android 4.4 did, I'd keep supporting 5.x as much as possible. We don't have plans to drop support for it, as of now.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! This converts to Kotlin a big chunk of stuff!

Should this PR be made on the refactor branch? I figure it might introduce unexpected bugs due to the big amount of files touched, so I'd rather keep it experimental. At the moment the dev branch is there just for bugfixes anyway and should be kept as stable as possible.

Please check, in all converted files, that the nullability of fields makes sense, and make them non-null unless they are nullable in the database. This should allow removing some ? and especially some !!, which ideally shouldn't exist at all. I left a few comments about this but not for all files

Note: before merging someone should do a very quick comparison of the files with a proper git diff just to check if there is any unexpected difference between the java and the kotlin files. Unfortunately the GitHub UI doesn't allow comparing files with different names.

@theimpulson
Copy link
Member Author

Should this PR be made on the refactor branch? I figure it might introduce unexpected bugs due to the big amount of files touched, so I'd rather keep it experimental. At the moment the dev branch is there just for bugfixes anyway and should be kept as stable as possible.

I wasn't sure about this, so just opened against the dev branch as that seems to the one currently being used for releases. What's our plan with branches? Do we intend to keep doing releases using dev with new completed compose changes or switch to refactor once everything is in compose and do a big release?

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Note: before merging someone should do a very quick comparison of the files with a proper git diff just to check if there is any unexpected difference between the java and the kotlin files. Unfortunately the GitHub UI doesn't allow comparing files with different names.

I did this. Looks good from my side. The only thing i noticed was this

@theimpulson theimpulson force-pushed the kspMigration branch 2 times, most recently from d734611 to 8d1cb73 Compare November 3, 2025 08:01
Room has been convereted into a KMP library in the latest stable releases and
annotation processing requires KSP which only generates kotlin classes

Signed-off-by: Aayush Gupta <[email protected]>
The lastUpdated parameter can be null, adjust return types to signal that too

Signed-off-by: Aayush Gupta <[email protected]>
The defaults should be supplied to the image loading software not the database library.
This would also break when we shrink resources as that would rename the resources.

Signed-off-by: Aayush Gupta <[email protected]>
Fixes [ksp] app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistStreamDAO.kt:140: The columns returned by the query does not have the fields [thumbnailUrl,isThumbnailPermanent,thumbnailStreamId,displayIndex,orderingName] in org.schabi.newpipe.database.playlist.PlaylistDuplicatesEntry even though they are annotated as non-null or primitive. Columns returned by the query: [uid,streamCount,timesStreamIsContained]

Signed-off-by: Aayush Gupta <[email protected]>
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! The two commits you pushed make sense to me, I confirm that Picasso will just show the correct placeholder when the playlist url is null. I also tested on my device by importing my backup with some playlists and they seem to work. I think we can merge this, if @TobiGr does not have any more comments.

Edit: are you sure that (SELECT thumbnail_url FROM streams WHERE streams.uid = thumbnail_stream_id) just returns null when thumbnail_stream_id is not a stream id?

Edit2: see here, so LGTM. I also tested an empty playlist which works.

During a particular execution, if the subquery returns no rows, that is not an error; the scalar result is taken to be null.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

I tested everything once again. LGTM

@TobiGr TobiGr merged commit f836f5e into dev Nov 7, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Rewrite Nov 7, 2025
@TobiGr TobiGr deleted the kspMigration branch November 7, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codequality Improvements to the codebase to improve the code quality database Issue and PRs related to database operations dependency Issues and PRs related to dependencies size/giant PRs with more than 750 changed lines

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Potential UI thread blocking issue

5 participants