feat: Added coroutine worker for importing watch history #7973
feat: Added coroutine worker for importing watch history #7973SE-Bilal-Ahmad wants to merge 20 commits intolibre-tube:masterfrom
Conversation
|
If someone wants i can add an ongoing notification for subscription and playlist imports however that will not be able to tell how much content to import has been left |
There was a problem hiding this comment.
I generally think that the idea of putting the import work into a dedicated worker goes into the right direction, thanks for proposing.
However, we decided to not add Dagger/Hilt as a dependency, so please refactor your PR to work without it.
Apart from that I left some comments, please have a look at them. Without willing to attack you personally, this code looks a bit AI-generated .
We don't accept contributions that add much additional code that was AI-generated here. So please clarify that this is not the case or rewrite the AI-generated parts as we don't have any permission to license code that was stolen from other projects as GPL3.
|
@Bnyro do i need to rewrite the other code as well by that i meant the pausing handler since thats not AI generated the only Ai generated one is the notification part that too is in much smaller portion not all of it was Ai generated so please clarify |
|
Only the AI generated code. |
|
okay i will be rewriting it |
|
@Bnyro could you please take a look at this pull request and see whether it’s ready to be merged into the app or not |
| private const val YOUTUBE_IMG_URL = "https://img.youtube.com" | ||
| private val IMPORT_THUMBNAIL_QUALITY = "mqdefault" | ||
| private val VIDEO_ID_LENGTH = 11 | ||
| private val YOUTUBE_IMG_URL = "https://img.youtube.com" |
| classpath(libs.kotlin.serialization) | ||
| classpath(libs.androidx.navigation.safeargs) | ||
|
|
||
| classpath("com.google.dagger:hilt-android-gradle-plugin:2.41") |
There was a problem hiding this comment.
remove this please, we don't want to use dagger hilt
| val parsedId: UUID? = try { | ||
| UUID.fromString(id) | ||
| } catch (e: Exception) { | ||
| null | ||
| } |
There was a problem hiding this comment.
| val parsedId: UUID? = try { | |
| UUID.fromString(id) | |
| } catch (e: Exception) { | |
| null | |
| } | |
| val parsedId runCatching { | |
| UUID.fromString(id) | |
| }.getOrNull() |
| private val IMPORT_THUMBNAIL_QUALITY = "mqdefault" | ||
| private val VIDEO_ID_LENGTH = 11 | ||
| private val YOUTUBE_IMG_URL = "https://img.youtube.com" |
There was a problem hiding this comment.
Please make all these values const and move them to the companion object.
| } | ||
| }?.flatten().orEmpty() | ||
|
|
||
| registerReceiver() |
There was a problem hiding this comment.
Why register the receiver again here? It's already registered, right?
| val importFormatString = | ||
| inputData.getString(WorkersData.IMPORT_FORMAT) ?: ImportFormat.YOUTUBEJSON.toString() | ||
| val importFormat = enumValueOf<ImportFormat>(importFormatString) | ||
| val videos = fileUrisString?.map { a -> |
There was a problem hiding this comment.
| val videos = fileUrisString?.map { a -> | |
| val videos = fileUrisString?.flatMap { a -> |
and remove flatten at the end.
| inputData.getString(WorkersData.IMPORT_FORMAT) ?: ImportFormat.YOUTUBEJSON.toString() | ||
| val importFormat = enumValueOf<ImportFormat>(importFormatString) | ||
| val videos = fileUrisString?.map { a -> | ||
| val fileUrisDeserialized = Uri.parse(a) |
There was a problem hiding this comment.
| val fileUrisDeserialized = Uri.parse(a) | |
| val fileUri = a.toUri() |
| val importFormatString = | ||
| inputData.getString(WorkersData.IMPORT_FORMAT) ?: ImportFormat.YOUTUBEJSON.toString() | ||
| val importFormat = enumValueOf<ImportFormat>(importFormatString) | ||
| val videos = fileUrisString?.map { a -> |
There was a problem hiding this comment.
Please use a more descriptive name instead of a.
| setForeground(getForegroundInfo()) | ||
| val pausingHandle = ImportHandler() | ||
| val importType = inputData.getString(WorkersData.IMPORT_TYPE) | ||
| ?: ImportType.IMPORT_WATCH_HISTORY.toString() |
There was a problem hiding this comment.
If no import type is provided, we should probably throw an error instead of just assuming that it's IMPORT_WATCH_HISTORY.
| } | ||
|
|
||
|
|
||
| private suspend fun ImportWatchHistory(): Unit { |
There was a problem hiding this comment.
| private suspend fun ImportWatchHistory(): Unit { | |
| private suspend fun importWatchHistory(): Unit { |
method names should be lowercase
| } | ||
| } | ||
|
|
||
| private fun notifyNotification(notification: Notification) { |
There was a problem hiding this comment.
| private fun notifyNotification(notification: Notification) { | |
| private fun updateNotification(notification: Notification) { |
| } | ||
|
|
||
| companion object { | ||
| fun importWatchHistory(context: Context,uris: List<Uri>, importFormat: ImportFormat) { |
There was a problem hiding this comment.
| fun importWatchHistory(context: Context,uris: List<Uri>, importFormat: ImportFormat) { | |
| fun startImportWatchHistoryWorker(context: Context,uris: List<Uri>, importFormat: ImportFormat) { |
| private suspend fun checkIfPausedOrCancelled( | ||
| currentState: Int, | ||
| finalState: Int, | ||
| dispatch: suspend (Boolean) -> Unit | ||
| ) { | ||
| val importHandler = ImportHandler.current() | ||
| val now = System.currentTimeMillis() | ||
| if (importHandler.isPaused) { | ||
| dispatch(false) | ||
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.PAUSED)) | ||
| importHandler.awaitResumed() | ||
| } else if (!importHandler.isPaused) { | ||
| dispatch(true) | ||
| if (now - lastUpdateTime >= updateInterval) { | ||
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.RUNNING)) | ||
| lastUpdateTime = now | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't really understand what this method is doing, in any case its name is not very descriptive.
As I understand it, the method is supposed to only execute the work from dispatch if ImportHandler#paused is false. However, in this case, why don't you just do
| private suspend fun checkIfPausedOrCancelled( | |
| currentState: Int, | |
| finalState: Int, | |
| dispatch: suspend (Boolean) -> Unit | |
| ) { | |
| val importHandler = ImportHandler.current() | |
| val now = System.currentTimeMillis() | |
| if (importHandler.isPaused) { | |
| dispatch(false) | |
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.PAUSED)) | |
| importHandler.awaitResumed() | |
| } else if (!importHandler.isPaused) { | |
| dispatch(true) | |
| if (now - lastUpdateTime >= updateInterval) { | |
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.RUNNING)) | |
| lastUpdateTime = now | |
| } | |
| } | |
| } | |
| private suspend fun runActionAndUpdateNotificationIfNotPaused( | |
| currentState: Int, | |
| finalState: Int, | |
| action: suspend () -> Unit | |
| ) { | |
| val importHandler = ImportHandler.current() | |
| val now = System.currentTimeMillis() | |
| if (importHandler.isPaused) { | |
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.PAUSED)) | |
| importHandler.awaitResumed() | |
| } else if (!importHandler.isPaused) { | |
| action() | |
| if (now - lastUpdateTime >= updateInterval) { | |
| notifyNotification(notificationFactory.updateState(currentState, finalState, ImportState.RUNNING)) | |
| lastUpdateTime = now | |
| } | |
| } | |
| } |
| if (videos.isEmpty()) { | ||
| applicationContext.toastFromMainDispatcher(R.string.emptyList) |
There was a problem hiding this comment.
Please do this check before you execute the while loop for readability.
|
|
||
|
|
||
| var lastUpdateTime = 0L | ||
| val updateInterval = 500L |
There was a problem hiding this comment.
Please make update interval a constant in the companion object.
| private var importFormat: ImportFormat = ImportFormat.NEWPIPE | ||
|
|
||
|
|
||
| override val titleResourceId: Int = R.string.backup_restore |
There was a problem hiding this comment.
Please undo this change, perhaps this was by accident?
1- Previously a coroutine was executed which imported watch history from youtube but there was no way to know the status of the import history i have a added a coroutine worker whose job is to import the watch history with a foreground notification.
2- I have also added dagger hilt in order to inject dependencies. I could have used other approaches but that wouldnt have been a clean solution
3- For now only watch history is being imported through coroutine worker. The reason is that we are inserting watch item one by one but for subscriptions one complete list is inserted which makes it nearly impossible to know the status of those imports however i will still look for a way to achieve that and for playlists too
Bug #7907