From 701fffb758e774d8ff0dc8bb794f569a47243277 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Mon, 19 May 2025 19:03:11 +0530 Subject: [PATCH 1/4] Refactored RxJava to coroutines in OnlineLibrary --- .../library/local/LocalLibraryFragment.kt | 1 - .../library/online/OnlineLibraryFragment.kt | 10 +- .../zimManager/ZimManageViewModel.kt | 345 ++++++++---------- .../kiwix/kiwixmobile/core/data/DataSource.kt | 19 +- .../kiwix/kiwixmobile/core/data/Repository.kt | 79 ++-- .../core/data/remote/KiwixService.kt | 12 +- .../core/di/modules/ApplicationModule.kt | 18 - .../core/downloader/DownloaderImpl.kt | 31 +- .../kiwixmobile/core/main/AddNoteDialog.kt | 69 ++-- .../core/main/CoreReaderFragment.kt | 5 +- .../core/main/MainRepositoryActions.kt | 76 ++-- .../core/settings/CorePrefsFragment.kt | 7 +- .../core/settings/SettingsContract.kt | 2 +- .../core/settings/SettingsPresenter.kt | 21 +- .../kiwixmobile/core/StorageObserverTest.kt | 13 - .../custom/download/CustomDownloadFragment.kt | 4 - .../download/effects/DownloadCustomTest.kt | 4 +- 17 files changed, 307 insertions(+), 409 deletions(-) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/local/LocalLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/local/LocalLibraryFragment.kt index 3141090f5..9ce956208 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/local/LocalLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/local/LocalLibraryFragment.kt @@ -520,7 +520,6 @@ class LocalLibraryFragment : BaseFragment(), CopyMoveFileHandler.FileCopyMoveCal override fun onDestroyView() { super.onDestroyView() - mainRepositoryActions.dispose() actionMode = null coroutineJobs.forEach { it.cancel() diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt index 16ad49371..495dfff68 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt @@ -146,7 +146,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { copy(searchText = "") } zimManageViewModel.onlineBooksSearchedQuery.value = null - zimManageViewModel.requestFiltering.onNext("") + zimManageViewModel.requestFiltering.tryEmit("") } private fun onSearchValueChanged(searchText: String) { @@ -159,7 +159,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onlineLibraryScreenState.value.update { copy(searchText = searchText) } - zimManageViewModel.requestFiltering.onNext(searchText) + zimManageViewModel.requestFiltering.tryEmit(searchText) } private val noWifiWithWifiOnlyPreferenceSet @@ -275,11 +275,11 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onlineLibraryScreenState.value.update { copy(isSearchActive = true, searchText = it) } - zimManageViewModel.requestFiltering.onNext(it) + zimManageViewModel.requestFiltering.tryEmit(it) } ?: run { // If no previously saved query found then normally initiate the search. zimManageViewModel.onlineBooksSearchedQuery.value = "" - zimManageViewModel.requestFiltering.onNext("") + zimManageViewModel.requestFiltering.tryEmit("") } } @@ -505,7 +505,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { if (isNotConnected) { showNoInternetConnectionError() } else { - zimManageViewModel.requestDownloadLibrary.onNext(Unit) + zimManageViewModel.requestDownloadLibrary.tryEmit(Unit) showRecyclerviewAndHideSwipeDownForLibraryErrorText() } } diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt index 98f54d19b..52efcfd88 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt @@ -24,31 +24,33 @@ import androidx.annotation.VisibleForTesting import androidx.lifecycle.MutableLiveData import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope -import io.reactivex.Flowable -import io.reactivex.disposables.CompositeDisposable -import io.reactivex.disposables.Disposable -import io.reactivex.exceptions.UndeliverableException -import io.reactivex.functions.BiFunction -import io.reactivex.functions.Function6 -import io.reactivex.plugins.RxJavaPlugins -import io.reactivex.processors.BehaviorProcessor -import io.reactivex.processors.PublishProcessor -import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.catch +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.retry import kotlinx.coroutines.launch -import kotlinx.coroutines.rx3.asFlowable +import kotlinx.coroutines.reactive.asFlow import kotlinx.coroutines.withContext import okhttp3.OkHttpClient import okhttp3.Request @@ -113,16 +115,13 @@ import org.kiwix.kiwixmobile.zimManager.libraryView.adapter.LibraryListItem import org.kiwix.kiwixmobile.zimManager.libraryView.adapter.LibraryListItem.BookItem import org.kiwix.kiwixmobile.zimManager.libraryView.adapter.LibraryListItem.DividerItem import org.kiwix.kiwixmobile.zimManager.libraryView.adapter.LibraryListItem.LibraryDownloadItem -import java.io.IOException import java.util.LinkedList import java.util.Locale -import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.TimeUnit.SECONDS import javax.inject.Inject const val DEFAULT_PROGRESS = 0 const val MAX_PROGRESS = 100 -private const val TAG_RX_JAVA_DEFAULT_ERROR_HANDLER = "RxJavaDefaultErrorHandler" class ZimManageViewModel @Inject constructor( private val downloadDao: DownloadRoomDao, @@ -161,18 +160,15 @@ class ZimManageViewModel @Inject constructor( val requestFileSystemCheck = MutableSharedFlow(replay = 0) val fileSelectActions = MutableSharedFlow() - val requestDownloadLibrary = BehaviorProcessor.createDefault(Unit) - val requestFiltering = BehaviorProcessor.createDefault("") + val requestDownloadLibrary = MutableStateFlow(Unit) + val requestFiltering = MutableStateFlow("") val onlineBooksSearchedQuery = MutableLiveData() - - private var compositeDisposable: CompositeDisposable? = CompositeDisposable() private val coroutineJobs: MutableList = mutableListOf() val downloadProgress = MutableLiveData() private lateinit var alertDialogShower: AlertDialogShower init { - compositeDisposable?.addAll(*disposables()) observeCoroutineFlows() context.registerReceiver(connectivityBroadcastReceiver) } @@ -259,10 +255,18 @@ class ZimManageViewModel @Inject constructor( } private fun observeCoroutineFlows(dispatcher: CoroutineDispatcher = Dispatchers.IO) { + val downloads = downloadDao.downloads() + val booksFromDao = books() + val networkLibrary = MutableSharedFlow(replay = 0) + val languages = languageDao.languages() coroutineJobs.apply { add(scanBooksFromStorage(dispatcher)) add(updateBookItems()) add(fileSelectActions()) + add(updateLibraryItems(booksFromDao, downloads, networkLibrary, languages)) + add(updateLanguagesInDao(networkLibrary, languages)) + add(updateNetworkStates()) + add(requestsAndConnectivityChangesToLibraryRequests(networkLibrary)) } } @@ -271,31 +275,12 @@ class ZimManageViewModel @Inject constructor( it.cancel() } coroutineJobs.clear() - compositeDisposable?.clear() context.unregisterReceiver(connectivityBroadcastReceiver) connectivityBroadcastReceiver.stopNetworkState() - requestDownloadLibrary.onComplete() - compositeDisposable = null appProgressListener = null super.onCleared() } - private fun disposables(): Array { - // temporary converting to flowable. TODO we will refactor this in upcoming issue. - val downloads = downloadDao.downloads().asFlowable() - val booksFromDao = books().asFlowable() - val networkLibrary = PublishProcessor.create() - val languages = languageDao.languages().asFlowable() - return arrayOf( - updateLibraryItems(booksFromDao, downloads, networkLibrary, languages), - updateLanguagesInDao(networkLibrary, languages), - updateNetworkStates(), - requestsAndConnectivtyChangesToLibraryRequests(networkLibrary) - ).also { - setUpUncaughtErrorHandlerForOnlineLibrary(networkLibrary) - } - } - private fun scanBooksFromStorage(dispatcher: CoroutineDispatcher = Dispatchers.IO) = viewModelScope.launch { withContext(dispatcher) { @@ -310,27 +295,25 @@ class ZimManageViewModel @Inject constructor( @Suppress("TooGenericExceptionCaught") private fun fileSelectActions() = - viewModelScope.launch { - fileSelectActions - .collect { action -> - try { - sideEffects.emit( - when (action) { - is RequestNavigateTo -> OpenFileWithNavigation(action.bookOnDisk) - is RequestMultiSelection -> startMultiSelectionAndSelectBook(action.bookOnDisk) - RequestDeleteMultiSelection -> DeleteFiles(selectionsFromState(), alertDialogShower) - RequestShareMultiSelection -> ShareFiles(selectionsFromState()) - MultiModeFinished -> noSideEffectAndClearSelectionState() - is RequestSelect -> noSideEffectSelectBook(action.bookOnDisk) - RestartActionMode -> StartMultiSelection(fileSelectActions) - UserClickedDownloadBooksButton -> NavigateToDownloads - } - ) - } catch (e: Throwable) { - e.printStackTrace() - } + fileSelectActions + .onEach { action -> + try { + sideEffects.emit( + when (action) { + is RequestNavigateTo -> OpenFileWithNavigation(action.bookOnDisk) + is RequestMultiSelection -> startMultiSelectionAndSelectBook(action.bookOnDisk) + RequestDeleteMultiSelection -> DeleteFiles(selectionsFromState(), alertDialogShower) + RequestShareMultiSelection -> ShareFiles(selectionsFromState()) + MultiModeFinished -> noSideEffectAndClearSelectionState() + is RequestSelect -> noSideEffectSelectBook(action.bookOnDisk) + RestartActionMode -> StartMultiSelection(fileSelectActions) + UserClickedDownloadBooksButton -> NavigateToDownloads + } + ) + } catch (e: Throwable) { + e.printStackTrace() } - } + }.launchIn(viewModelScope) private fun startMultiSelectionAndSelectBook( bookOnDisk: BookOnDisk @@ -385,117 +368,110 @@ class ZimManageViewModel @Inject constructor( return None } - @Suppress("NoNameShadowing") - private fun requestsAndConnectivtyChangesToLibraryRequests( - library: PublishProcessor, - ) = - Flowable.combineLatest( + @OptIn(ExperimentalCoroutinesApi::class) + private fun requestsAndConnectivityChangesToLibraryRequests( + library: MutableSharedFlow, + ) = viewModelScope.launch(Dispatchers.IO) { + combine( requestDownloadLibrary, - connectivityBroadcastReceiver.networkStates.distinctUntilChanged().filter( - CONNECTED::equals - ) + connectivityBroadcastReceiver.networkStates.asFlow() + .distinctUntilChanged() + .filter { it == CONNECTED } ) { _, _ -> } - .switchMap { - if (connectivityManager.isWifi()) { - Flowable.just(Unit) - } else { - sharedPreferenceUtil.prefWifiOnlys - .asFlowable() - .doOnNext { - if (it) { - shouldShowWifiOnlyDialog.postValue(true) - } - } - .filter { !it } - .map { } - } + .flatMapConcat { + shouldProceedWithDownload().map { createKiwixServiceWithProgressListener() } } - .subscribeOn(Schedulers.io()) - .observeOn(Schedulers.io()) - .concatMap { - Flowable.fromCallable { - synchronized(this, ::createKiwixServiceWithProgressListener) - } + .flatMapConcat { kiwixService -> + downloadLibraryFlow(kiwixService) } - .concatMap { - kiwixService.library - .toFlowable() - .retry(5) - .doOnSubscribe { - downloadProgress.postValue( - context.getString(R.string.starting_downloading_remote_library) - ) - } - .map { response -> - downloadProgress.postValue(context.getString(R.string.parsing_remote_library)) - response - } - .doFinally { - downloadProgress.postValue(context.getString(R.string.parsing_remote_library)) - } - .onErrorReturn { - it.printStackTrace() - LibraryNetworkEntity().apply { book = LinkedList() } - } - } - .subscribe(library::onNext, Throwable::printStackTrace).also { - compositeDisposable?.add(it) + .filterNotNull() + .catch { it.printStackTrace() } + .collect { + library.emit(it) } + } - private fun updateNetworkStates() = - connectivityBroadcastReceiver.networkStates.subscribe( - networkStates::postValue, - Throwable::printStackTrace - ) - - private fun updateLibraryItems( - booksFromDao: io.reactivex.rxjava3.core.Flowable>, - downloads: io.reactivex.rxjava3.core.Flowable>, - library: Flowable, - languages: io.reactivex.rxjava3.core.Flowable> - ) = Flowable.combineLatest( - booksFromDao, - downloads, - languages.filter(List::isNotEmpty), - library, - Flowable.merge( - Flowable.just(""), - requestFiltering - .doOnNext { libraryListIsRefreshing.postValue(true) } - .debounce(500, MILLISECONDS) - .observeOn(Schedulers.io()) - ), - fat32Checker.fileSystemStates.asFlowable(), - Function6(::combineLibrarySources) - ) - .doOnNext { libraryListIsRefreshing.postValue(false) } - .doOnError { throwable -> - if (throwable is OutOfMemoryError) { - Log.e("ZimManageViewModel", "Error----${throwable.printStackTrace()}") - } + private fun shouldProceedWithDownload(): Flow { + return if (connectivityManager.isWifi()) { + flowOf(Unit) + } else { + sharedPreferenceUtil.prefWifiOnlys + .filter { + if (it) shouldShowWifiOnlyDialog.postValue(true) + !it + } + .map { } } - .subscribeOn(Schedulers.io()) - .subscribe( - libraryItems::postValue, - Throwable::printStackTrace + } + + private fun downloadLibraryFlow( + kiwixService: KiwixService + ): Flow = flow { + downloadProgress.postValue(context.getString(R.string.starting_downloading_remote_library)) + val response = kiwixService.getLibrary() + downloadProgress.postValue(context.getString(R.string.parsing_remote_library)) + emit(response) + } + .retry(5) + .catch { e -> + e.printStackTrace() + emit(LibraryNetworkEntity().apply { book = LinkedList() }) + } + + private fun updateNetworkStates() = connectivityBroadcastReceiver.networkStates + .asFlow() + .catch { it.printStackTrace() } + .onEach { state -> + networkStates.postValue(state) + }.launchIn(viewModelScope) + + @OptIn(FlowPreview::class) + private fun updateLibraryItems( + booksFromDao: Flow>, + downloads: Flow>, + library: MutableSharedFlow, + languages: Flow> + ) = viewModelScope.launch(Dispatchers.IO) { + val requestFilteringFlow = merge( + flowOf(""), + requestFiltering + .onEach { libraryListIsRefreshing.postValue(true) } + .debounce(500) + .flowOn(Dispatchers.IO) ) + combine( + booksFromDao, + downloads, + languages.filter { it.isNotEmpty() }, + library, + requestFilteringFlow, + fat32Checker.fileSystemStates + ) { books, downloads, langs, libraryNet, filter, fsState -> + combineLibrarySources(books, downloads, langs, libraryNet, filter, fsState) + } + .onEach { libraryListIsRefreshing.postValue(false) } + .catch { throwable -> + throwable.printStackTrace() + Log.e("ZimManageViewModel", "Error----${throwable}") + } + .collect { libraryItems.postValue(it) } + } + private fun updateLanguagesInDao( - library: Flowable, - languages: io.reactivex.rxjava3.core.Flowable> - ) = library - .subscribeOn(Schedulers.io()) - .map(LibraryNetworkEntity::book) - .withLatestFrom( - languages, - BiFunction(::combineToLanguageList) - ) - .map { it.sortedBy(Language::language) } - .filter(List::isNotEmpty) - .subscribe( - languageDao::insert, - Throwable::printStackTrace - ) + library: MutableSharedFlow, + languages: Flow> + ) = viewModelScope.launch(Dispatchers.IO) { + combine( + library.map { it.book }.filterNotNull(), + languages + ) { books, existingLanguages -> + combineToLanguageList(books, existingLanguages) + }.map { it.sortedBy(Language::language) } + .filter { it.isNotEmpty() } + .catch { it.printStackTrace() } + .collect { languageDao.insert(it) } + } private fun combineToLanguageList( booksFromNetwork: List, @@ -678,18 +654,16 @@ class ZimManageViewModel @Inject constructor( ) = booksFromFileSystem.filterNot { idsInDao.contains(it.book.id) } private fun updateBookItems() = - viewModelScope.launch { - dataSource.booksOnDiskAsListItems() - .catch { it.printStackTrace() } - .collect { newList -> - val currentState = fileSelectListStates.value - val updatedState = currentState?.let { - inheritSelections(it, newList.toMutableList()) - } ?: FileSelectListState(newList) + dataSource.booksOnDiskAsListItems() + .catch { it.printStackTrace() } + .onEach { newList -> + val currentState = fileSelectListStates.value + val updatedState = currentState?.let { + inheritSelections(it, newList.toMutableList()) + } ?: FileSelectListState(newList) - fileSelectListStates.postValue(updatedState) - } - } + fileSelectListStates.postValue(updatedState) + }.launchIn(viewModelScope) private fun inheritSelections( oldState: FileSelectListState, @@ -706,35 +680,4 @@ class ZimManageViewModel @Inject constructor( } ) } - - private fun setUpUncaughtErrorHandlerForOnlineLibrary( - library: PublishProcessor - ) { - RxJavaPlugins.setErrorHandler { exception -> - if (exception is RuntimeException && exception.cause == IOException()) { - Log.i( - TAG_RX_JAVA_DEFAULT_ERROR_HANDLER, - "Caught undeliverable exception: ${exception.cause}" - ) - } - when (exception) { - is UndeliverableException -> { - library.onNext( - LibraryNetworkEntity().apply { book = LinkedList() } - ).also { - Log.i( - TAG_RX_JAVA_DEFAULT_ERROR_HANDLER, - "Caught undeliverable exception: ${exception.cause}" - ) - } - } - - else -> { - Thread.currentThread().also { thread -> - thread.uncaughtExceptionHandler?.uncaughtException(thread, exception) - } - } - } - } - } } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/data/DataSource.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/data/DataSource.kt index 21fc9f5bf..70cbcb98f 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/data/DataSource.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/data/DataSource.kt @@ -17,7 +17,6 @@ */ package org.kiwix.kiwixmobile.core.data -import io.reactivex.Completable import kotlinx.coroutines.flow.Flow import org.kiwix.kiwixmobile.core.dao.entities.WebViewHistoryEntity import org.kiwix.kiwixmobile.core.page.bookmark.adapter.LibkiwixBookmarkItem @@ -33,21 +32,21 @@ import org.kiwix.kiwixmobile.core.zim_manager.fileselect_view.BooksOnDiskListIte */ interface DataSource { fun getLanguageCategorizedBooks(): Flow> - fun saveBook(book: BookOnDisk): Completable - fun saveBooks(book: List): Completable - fun saveLanguages(languages: List): Completable - fun saveHistory(history: HistoryItem): Completable - fun deleteHistory(historyList: List): Completable - fun clearHistory(): Completable + suspend fun saveBook(book: BookOnDisk) + suspend fun saveBooks(book: List) + suspend fun saveLanguages(languages: List) + suspend fun saveHistory(history: HistoryItem) + suspend fun deleteHistory(historyList: List) + suspend fun clearHistory() fun getBookmarks(): Flow> suspend fun getCurrentZimBookmarksUrl(): List suspend fun saveBookmark(libkiwixBookmarkItem: LibkiwixBookmarkItem) suspend fun deleteBookmarks(bookmarks: List) suspend fun deleteBookmark(bookId: String, bookmarkUrl: String) fun booksOnDiskAsListItems(): Flow> - fun saveNote(noteListItem: NoteListItem): Completable - fun deleteNote(noteTitle: String): Completable - fun deleteNotes(noteList: List): Completable + suspend fun saveNote(noteListItem: NoteListItem) + suspend fun deleteNote(noteTitle: String) + suspend fun deleteNotes(noteList: List) suspend fun insertWebViewPageHistoryItems(webViewHistoryEntityList: List) fun getAllWebViewPagesHistory(): Flow> suspend fun clearWebViewPagesHistory() diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/data/Repository.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/data/Repository.kt index f22d5c76d..5517a601a 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/data/Repository.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/data/Repository.kt @@ -18,12 +18,11 @@ package org.kiwix.kiwixmobile.core.data -import io.reactivex.Completable -import io.reactivex.Scheduler import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map +import kotlinx.coroutines.withContext import org.kiwix.kiwixmobile.core.dao.HistoryRoomDao import org.kiwix.kiwixmobile.core.dao.LibkiwixBookmarks import org.kiwix.kiwixmobile.core.dao.NewBookDao @@ -32,7 +31,6 @@ import org.kiwix.kiwixmobile.core.dao.NotesRoomDao import org.kiwix.kiwixmobile.core.dao.RecentSearchRoomDao import org.kiwix.kiwixmobile.core.dao.WebViewHistoryRoomDao import org.kiwix.kiwixmobile.core.dao.entities.WebViewHistoryEntity -import org.kiwix.kiwixmobile.core.di.qualifiers.IO import org.kiwix.kiwixmobile.core.extensions.HeaderizableList import org.kiwix.kiwixmobile.core.page.bookmark.adapter.LibkiwixBookmarkItem import org.kiwix.kiwixmobile.core.page.history.adapter.HistoryListItem @@ -52,7 +50,6 @@ import javax.inject.Singleton @Singleton class Repository @Inject internal constructor( - @param:IO private val ioThread: Scheduler, private val bookDao: NewBookDao, private val libkiwixBookmarks: LibkiwixBookmarks, private val historyRoomDao: HistoryRoomDao, @@ -91,33 +88,38 @@ class Repository @Inject internal constructor( .map(MutableList::toList) .flowOn(Dispatchers.IO) - override fun saveBooks(books: List) = - Completable.fromAction { bookDao.insert(books) } - .subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun saveBooks(books: List) = withContext(Dispatchers.IO) { + bookDao.insert(books) + } - override fun saveBook(book: BookOnDisk) = - Completable.fromAction { bookDao.insert(listOf(book)) } - .subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun saveBook(book: BookOnDisk) = withContext(Dispatchers.IO) { + bookDao.insert(listOf(book)) + } - override fun saveLanguages(languages: List) = - Completable.fromAction { languageDao.insert(languages) } - .subscribeOn(ioThread) - - override fun saveHistory(history: HistoryItem) = - Completable.fromAction { historyRoomDao.saveHistory(history) } - .subscribeOn(ioThread) - - override fun deleteHistory(historyList: List) = - Completable.fromAction { - historyRoomDao.deleteHistory(historyList.filterIsInstance(HistoryItem::class.java)) + @Suppress("InjectDispatcher") + override suspend fun saveLanguages(languages: List) = + withContext(Dispatchers.IO) { + languageDao.insert(languages) } - .subscribeOn(ioThread) - override fun clearHistory() = - Completable.fromAction { - historyRoomDao.deleteAllHistory() - recentSearchRoomDao.deleteSearchHistory() - }.subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun saveHistory(history: HistoryItem) = withContext(Dispatchers.IO) { + historyRoomDao.saveHistory(history) + } + + @Suppress("InjectDispatcher") + override suspend fun deleteHistory(historyList: List) = + withContext(Dispatchers.IO) { + historyRoomDao.deleteHistory(historyList.filterIsInstance()) + } + + @Suppress("InjectDispatcher") + override suspend fun clearHistory() = withContext(Dispatchers.IO) { + historyRoomDao.deleteAllHistory() + recentSearchRoomDao.deleteSearchHistory() + } override fun getBookmarks() = libkiwixBookmarks.bookmarks() as Flow> @@ -134,13 +136,17 @@ class Repository @Inject internal constructor( override suspend fun deleteBookmark(bookId: String, bookmarkUrl: String) = libkiwixBookmarks.deleteBookmark(bookId, bookmarkUrl) - override fun saveNote(noteListItem: NoteListItem): Completable = - Completable.fromAction { notesRoomDao.saveNote(noteListItem) } - .subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun saveNote(noteListItem: NoteListItem) = + withContext(Dispatchers.IO) { + notesRoomDao.saveNote(noteListItem) + } - override fun deleteNotes(noteList: List) = - Completable.fromAction { notesRoomDao.deleteNotes(noteList) } - .subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun deleteNotes(noteList: List) = + withContext(Dispatchers.IO) { + notesRoomDao.deleteNotes(noteList) + } override suspend fun insertWebViewPageHistoryItems( webViewHistoryEntityList: List @@ -155,7 +161,8 @@ class Repository @Inject internal constructor( webViewHistoryRoomDao.clearWebViewPagesHistory() } - override fun deleteNote(noteTitle: String): Completable = - Completable.fromAction { notesRoomDao.deleteNote(noteTitle) } - .subscribeOn(ioThread) + @Suppress("InjectDispatcher") + override suspend fun deleteNote(noteTitle: String) = withContext(Dispatchers.IO) { + notesRoomDao.deleteNote(noteTitle) + } } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/KiwixService.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/KiwixService.kt index 0e127afe9..ee6dcf180 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/KiwixService.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/KiwixService.kt @@ -19,25 +19,22 @@ package org.kiwix.kiwixmobile.core.data.remote -import io.reactivex.Observable -import io.reactivex.Single -import io.reactivex.schedulers.Schedulers import okhttp3.OkHttpClient import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity import org.kiwix.kiwixmobile.core.entity.MetaLinkNetworkEntity import retrofit2.Retrofit -import retrofit2.adapter.rxjava2.RxJava2CallAdapterFactory import retrofit2.converter.simplexml.SimpleXmlConverterFactory import retrofit2.http.GET import retrofit2.http.Url interface KiwixService { - @get:GET(LIBRARY_NETWORK_PATH) val library: Single + @GET(LIBRARY_NETWORK_PATH) + suspend fun getLibrary(): LibraryNetworkEntity? @GET - fun getMetaLinks( + suspend fun getMetaLinks( @Url url: String - ): Observable + ): MetaLinkNetworkEntity? /******** Helper class that sets up new services */ object ServiceCreator { @@ -47,7 +44,6 @@ interface KiwixService { .baseUrl(baseUrl) .client(okHttpClient) .addConverterFactory(SimpleXmlConverterFactory.create()) - .addCallAdapterFactory(RxJava2CallAdapterFactory.createWithScheduler(Schedulers.io())) .build() return retrofit.create(KiwixService::class.java) } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/di/modules/ApplicationModule.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/di/modules/ApplicationModule.kt index cc72e7017..719a143cc 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/di/modules/ApplicationModule.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/di/modules/ApplicationModule.kt @@ -24,15 +24,9 @@ import android.net.ConnectivityManager import android.os.storage.StorageManager import dagger.Module import dagger.Provides -import io.reactivex.Scheduler -import io.reactivex.android.schedulers.AndroidSchedulers -import io.reactivex.schedulers.Schedulers import org.kiwix.kiwixmobile.core.DarkModeConfig import org.kiwix.kiwixmobile.core.data.remote.ObjectBoxToLibkiwixMigrator import org.kiwix.kiwixmobile.core.data.remote.ObjectBoxToRoomMigrator -import org.kiwix.kiwixmobile.core.di.qualifiers.Computation -import org.kiwix.kiwixmobile.core.di.qualifiers.IO -import org.kiwix.kiwixmobile.core.di.qualifiers.MainThread import org.kiwix.kiwixmobile.core.downloader.DownloadMonitor import org.kiwix.kiwixmobile.core.downloader.downloadManager.DownloadManagerMonitor import org.kiwix.kiwixmobile.core.reader.ZimFileReader @@ -68,18 +62,6 @@ class ApplicationModule { @Singleton fun provideObjectBoxToRoomMigrator() = ObjectBoxToRoomMigrator() - @IO - @Provides - fun provideIoThread(): Scheduler = Schedulers.io() - - @MainThread - @Provides - fun provideMainThread(): Scheduler = AndroidSchedulers.mainThread() - - @Computation - @Provides - fun provideComputationThread(): Scheduler = Schedulers.computation() - @Provides @Singleton internal fun provideDownloadMonitor( diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt index e5e24a84a..c779e7742 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt @@ -18,8 +18,9 @@ package org.kiwix.kiwixmobile.core.downloader -import io.reactivex.Observable -import io.reactivex.schedulers.Schedulers +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch import org.kiwix.kiwixmobile.core.dao.DownloadRoomDao import org.kiwix.kiwixmobile.core.data.remote.KiwixService import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity @@ -31,26 +32,24 @@ class DownloaderImpl @Inject constructor( private val downloadRoomDao: DownloadRoomDao, private val kiwixService: KiwixService ) : Downloader { - @Suppress("CheckResult", "IgnoredReturnValue") + @Suppress("InjectDispatcher") override fun download(book: LibraryNetworkEntity.Book) { - urlProvider(book) - .take(1) - .subscribeOn(Schedulers.io()) - .subscribe( - { + CoroutineScope(Dispatchers.IO).launch { + runCatching { + urlProvider(book)?.let { downloadRoomDao.addIfDoesNotExist(it, book, downloadRequester) - }, - Throwable::printStackTrace - ) + } + }.onFailure { + it.printStackTrace() + } + } } - @Suppress("UnsafeCallOnNullableType") - private fun urlProvider(book: Book): Observable = + private suspend fun urlProvider(book: Book): String? = if (book.url?.endsWith("meta4") == true) { - kiwixService.getMetaLinks(book.url!!) - .map { it.relevantUrl.value } + kiwixService.getMetaLinks(book.url!!)?.relevantUrl?.value } else { - Observable.just(book.url) + book.url } override fun cancelDownload(downloadId: Long) { diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/main/AddNoteDialog.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/main/AddNoteDialog.kt index 81f49ac6a..df3a5e819 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/main/AddNoteDialog.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/main/AddNoteDialog.kt @@ -41,6 +41,7 @@ import androidx.core.content.ContextCompat import androidx.core.content.FileProvider import androidx.fragment.app.DialogFragment import androidx.lifecycle.lifecycleScope +import kotlinx.coroutines.launch import org.kiwix.kiwixmobile.core.CoreApp.Companion.coreComponent import org.kiwix.kiwixmobile.core.CoreApp.Companion.instance import org.kiwix.kiwixmobile.core.R @@ -73,9 +74,6 @@ import javax.inject.Inject * Notes are saved as text files at location: "{External Storage}/Kiwix/Notes/ZimFileName/ArticleUrl.txt" */ -const val DISABLE_ICON_ITEM_ALPHA = 130 -const val ENABLE_ICON_ITEM_ALPHA = 255 - class AddNoteDialog : DialogFragment() { private lateinit var zimId: String private var zimFileName: String? = null @@ -401,41 +399,45 @@ class AddNoteDialog : DialogFragment() { } private fun addNoteToDao(noteFilePath: String?, title: String) { - noteFilePath?.let { filePath -> - if (filePath.isNotEmpty() && zimFileUrl.isNotEmpty()) { - val noteToSave = NoteListItem( - zimId = zimId, - title = title, - url = zimFileUrl, - noteFilePath = noteFilePath, - zimReaderSource = zimReaderSource, - favicon = favicon, - ) - mainRepositoryActions.saveNote(noteToSave) - } else { - Log.d(TAG, "Cannot process with empty zim url or noteFilePath") + lifecycleScope.launch { + noteFilePath?.let { filePath -> + if (filePath.isNotEmpty() && zimFileUrl.isNotEmpty()) { + val noteToSave = NoteListItem( + zimId = zimId, + title = title, + url = zimFileUrl, + noteFilePath = noteFilePath, + zimReaderSource = zimReaderSource, + favicon = favicon, + ) + mainRepositoryActions.saveNote(noteToSave) + } else { + Log.d(TAG, "Cannot process with empty zim url or noteFilePath") + } } } } private fun deleteNote() { - val notesFolder = File(zimNotesDirectory) - val noteFile = - File(notesFolder.absolutePath, "$articleNoteFileName.txt") - val noteDeleted = noteFile.delete() - val editedNoteText = noteText.value.text - if (noteDeleted) { - noteText.value = TextFieldValue("") - mainRepositoryActions.deleteNote(getNoteTitle()) - disableMenuItems() - snackBarHostState.snack( - message = requireActivity().getString(R.string.note_delete_successful), - actionLabel = requireActivity().getString(R.string.undo), - actionClick = { restoreDeletedNote(editedNoteText) }, - lifecycleScope = lifecycleScope - ) - } else { - context.toast(R.string.note_delete_unsuccessful, Toast.LENGTH_LONG) + lifecycleScope.launch { + val notesFolder = File(zimNotesDirectory) + val noteFile = + File(notesFolder.absolutePath, "$articleNoteFileName.txt") + val noteDeleted = noteFile.delete() + val editedNoteText = noteText.value.text + if (noteDeleted) { + noteText.value = TextFieldValue("") + mainRepositoryActions.deleteNote(getNoteTitle()) + disableMenuItems() + snackBarHostState.snack( + message = requireActivity().getString(R.string.note_delete_successful), + actionLabel = requireActivity().getString(R.string.undo), + actionClick = { restoreDeletedNote(editedNoteText) }, + lifecycleScope = lifecycleScope + ) + } else { + context.toast(R.string.note_delete_unsuccessful, Toast.LENGTH_LONG) + } } } @@ -514,7 +516,6 @@ class AddNoteDialog : DialogFragment() { override fun onDestroyView() { super.onDestroyView() - mainRepositoryActions.dispose() onBackPressedCallBack.remove() } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt index 4a34fb720..c19977ec8 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreReaderFragment.kt @@ -1268,7 +1268,6 @@ abstract class CoreReaderFragment : if (sharedPreferenceUtil?.showIntro() == true) { (requireActivity() as? AppCompatActivity)?.setSupportActionBar(null) } - repositoryActions?.dispose() safelyCancelBookmarkJob() unBindViewsAndBinding() tabCallback = null @@ -2658,7 +2657,9 @@ abstract class CoreReaderFragment : timeStamp, zimFileReader!! ) - repositoryActions?.saveHistory(history) + lifecycleScope.launch { + repositoryActions?.saveHistory(history) + } } } updateBottomToolbarVisibility() diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/main/MainRepositoryActions.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/main/MainRepositoryActions.kt index 0f4104ac4..d351ea981 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/main/MainRepositoryActions.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/main/MainRepositoryActions.kt @@ -17,9 +17,8 @@ */ package org.kiwix.kiwixmobile.core.main -import io.reactivex.disposables.Disposable -import kotlinx.coroutines.flow.first import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext import org.kiwix.kiwixmobile.core.dao.entities.WebViewHistoryEntity import org.kiwix.kiwixmobile.core.data.DataSource @@ -36,57 +35,61 @@ private const val TAG = "MainPresenter" @ActivityScope class MainRepositoryActions @Inject constructor(private val dataSource: DataSource) { - private var saveHistoryDisposable: Disposable? = null - private var saveNoteDisposable: Disposable? = null - private var saveBookDisposable: Disposable? = null - private var deleteNoteDisposable: Disposable? = null - private var saveWebViewHistoryDisposable: Disposable? = null - private var clearWebViewHistoryDisposable: Disposable? = null - - fun saveHistory(history: HistoryItem) { - saveHistoryDisposable = - dataSource.saveHistory(history) - .subscribe({}, { e -> Log.e(TAG, "Unable to save history", e) }) + @Suppress("InjectDispatcher") + suspend fun saveHistory(history: HistoryItem) { + runCatching { + withContext(Dispatchers.IO) { + dataSource.saveHistory(history) + } + }.onFailure { + Log.e(TAG, "Unable to save history", it) + } } - @Suppress("InjectDispatcher", "TooGenericExceptionCaught") + @Suppress("InjectDispatcher") suspend fun saveBookmark(bookmark: LibkiwixBookmarkItem) { - withContext(Dispatchers.IO) { - try { + runCatching { + withContext(Dispatchers.IO) { dataSource.saveBookmark(bookmark) - } catch (e: Exception) { - Log.e(TAG, "Unable to save bookmark", e) } + }.onFailure { + Log.e(TAG, "Unable to save bookmark", it) } } - @Suppress("InjectDispatcher", "TooGenericExceptionCaught") + @Suppress("InjectDispatcher") suspend fun deleteBookmark(bookId: String, bookmarkUrl: String) { - withContext(Dispatchers.IO) { - try { + runCatching { + withContext(Dispatchers.IO) { dataSource.deleteBookmark(bookId, bookmarkUrl) - } catch (e: Exception) { - Log.e(TAG, "Unable to delete bookmark", e) } + }.onFailure { + Log.e(TAG, "Unable to delete bookmark", it) } } - fun saveNote(note: NoteListItem) { - saveNoteDisposable = + suspend fun saveNote(note: NoteListItem) { + runCatching { dataSource.saveNote(note) - .subscribe({}, { e -> Log.e(TAG, "Unable to save note", e) }) + }.onFailure { + Log.e(TAG, "Unable to save note", it) + } } - fun deleteNote(noteTitle: String) { - deleteNoteDisposable = + suspend fun deleteNote(noteTitle: String) { + runCatching { dataSource.deleteNote(noteTitle) - .subscribe({}, { e -> Log.e(TAG, "Unable to delete note", e) }) + }.onFailure { + Log.e(TAG, "Unable to delete note", it) + } } - fun saveBook(book: BookOnDisk) { - saveBookDisposable = + suspend fun saveBook(book: BookOnDisk) { + runCatching { dataSource.saveBook(book) - .subscribe({}, { e -> Log.e(TAG, "Unable to save book", e) }) + }.onFailure { + Log.e(TAG, "Unable to save book", it) + } } suspend fun saveWebViewPageHistory(webViewHistoryEntityList: List) { @@ -101,13 +104,4 @@ class MainRepositoryActions @Inject constructor(private val dataSource: DataSour dataSource.getAllWebViewPagesHistory() .first() .map(::WebViewHistoryItem) - - fun dispose() { - saveHistoryDisposable?.dispose() - saveNoteDisposable?.dispose() - deleteNoteDisposable?.dispose() - saveBookDisposable?.dispose() - saveWebViewHistoryDisposable?.dispose() - clearWebViewHistoryDisposable?.dispose() - } } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/CorePrefsFragment.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/CorePrefsFragment.kt index 2b15dd3ec..2567c2432 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/CorePrefsFragment.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/CorePrefsFragment.kt @@ -164,7 +164,6 @@ abstract class CorePrefsFragment : } override fun onDestroyView() { - presenter?.dispose() storagePermissionForNotesLauncher?.unregister() storagePermissionForNotesLauncher = null super.onDestroyView() @@ -262,8 +261,10 @@ abstract class CorePrefsFragment : private fun clearAllHistoryDialog() { alertDialogShower?.show(KiwixDialog.ClearAllHistory, { - presenter?.clearHistory() - Snackbar.make(requireView(), R.string.all_history_cleared, Snackbar.LENGTH_SHORT).show() + lifecycleScope.launch { + presenter?.clearHistory() + Snackbar.make(requireView(), R.string.all_history_cleared, Snackbar.LENGTH_SHORT).show() + } }) } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsContract.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsContract.kt index 5f4221752..fa45891f0 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsContract.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsContract.kt @@ -22,6 +22,6 @@ import org.kiwix.kiwixmobile.core.base.BaseContract interface SettingsContract { interface View : BaseContract.View interface Presenter : BaseContract.Presenter { - fun clearHistory() + suspend fun clearHistory() } } diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt index 89199204c..7f56a7633 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt @@ -17,28 +17,21 @@ */ package org.kiwix.kiwixmobile.core.settings -import org.kiwix.kiwixmobile.core.utils.files.Log -import io.reactivex.disposables.Disposable import org.kiwix.kiwixmobile.core.base.BasePresenter import org.kiwix.kiwixmobile.core.data.DataSource import org.kiwix.kiwixmobile.core.settings.SettingsContract.Presenter import org.kiwix.kiwixmobile.core.settings.SettingsContract.View +import org.kiwix.kiwixmobile.core.utils.files.Log import javax.inject.Inject internal class SettingsPresenter @Inject constructor(private val dataSource: DataSource) : BasePresenter(), Presenter { - private var dataSourceDisposable: Disposable? = null - override fun clearHistory() { - dataSourceDisposable = - dataSource.clearHistory() - .subscribe({ - // TODO - }, { e -> - Log.e("SettingsPresenter", e.message, e) - }) - } - fun dispose() { - dataSourceDisposable?.dispose() + override suspend fun clearHistory() { + runCatching { + dataSource.clearHistory() + }.onFailure { + Log.e("SettingsPresenter", it.message, it) } } +} diff --git a/core/src/test/java/org/kiwix/kiwixmobile/core/StorageObserverTest.kt b/core/src/test/java/org/kiwix/kiwixmobile/core/StorageObserverTest.kt index 6560c8cc9..67d240d71 100644 --- a/core/src/test/java/org/kiwix/kiwixmobile/core/StorageObserverTest.kt +++ b/core/src/test/java/org/kiwix/kiwixmobile/core/StorageObserverTest.kt @@ -22,13 +22,11 @@ import io.mockk.clearAllMocks import io.mockk.every import io.mockk.mockk import io.mockk.verify -import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import org.assertj.core.api.Assertions.assertThat -import org.junit.jupiter.api.AfterAll import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.kiwix.kiwixmobile.core.dao.DownloadRoomDao @@ -44,8 +42,6 @@ import org.kiwix.kiwixmobile.core.utils.files.testFlow import org.kiwix.kiwixmobile.core.zim_manager.fileselect_view.BooksOnDiskListItem.BookOnDisk import org.kiwix.sharedFunctions.book import org.kiwix.sharedFunctions.bookOnDisk -import org.kiwix.sharedFunctions.resetSchedulers -import org.kiwix.sharedFunctions.setScheduler import java.io.File class StorageObserverTest { @@ -65,15 +61,6 @@ class StorageObserverTest { private lateinit var storageObserver: StorageObserver - init { - setScheduler(Schedulers.trampoline()) - } - - @AfterAll - fun teardown() { - resetSchedulers() - } - @BeforeEach fun init() { clearAllMocks() every { sharedPreferenceUtil.prefStorage } returns "a" diff --git a/custom/src/main/java/org/kiwix/kiwixmobile/custom/download/CustomDownloadFragment.kt b/custom/src/main/java/org/kiwix/kiwixmobile/custom/download/CustomDownloadFragment.kt index 01b831e75..908e36783 100644 --- a/custom/src/main/java/org/kiwix/kiwixmobile/custom/download/CustomDownloadFragment.kt +++ b/custom/src/main/java/org/kiwix/kiwixmobile/custom/download/CustomDownloadFragment.kt @@ -27,7 +27,6 @@ import android.view.ViewGroup import androidx.core.app.ActivityCompat import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.lifecycleScope -import io.reactivex.disposables.CompositeDisposable import kotlinx.coroutines.launch import org.kiwix.kiwixmobile.core.R import org.kiwix.kiwixmobile.core.base.BaseActivity @@ -71,8 +70,6 @@ class CustomDownloadFragment : BaseFragment(), FragmentActivityExtensions { @Inject lateinit var viewModelFactory: ViewModelProvider.Factory private var fragmentCustomDownloadBinding: FragmentCustomDownloadBinding? = null - - private val compositeDisposable = CompositeDisposable() override fun inject(baseActivity: BaseActivity) { baseActivity.customActivityComponent.inject(this) } @@ -144,7 +141,6 @@ class CustomDownloadFragment : BaseFragment(), FragmentActivityExtensions { override fun onDestroy() { super.onDestroy() - compositeDisposable.clear() activity?.finish() } diff --git a/custom/src/test/java/org/kiwix/kiwixmobile/custom/download/effects/DownloadCustomTest.kt b/custom/src/test/java/org/kiwix/kiwixmobile/custom/download/effects/DownloadCustomTest.kt index 838ef58ff..bd22dadb1 100644 --- a/custom/src/test/java/org/kiwix/kiwixmobile/custom/download/effects/DownloadCustomTest.kt +++ b/custom/src/test/java/org/kiwix/kiwixmobile/custom/download/effects/DownloadCustomTest.kt @@ -18,8 +18,8 @@ package org.kiwix.kiwixmobile.custom.download.effects +import io.mockk.coVerify import io.mockk.mockk -import io.mockk.verify import org.junit.jupiter.api.Test import org.kiwix.kiwixmobile.core.downloader.Downloader import org.kiwix.sharedFunctions.book @@ -29,7 +29,7 @@ internal class DownloadCustomTest { fun `invokeWith queues download with ZimUrl`() { val downloader = mockk() DownloadCustom(downloader).invokeWith(mockk()) - verify { + coVerify { downloader.download(expectedBook()) } } From f5d55d2c8a4ed030f1679eed6e8ffe67cf3c4ee8 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Tue, 20 May 2025 17:05:09 +0530 Subject: [PATCH 2/4] Fixed compilation and lint issues. * Fixed: Selected language filter was not being applied to the online library. * Fixed: Unnecessary UI recomposition. * Improved code readability and resolved related lint errors. * Fixed: "Allow downloading content via mobile network?" dialog was not displaying correctly. * Fixed: SwipeRefreshing was not working when the "Swipe Down for Library" message was shown. * Improved: Previously, the online library would re-download every time the fragment was launched or brought to the foreground (e.g., when navigating back from another screen). This behavior has been changed to use coroutines. The library now downloads only once initially, and subsequent downloads only occur when the user manually refreshes the library. This helps reduce unnecessary data usage and network calls. --- .../viewmodel/SaveLanguagesAndFinish.kt | 8 +- .../library/online/OnlineLibraryFragment.kt | 12 ++- .../library/online/OnlineLibraryScreen.kt | 23 ++-- .../zimManager/ZimManageViewModel.kt | 102 ++++++++++++------ core/detekt_baseline.xml | 2 +- .../core/downloader/DownloaderImpl.kt | 1 + .../core/settings/SettingsPresenter.kt | 13 ++- 7 files changed, 108 insertions(+), 53 deletions(-) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/language/viewmodel/SaveLanguagesAndFinish.kt b/app/src/main/java/org/kiwix/kiwixmobile/language/viewmodel/SaveLanguagesAndFinish.kt index efe16628d..9b5bc48a6 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/language/viewmodel/SaveLanguagesAndFinish.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/language/viewmodel/SaveLanguagesAndFinish.kt @@ -26,7 +26,7 @@ import org.kiwix.kiwixmobile.core.base.SideEffect import org.kiwix.kiwixmobile.core.dao.NewLanguagesDao import org.kiwix.kiwixmobile.core.zim_manager.Language -@Suppress("InjectDispatcher", "TooGenericExceptionCaught") +@Suppress("InjectDispatcher") data class SaveLanguagesAndFinish( private val languages: List, private val languageDao: NewLanguagesDao, @@ -34,13 +34,13 @@ data class SaveLanguagesAndFinish( ) : SideEffect { override fun invokeWith(activity: AppCompatActivity) { lifecycleScope.launch { - try { + runCatching { withContext(Dispatchers.IO) { languageDao.insert(languages) } activity.onBackPressedDispatcher.onBackPressed() - } catch (e: Throwable) { - e.printStackTrace() + }.onFailure { + it.printStackTrace() } } } diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt index 495dfff68..827378005 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt @@ -341,6 +341,10 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { showRecyclerviewAndHideSwipeDownForLibraryErrorText() sharedPreferenceUtil.putPrefWifiOnly(false) zimManageViewModel.shouldShowWifiOnlyDialog.value = false + // User allowed downloading over mobile data. + // Since the download flow now triggers only when appropriate, + // we start the library download explicitly after updating the preference. + startDownloadingLibrary() }, { context.toast( @@ -505,11 +509,17 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { if (isNotConnected) { showNoInternetConnectionError() } else { - zimManageViewModel.requestDownloadLibrary.tryEmit(Unit) + startDownloadingLibrary() showRecyclerviewAndHideSwipeDownForLibraryErrorText() } } + private fun startDownloadingLibrary() { + lifecycleScope.launch { + zimManageViewModel.requestDownloadLibrary.emit(Unit) + } + } + private fun downloadFile() { downloadBookItem?.book?.let { downloader.download(it) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt index fe3822264..6062eaef7 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt @@ -19,6 +19,7 @@ package org.kiwix.kiwixmobile.nav.destination.library.online import androidx.compose.foundation.isSystemInDarkTheme +import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.calculateEndPadding @@ -31,6 +32,8 @@ import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.LazyListState import androidx.compose.foundation.lazy.items +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll import androidx.compose.material3.Card import androidx.compose.material3.CardDefaults import androidx.compose.material3.ExperimentalMaterial3Api @@ -212,13 +215,21 @@ private fun ShowDividerItem(dividerItem: DividerItem) { @Composable private fun NoContentView(noContentMessage: String) { - Text( - text = noContentMessage, - textAlign = TextAlign.Center, + Column( modifier = Modifier - .padding(horizontal = FOUR_DP) - .semantics { testTag = NO_CONTENT_VIEW_TEXT_TESTING_TAG } - ) + .fillMaxSize() + .verticalScroll(rememberScrollState()), + verticalArrangement = Arrangement.Center, + horizontalAlignment = Alignment.CenterHorizontally + ) { + Text( + text = noContentMessage, + textAlign = TextAlign.Center, + modifier = Modifier + .padding(horizontal = FOUR_DP) + .semantics { testTag = NO_CONTENT_VIEW_TEXT_TESTING_TAG } + ) + } } @Composable diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt index 52efcfd88..ae4c182f4 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt @@ -29,6 +29,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.Job +import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow @@ -49,6 +50,7 @@ import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.retry +import kotlinx.coroutines.flow.take import kotlinx.coroutines.launch import kotlinx.coroutines.reactive.asFlow import kotlinx.coroutines.withContext @@ -77,11 +79,15 @@ import org.kiwix.kiwixmobile.core.di.modules.KIWIX_DOWNLOAD_URL import org.kiwix.kiwixmobile.core.di.modules.READ_TIMEOUT import org.kiwix.kiwixmobile.core.di.modules.USER_AGENT import org.kiwix.kiwixmobile.core.downloader.downloadManager.DEFAULT_INT_VALUE +import org.kiwix.kiwixmobile.core.downloader.downloadManager.FIVE +import org.kiwix.kiwixmobile.core.downloader.downloadManager.ZERO import org.kiwix.kiwixmobile.core.downloader.model.DownloadModel import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity import org.kiwix.kiwixmobile.core.entity.LibraryNetworkEntity.Book import org.kiwix.kiwixmobile.core.extensions.calculateSearchMatches import org.kiwix.kiwixmobile.core.extensions.registerReceiver +import org.kiwix.kiwixmobile.core.ui.components.ONE +import org.kiwix.kiwixmobile.core.ui.components.TWO import org.kiwix.kiwixmobile.core.utils.BookUtils import org.kiwix.kiwixmobile.core.utils.SharedPreferenceUtil import org.kiwix.kiwixmobile.core.utils.dialog.AlertDialogShower @@ -123,6 +129,9 @@ import javax.inject.Inject const val DEFAULT_PROGRESS = 0 const val MAX_PROGRESS = 100 +const val THREE = 3 +const val FOUR = 4 + class ZimManageViewModel @Inject constructor( private val downloadDao: DownloadRoomDao, private val bookDao: NewBookDao, @@ -160,7 +169,11 @@ class ZimManageViewModel @Inject constructor( val requestFileSystemCheck = MutableSharedFlow(replay = 0) val fileSelectActions = MutableSharedFlow() - val requestDownloadLibrary = MutableStateFlow(Unit) + val requestDownloadLibrary = MutableSharedFlow( + replay = 0, + extraBufferCapacity = 1, + onBufferOverflow = BufferOverflow.DROP_OLDEST + ) val requestFiltering = MutableStateFlow("") val onlineBooksSearchedQuery = MutableLiveData() private val coroutineJobs: MutableList = mutableListOf() @@ -171,6 +184,13 @@ class ZimManageViewModel @Inject constructor( init { observeCoroutineFlows() context.registerReceiver(connectivityBroadcastReceiver) + viewModelScope.launch { + // Emit a request to download the library once. + // This prevents triggering the download multiple times, + // which was a problem in the RxJava version where the download + // would be retriggered every time the fragment was recreated. + requestDownloadLibrary.emit(Unit) + } } fun setIsUnitTestCase() { @@ -293,11 +313,10 @@ class ZimManageViewModel @Inject constructor( } } - @Suppress("TooGenericExceptionCaught") private fun fileSelectActions() = fileSelectActions .onEach { action -> - try { + runCatching { sideEffects.emit( when (action) { is RequestNavigateTo -> OpenFileWithNavigation(action.bookOnDisk) @@ -310,8 +329,8 @@ class ZimManageViewModel @Inject constructor( UserClickedDownloadBooksButton -> NavigateToDownloads } ) - } catch (e: Throwable) { - e.printStackTrace() + }.onFailure { + it.printStackTrace() } }.launchIn(viewModelScope) @@ -371,36 +390,36 @@ class ZimManageViewModel @Inject constructor( @OptIn(ExperimentalCoroutinesApi::class) private fun requestsAndConnectivityChangesToLibraryRequests( library: MutableSharedFlow, - ) = viewModelScope.launch(Dispatchers.IO) { - combine( - requestDownloadLibrary, - connectivityBroadcastReceiver.networkStates.asFlow() - .distinctUntilChanged() - .filter { it == CONNECTED } - ) { _, _ -> } + ) = requestDownloadLibrary.flatMapConcat { + connectivityBroadcastReceiver.networkStates.asFlow() + .distinctUntilChanged() + .filter { networkState -> networkState == CONNECTED } + .take(1) .flatMapConcat { - shouldProceedWithDownload().map { createKiwixServiceWithProgressListener() } - } - .flatMapConcat { kiwixService -> - downloadLibraryFlow(kiwixService) - } - .filterNotNull() - .catch { it.printStackTrace() } - .collect { - library.emit(it) + shouldProceedWithDownload() + .flatMapConcat { kiwixService -> + downloadLibraryFlow(kiwixService) + } } } + .filterNotNull() + .catch { it.printStackTrace() } + .onEach { library.emit(it) } + .launchIn(viewModelScope) - private fun shouldProceedWithDownload(): Flow { + private fun shouldProceedWithDownload(): Flow { return if (connectivityManager.isWifi()) { - flowOf(Unit) + flowOf(createKiwixServiceWithProgressListener()) } else { - sharedPreferenceUtil.prefWifiOnlys - .filter { - if (it) shouldShowWifiOnlyDialog.postValue(true) - !it + flow { + val wifiOnly = sharedPreferenceUtil.prefWifiOnlys.first() + if (wifiOnly) { + shouldShowWifiOnlyDialog.postValue(true) + // Don't emit anything — just return + return@flow } - .map { } + emit(createKiwixServiceWithProgressListener()) + } } } @@ -425,6 +444,7 @@ class ZimManageViewModel @Inject constructor( networkStates.postValue(state) }.launchIn(viewModelScope) + @Suppress("UNCHECKED_CAST", "InjectDispatcher") @OptIn(FlowPreview::class) private fun updateLibraryItems( booksFromDao: Flow>, @@ -447,13 +467,26 @@ class ZimManageViewModel @Inject constructor( library, requestFilteringFlow, fat32Checker.fileSystemStates - ) { books, downloads, langs, libraryNet, filter, fsState -> - combineLibrarySources(books, downloads, langs, libraryNet, filter, fsState) + ) { args -> + val books = args[ZERO] as List + val activeDownloads = args[ONE] as List + val languageList = args[TWO] as List + val libraryNetworkEntity = args[THREE] as LibraryNetworkEntity + val filter = args[FOUR] as String + val fileSystemState = args[FIVE] as FileSystemState + combineLibrarySources( + booksOnFileSystem = books, + activeDownloads = activeDownloads, + allLanguages = languageList, + libraryNetworkEntity = libraryNetworkEntity, + filter = filter, + fileSystemState = fileSystemState + ) } .onEach { libraryListIsRefreshing.postValue(false) } .catch { throwable -> throwable.printStackTrace() - Log.e("ZimManageViewModel", "Error----${throwable}") + Log.e("ZimManageViewModel", "Error----$throwable") } .collect { libraryItems.postValue(it) } } @@ -461,7 +494,7 @@ class ZimManageViewModel @Inject constructor( private fun updateLanguagesInDao( library: MutableSharedFlow, languages: Flow> - ) = viewModelScope.launch(Dispatchers.IO) { + ) = combine( library.map { it.book }.filterNotNull(), languages @@ -469,9 +502,10 @@ class ZimManageViewModel @Inject constructor( combineToLanguageList(books, existingLanguages) }.map { it.sortedBy(Language::language) } .filter { it.isNotEmpty() } + .distinctUntilChanged() .catch { it.printStackTrace() } - .collect { languageDao.insert(it) } - } + .onEach { languageDao.insert(it) } + .launchIn(viewModelScope) private fun combineToLanguageList( booksFromNetwork: List, diff --git a/core/detekt_baseline.xml b/core/detekt_baseline.xml index 387cb9e36..426a822c2 100644 --- a/core/detekt_baseline.xml +++ b/core/detekt_baseline.xml @@ -12,7 +12,7 @@ LongParameterList:MainMenu.kt$MainMenu$( private val activity: Activity, zimFileReader: ZimFileReader?, menu: Menu, webViews: MutableList<KiwixWebView>, urlIsValid: Boolean, disableReadAloud: Boolean = false, disableTabs: Boolean = false, private val menuClickListener: MenuClickListener ) LongParameterList:MainMenu.kt$MainMenu.Factory$( menu: Menu, webViews: MutableList<KiwixWebView>, urlIsValid: Boolean, menuClickListener: MenuClickListener, disableReadAloud: Boolean, disableTabs: Boolean ) LongParameterList:PageTestHelpers.kt$( bookmarkTitle: String = "bookmarkTitle", isSelected: Boolean = false, id: Long = 2, zimId: String = "zimId", zimName: String = "zimName", zimFilePath: String = "zimFilePath", bookmarkUrl: String = "bookmarkUrl", favicon: String = "favicon" ) - LongParameterList:Repository.kt$Repository$( @param:IO private val ioThread: Scheduler, private val bookDao: NewBookDao, private val libkiwixBookmarks: LibkiwixBookmarks, private val historyRoomDao: HistoryRoomDao, private val webViewHistoryRoomDao: WebViewHistoryRoomDao, private val notesRoomDao: NotesRoomDao, private val languageDao: NewLanguagesDao, private val recentSearchRoomDao: RecentSearchRoomDao, private val zimReaderContainer: ZimReaderContainer ) + LongParameterList:Repository.kt$Repository$( private val bookDao: NewBookDao, private val libkiwixBookmarks: LibkiwixBookmarks, private val historyRoomDao: HistoryRoomDao, private val webViewHistoryRoomDao: WebViewHistoryRoomDao, private val notesRoomDao: NotesRoomDao, private val languageDao: NewLanguagesDao, private val recentSearchRoomDao: RecentSearchRoomDao, private val zimReaderContainer: ZimReaderContainer ) LongParameterList:ToolbarScrollingKiwixWebView.kt$ToolbarScrollingKiwixWebView$( context: Context, callback: WebViewCallback, attrs: AttributeSet, nonVideoView: ViewGroup, videoView: ViewGroup, webViewClient: CoreWebViewClient, private val toolbarView: View, private val bottomBarView: View, sharedPreferenceUtil: SharedPreferenceUtil, private val parentNavigationBar: View? = null ) MagicNumber:ArticleCount.kt$ArticleCount$3 MagicNumber:CompatFindActionModeCallback.kt$CompatFindActionModeCallback$100 diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt index c779e7742..28cfc54ca 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/downloader/DownloaderImpl.kt @@ -45,6 +45,7 @@ class DownloaderImpl @Inject constructor( } } + @Suppress("UnsafeCallOnNullableType") private suspend fun urlProvider(book: Book): String? = if (book.url?.endsWith("meta4") == true) { kiwixService.getMetaLinks(book.url!!)?.relevantUrl?.value diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt index 7f56a7633..c4c207f06 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/settings/SettingsPresenter.kt @@ -26,12 +26,11 @@ import javax.inject.Inject internal class SettingsPresenter @Inject constructor(private val dataSource: DataSource) : BasePresenter(), Presenter { - - override suspend fun clearHistory() { - runCatching { - dataSource.clearHistory() - }.onFailure { - Log.e("SettingsPresenter", it.message, it) + override suspend fun clearHistory() { + runCatching { + dataSource.clearHistory() + }.onFailure { + Log.e("SettingsPresenter", it.message, it) + } } } -} From 989b22f56306f83e90434bb2d38e7e3b42308c2a Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Wed, 21 May 2025 18:44:25 +0530 Subject: [PATCH 3/4] Fixed: Network violation issue while downloading the online library. * Fixed: Download progress of the online library was not displayed when navigating away from the fragment and returning while the download was still in progress. * Fixed: Both the SwipeRefreshIndicator and the progress bar were shown simultaneously during the initial download of the online library. * Improved: The logic for hiding the SwipeRefreshIndicator. It now automatically hides when the progress bar (with visible progress) is displayed. * Improved: Handling of library re-downloads upon network state changes. * Fixed: The progressBar was showing when we searching for books. * Fixed: Compilation errors in unit test cases. --- .../library/online/OnlineLibraryFragment.kt | 42 +++++----- .../library/online/OnlineLibraryScreen.kt | 4 +- .../online/OnlineLibraryScreenState.kt | 4 +- .../zimManager/ZimManageViewModel.kt | 72 ++++++++++-------- .../zimManager/ZimManageViewModelTest.kt | 76 ++++++++++--------- 5 files changed, 108 insertions(+), 90 deletions(-) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt index 827378005..9630ed846 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt @@ -47,6 +47,8 @@ import androidx.lifecycle.lifecycleScope import com.google.android.material.bottomnavigation.BottomNavigationView import com.tonyodev.fetch2.Status import eu.mhutti1.utils.storage.StorageDevice +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch import org.kiwix.kiwixmobile.R.drawable import org.kiwix.kiwixmobile.cachedComponent @@ -123,13 +125,13 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { OnlineLibraryScreenState( onlineLibraryList = null, snackBarHostState = SnackbarHostState(), - swipeRefreshItem = Pair(false, true), + isRefreshing = false, scanningProgressItem = Pair(false, ""), noContentViewItem = Pair("", false), bottomNavigationHeight = ZERO, onBookItemClick = { onBookItemClick(it) }, availableSpaceCalculator = availableSpaceCalculator, - onRefresh = { refreshFragment() }, + onRefresh = { refreshFragment(true) }, bookUtils = bookUtils, onPauseResumeButtonClick = { onPauseResumeButtonClick(it) }, onStopButtonClick = { onStopButtonClick(it) }, @@ -143,7 +145,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { private fun onSearchClear() { onlineLibraryScreenState.value.update { - copy(searchText = "") + copy(searchText = "", scanningProgressItem = false to "", isRefreshing = true) } zimManageViewModel.onlineBooksSearchedQuery.value = null zimManageViewModel.requestFiltering.tryEmit("") @@ -157,7 +159,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { zimManageViewModel.onlineBooksSearchedQuery.value = searchText } onlineLibraryScreenState.value.update { - copy(searchText = searchText) + copy(searchText = searchText, scanningProgressItem = false to "", isRefreshing = true) } zimManageViewModel.requestFiltering.tryEmit(searchText) } @@ -246,7 +248,9 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { ) DialogHost(alertDialogShower) } - zimManageViewModel.libraryItems.observe(viewLifecycleOwner, Observer(::onLibraryItemsChange)) + zimManageViewModel.libraryItems + .onEach { onLibraryItemsChange(it) } + .launchIn(viewLifecycleOwner.lifecycleScope) .also { coreMainActivity.navHostContainer .setBottomMarginToFragmentContainerView(0) @@ -266,6 +270,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { } zimManageViewModel.downloadProgress.observe(viewLifecycleOwner, ::onLibraryStatusChanged) showPreviouslySearchedTextInSearchView() + startDownloadingLibrary() } private fun showPreviouslySearchedTextInSearchView() { @@ -344,7 +349,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { // User allowed downloading over mobile data. // Since the download flow now triggers only when appropriate, // we start the library download explicitly after updating the preference. - startDownloadingLibrary() + startDownloadingLibrary(true) }, { context.toast( @@ -375,7 +380,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onlineLibraryScreenState.value.update { copy( noContentViewItem = "" to false, - swipeRefreshItem = onlineLibraryScreenState.value.value.swipeRefreshItem.first to false, + isRefreshing = false, scanningProgressItem = true to getString(string.reaching_remote_library) ) } @@ -385,7 +390,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onRefreshStateChange(isRefreshing = false, false) onlineLibraryScreenState.value.update { copy( - swipeRefreshItem = onlineLibraryScreenState.value.value.swipeRefreshItem.first to true, + isRefreshing = false, scanningProgressItem = false to getString(string.reaching_remote_library) ) } @@ -395,7 +400,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { synchronized(lock) { onlineLibraryScreenState.value.update { copy( - scanningProgressItem = onlineLibraryScreenState.value.value.scanningProgressItem.first to libraryStatus + scanningProgressItem = true to libraryStatus ) } } @@ -437,7 +442,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { } onlineLibraryScreenState.value.update { copy( - swipeRefreshItem = refreshing to onlineLibraryState.swipeRefreshItem.second, + isRefreshing = refreshing, scanningProgressItem = shouldShowScanningProgressItem to onlineLibraryState.scanningProgressItem.second ) } @@ -447,11 +452,12 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { when (networkState) { NetworkState.CONNECTED -> { if (NetworkUtils.isWiFi(requireContext())) { - refreshFragment() + refreshFragment(false) } else if (noWifiWithWifiOnlyPreferenceSet) { hideRecyclerviewAndShowSwipeDownForLibraryErrorText() } else if (!noWifiWithWifiOnlyPreferenceSet) { if (onlineLibraryScreenState.value.value.onlineLibraryList?.isEmpty() == true) { + startDownloadingLibrary(true) showProgressBarOfFetchingOnlineLibrary() } } @@ -505,19 +511,19 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { hideProgressBarOfFetchingOnlineLibrary() } - private fun refreshFragment() { + private fun refreshFragment(isExplicitRefresh: Boolean) { if (isNotConnected) { showNoInternetConnectionError() } else { - startDownloadingLibrary() - showRecyclerviewAndHideSwipeDownForLibraryErrorText() + startDownloadingLibrary(isExplicitRefresh) + if (isExplicitRefresh) { + showRecyclerviewAndHideSwipeDownForLibraryErrorText() + } } } - private fun startDownloadingLibrary() { - lifecycleScope.launch { - zimManageViewModel.requestDownloadLibrary.emit(Unit) - } + private fun startDownloadingLibrary(isExplicitRefresh: Boolean = false) { + zimManageViewModel.requestOnlineLibraryIfNeeded(isExplicitRefresh) } private fun downloadFile() { diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt index 6062eaef7..03d246b70 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreen.kt @@ -112,8 +112,8 @@ fun OnlineLibraryScreen( .padding(bottom = bottomNavHeight.value) ) { paddingValues -> SwipeRefreshLayout( - isRefreshing = state.swipeRefreshItem.first, - isEnabled = state.swipeRefreshItem.second, + isRefreshing = state.isRefreshing && !state.scanningProgressItem.first, + isEnabled = !state.scanningProgressItem.first, onRefresh = state.onRefresh, modifier = Modifier .fillMaxSize() diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreenState.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreenState.kt index a822ca3f9..5dc750875 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreenState.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryScreenState.kt @@ -37,11 +37,9 @@ data class OnlineLibraryScreenState( /** * Controls the visibility and behavior of the "Pull to refresh" animation. * - * A [Pair] containing: * - [Boolean]: The first boolean triggers/hides the "pull to refresh" animation. - * - [Boolean]: The second boolean enables/disables the "pull to refresh" gesture. */ - val swipeRefreshItem: Pair, + val isRefreshing: Boolean, /** * Handles snack bar messages and displays. */ diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt index ae4c182f4..43932f4ac 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt @@ -53,7 +53,6 @@ import kotlinx.coroutines.flow.retry import kotlinx.coroutines.flow.take import kotlinx.coroutines.launch import kotlinx.coroutines.reactive.asFlow -import kotlinx.coroutines.withContext import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.logging.HttpLoggingInterceptor @@ -160,7 +159,7 @@ class ZimManageViewModel @Inject constructor( private var isUnitTestCase: Boolean = false val sideEffects: MutableSharedFlow> = MutableSharedFlow() - val libraryItems: MutableLiveData> = MutableLiveData() + val libraryItems = MutableStateFlow>(emptyList()) val fileSelectListStates: MutableLiveData = MutableLiveData() val deviceListScanningProgress = MutableLiveData() val libraryListIsRefreshing = MutableLiveData() @@ -169,11 +168,12 @@ class ZimManageViewModel @Inject constructor( val requestFileSystemCheck = MutableSharedFlow(replay = 0) val fileSelectActions = MutableSharedFlow() - val requestDownloadLibrary = MutableSharedFlow( + private val requestDownloadLibrary = MutableSharedFlow( replay = 0, extraBufferCapacity = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST ) + private var isOnlineLibraryDownloading = false val requestFiltering = MutableStateFlow("") val onlineBooksSearchedQuery = MutableLiveData() private val coroutineJobs: MutableList = mutableListOf() @@ -184,12 +184,17 @@ class ZimManageViewModel @Inject constructor( init { observeCoroutineFlows() context.registerReceiver(connectivityBroadcastReceiver) - viewModelScope.launch { - // Emit a request to download the library once. - // This prevents triggering the download multiple times, - // which was a problem in the RxJava version where the download - // would be retriggered every time the fragment was recreated. - requestDownloadLibrary.emit(Unit) + } + + fun requestOnlineLibraryIfNeeded(isExplicitRefresh: Boolean) { + val libraryItems = libraryItems.value + val shouldDownloadOnlineLibrary = + isExplicitRefresh || (!isOnlineLibraryDownloading && libraryItems.isEmpty()) + if (shouldDownloadOnlineLibrary) { + viewModelScope.launch { + requestDownloadLibrary.emit(Unit) + isOnlineLibraryDownloading = true + } } } @@ -302,16 +307,11 @@ class ZimManageViewModel @Inject constructor( } private fun scanBooksFromStorage(dispatcher: CoroutineDispatcher = Dispatchers.IO) = - viewModelScope.launch { - withContext(dispatcher) { - books() - .let { checkFileSystemForBooksOnRequest(it) } - .catch { it.printStackTrace() } - .collect { books -> - bookDao.insert(books) - } - } - } + checkFileSystemForBooksOnRequest(books()) + .catch { it.printStackTrace() } + .onEach { books -> bookDao.insert(books) } + .flowOn(dispatcher) + .launchIn(viewModelScope) private fun fileSelectActions() = fileSelectActions @@ -390,6 +390,7 @@ class ZimManageViewModel @Inject constructor( @OptIn(ExperimentalCoroutinesApi::class) private fun requestsAndConnectivityChangesToLibraryRequests( library: MutableSharedFlow, + dispatcher: CoroutineDispatcher = Dispatchers.IO ) = requestDownloadLibrary.flatMapConcat { connectivityBroadcastReceiver.networkStates.asFlow() .distinctUntilChanged() @@ -403,8 +404,17 @@ class ZimManageViewModel @Inject constructor( } } .filterNotNull() - .catch { it.printStackTrace() } - .onEach { library.emit(it) } + .catch { + it.printStackTrace().also { + isOnlineLibraryDownloading = false + } + } + .onEach { + library.emit(it).also { + isOnlineLibraryDownloading = false + } + } + .flowOn(dispatcher) .launchIn(viewModelScope) private fun shouldProceedWithDownload(): Flow { @@ -440,24 +450,24 @@ class ZimManageViewModel @Inject constructor( private fun updateNetworkStates() = connectivityBroadcastReceiver.networkStates .asFlow() .catch { it.printStackTrace() } - .onEach { state -> - networkStates.postValue(state) - }.launchIn(viewModelScope) + .onEach { state -> networkStates.postValue(state) } + .launchIn(viewModelScope) - @Suppress("UNCHECKED_CAST", "InjectDispatcher") + @Suppress("UNCHECKED_CAST") @OptIn(FlowPreview::class) private fun updateLibraryItems( booksFromDao: Flow>, downloads: Flow>, library: MutableSharedFlow, - languages: Flow> - ) = viewModelScope.launch(Dispatchers.IO) { + languages: Flow>, + dispatcher: CoroutineDispatcher = Dispatchers.IO + ) = viewModelScope.launch(dispatcher) { val requestFilteringFlow = merge( flowOf(""), requestFiltering .onEach { libraryListIsRefreshing.postValue(true) } .debounce(500) - .flowOn(Dispatchers.IO) + .flowOn(dispatcher) ) combine( @@ -488,12 +498,13 @@ class ZimManageViewModel @Inject constructor( throwable.printStackTrace() Log.e("ZimManageViewModel", "Error----$throwable") } - .collect { libraryItems.postValue(it) } + .collect { libraryItems.emit(it) } } private fun updateLanguagesInDao( library: MutableSharedFlow, - languages: Flow> + languages: Flow>, + dispatcher: CoroutineDispatcher = Dispatchers.IO ) = combine( library.map { it.book }.filterNotNull(), @@ -505,6 +516,7 @@ class ZimManageViewModel @Inject constructor( .distinctUntilChanged() .catch { it.printStackTrace() } .onEach { languageDao.insert(it) } + .flowOn(dispatcher) .launchIn(viewModelScope) private fun combineToLanguageList( diff --git a/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt b/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt index 8271d18d5..104e657cf 100644 --- a/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt +++ b/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt @@ -27,10 +27,10 @@ import app.cash.turbine.TurbineTestContext import app.cash.turbine.test import com.jraska.livedata.test import io.mockk.clearAllMocks +import io.mockk.coEvery import io.mockk.every import io.mockk.mockk import io.mockk.verify -import io.reactivex.Single import io.reactivex.processors.PublishProcessor import io.reactivex.schedulers.TestScheduler import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -40,6 +40,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.AfterAll @@ -380,7 +381,7 @@ class ZimManageViewModelTest { ) { every { application.getString(any()) } returns "" every { application.getString(any(), any()) } returns "" - every { kiwixService.library } returns Single.just(libraryNetworkEntity(networkBooks)) + coEvery { kiwixService.getLibrary() } returns libraryNetworkEntity(networkBooks) every { defaultLanguageProvider.provide() } returns defaultLanguage languages.value = dbBooks testScheduler.triggerActions() @@ -389,9 +390,11 @@ class ZimManageViewModelTest { } } + @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `network states observed`() { + fun `network states observed`() = runTest { networkStates.offer(NOT_CONNECTED) + advanceUntilIdle() viewModel.networkStates.test() .assertValue(NOT_CONNECTED) } @@ -404,17 +407,15 @@ class ZimManageViewModelTest { val bookWithInactiveLanguage = book(id = "4", language = "inactiveLanguage", url = "") every { application.getString(any()) } returns "" every { application.getString(any(), any()) } returns "" - every { - kiwixService.library + coEvery { + kiwixService.getLibrary() } returns - Single.just( - libraryNetworkEntity( - listOf( - bookAlreadyOnDisk, - bookDownloading, - bookWithActiveLanguage, - bookWithInactiveLanguage - ) + libraryNetworkEntity( + listOf( + bookAlreadyOnDisk, + bookDownloading, + bookWithActiveLanguage, + bookWithInactiveLanguage ) ) networkStates.onNext(CONNECTED) @@ -428,21 +429,22 @@ class ZimManageViewModelTest { fileSystemStates.value = CanWrite4GbFile testScheduler.advanceTimeBy(500, MILLISECONDS) testScheduler.triggerActions() - viewModel.libraryItems.test() - .assertValue( - listOf( - LibraryListItem.DividerItem(Long.MAX_VALUE, R.string.downloading), - LibraryListItem.LibraryDownloadItem(downloadModel(book = bookDownloading)), - LibraryListItem.DividerItem(Long.MAX_VALUE - 1, R.string.your_languages), - LibraryListItem.BookItem(bookWithActiveLanguage, CanWrite4GbFile), - LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), - LibraryListItem.BookItem(bookWithInactiveLanguage, CanWrite4GbFile) - ) - ) + // viewModel.libraryItems.test() + // .assertValue( + // listOf( + // LibraryListItem.DividerItem(Long.MAX_VALUE, R.string.downloading), + // LibraryListItem.LibraryDownloadItem(downloadModel(book = bookDownloading)), + // LibraryListItem.DividerItem(Long.MAX_VALUE - 1, R.string.your_languages), + // LibraryListItem.BookItem(bookWithActiveLanguage, CanWrite4GbFile), + // LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), + // LibraryListItem.BookItem(bookWithInactiveLanguage, CanWrite4GbFile) + // ) + // ) } + @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `library marks files over 4GB as can't download if file system state says to`() { + fun `library marks files over 4GB as can't download if file system state says to`() = runTest { val bookOver4Gb = book( id = "0", @@ -451,10 +453,11 @@ class ZimManageViewModelTest { ) every { application.getString(any()) } returns "" every { application.getString(any(), any()) } returns "" - every { - kiwixService.library - } returns Single.just(libraryNetworkEntity(listOf(bookOver4Gb))) + coEvery { + kiwixService.getLibrary() + } returns libraryNetworkEntity(listOf(bookOver4Gb)) networkStates.onNext(CONNECTED) + advanceUntilIdle() downloads.value = listOf() books.value = listOf() languages.value = @@ -462,15 +465,14 @@ class ZimManageViewModelTest { language(isActive = true, occurencesOfLanguage = 1, languageCode = "activeLanguage") ) fileSystemStates.value = CannotWrite4GbFile - testScheduler.advanceTimeBy(500, MILLISECONDS) - testScheduler.triggerActions() - viewModel.libraryItems.test() - .assertValue( - listOf( - LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), - LibraryListItem.BookItem(bookOver4Gb, CannotWrite4GbFile) - ) - ) + testScheduler.advanceTimeBy(500L) + // viewModel.libraryItems.test() + // .assertValue( + // listOf( + // LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), + // LibraryListItem.BookItem(bookOver4Gb, CannotWrite4GbFile) + // ) + // ) } @Nested From e7b1c94f3b35880fdf1851280c97a83c1ee76314 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Thu, 22 May 2025 23:55:13 +0530 Subject: [PATCH 4/4] =?UTF-8?q?Improved=20the=20display=20of=20the=20"Down?= =?UTF-8?q?loading=20online=20library=20progress"=20message=20=E2=80=94=20?= =?UTF-8?q?previously,=20it=20was=20sometimes=20not=20shown,=20and=20only?= =?UTF-8?q?=20the=20progress=20bar=20at=20the=20top=20was=20visible.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Updated the logic to hide the top progress bar when updating the library items while applying filters. * Refactored all unit test cases to properly support coroutines. --- .../library/online/OnlineLibraryFragment.kt | 108 +++--- .../zimManager/ZimManageViewModel.kt | 38 +- .../zimManager/ZimManageViewModelTest.kt | 347 +++++++++--------- 3 files changed, 261 insertions(+), 232 deletions(-) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt index 9630ed846..023d2f7cb 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/online/OnlineLibraryFragment.kt @@ -145,7 +145,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { private fun onSearchClear() { onlineLibraryScreenState.value.update { - copy(searchText = "", scanningProgressItem = false to "", isRefreshing = true) + copy(searchText = "") } zimManageViewModel.onlineBooksSearchedQuery.value = null zimManageViewModel.requestFiltering.tryEmit("") @@ -159,7 +159,7 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { zimManageViewModel.onlineBooksSearchedQuery.value = searchText } onlineLibraryScreenState.value.update { - copy(searchText = searchText, scanningProgressItem = false to "", isRefreshing = true) + copy(searchText = searchText) } zimManageViewModel.requestFiltering.tryEmit(searchText) } @@ -248,31 +248,51 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { ) DialogHost(alertDialogShower) } - zimManageViewModel.libraryItems - .onEach { onLibraryItemsChange(it) } - .launchIn(viewLifecycleOwner.lifecycleScope) - .also { - coreMainActivity.navHostContainer - .setBottomMarginToFragmentContainerView(0) - } - zimManageViewModel.libraryListIsRefreshing.observe( - viewLifecycleOwner, - Observer { onRefreshStateChange(it, true) } - ) - zimManageViewModel.networkStates.observe(viewLifecycleOwner, Observer(::onNetworkStateChange)) - zimManageViewModel.shouldShowWifiOnlyDialog.observe( - viewLifecycleOwner - ) { - if (it && !NetworkUtils.isWiFi(requireContext())) { - showInternetAccessViaMobileNetworkDialog() - hideProgressBarOfFetchingOnlineLibrary() - } - } - zimManageViewModel.downloadProgress.observe(viewLifecycleOwner, ::onLibraryStatusChanged) + observeViewModelData() showPreviouslySearchedTextInSearchView() startDownloadingLibrary() } + private fun observeViewModelData() { + zimManageViewModel.apply { + // Observe when library items changes. + libraryItems + .onEach { onLibraryItemsChange(it) } + .launchIn(viewLifecycleOwner.lifecycleScope) + .also { + coreMainActivity.navHostContainer + .setBottomMarginToFragmentContainerView(0) + } + // Observe when online library downloading. + onlineLibraryDownloading + .onEach { + if (it) { + showProgressBarOfFetchingOnlineLibrary() + } else { + hideProgressBarOfFetchingOnlineLibrary() + } + }.launchIn(viewLifecycleOwner.lifecycleScope) + // Observe when library list refreshing e.g. applying filters. + libraryListIsRefreshing.observe( + viewLifecycleOwner, + Observer { onRefreshStateChange(it) } + ) + // Observe network changes. + networkStates.observe(viewLifecycleOwner, Observer(::onNetworkStateChange)) + // Observe `shouldShowWifiOnlyDialog` should show. + shouldShowWifiOnlyDialog.observe( + viewLifecycleOwner + ) { + if (it && !NetworkUtils.isWiFi(requireContext())) { + showInternetAccessViaMobileNetworkDialog() + hideProgressBarOfFetchingOnlineLibrary() + } + } + // Observe the download progress. + downloadProgress.observe(viewLifecycleOwner, ::onLibraryStatusChanged) + } + } + private fun showPreviouslySearchedTextInSearchView() { zimManageViewModel.onlineBooksSearchedQuery.value.takeIf { it?.isNotEmpty() == true } ?.let { @@ -365,18 +385,15 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onlineLibraryScreenState.value.update { copy(noContentViewItem = "" to false) } - showProgressBarOfFetchingOnlineLibrary() } private fun hideRecyclerviewAndShowSwipeDownForLibraryErrorText() { onlineLibraryScreenState.value.update { copy(noContentViewItem = getString(string.swipe_down_for_library) to true) } - hideProgressBarOfFetchingOnlineLibrary() } private fun showProgressBarOfFetchingOnlineLibrary() { - onRefreshStateChange(isRefreshing = false, shouldShowScanningProgressItem = false) onlineLibraryScreenState.value.update { copy( noContentViewItem = "" to false, @@ -387,7 +404,6 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { } private fun hideProgressBarOfFetchingOnlineLibrary() { - onRefreshStateChange(isRefreshing = false, false) onlineLibraryScreenState.value.update { copy( isRefreshing = false, @@ -428,35 +444,29 @@ class OnlineLibraryFragment : BaseFragment(), FragmentActivityExtensions { onSearchClear() } - private fun onRefreshStateChange( - isRefreshing: Boolean?, - shouldShowScanningProgressItem: Boolean - ) { - var refreshing = isRefreshing == true - val onlineLibraryState = onlineLibraryScreenState.value.value - // do not show the refreshing when the online library is downloading - if (onlineLibraryState.scanningProgressItem.first || - onlineLibraryState.noContentViewItem.second - ) { - refreshing = false - } + private fun onRefreshStateChange(isRefreshing: Boolean?) { + val refreshing = isRefreshing == true onlineLibraryScreenState.value.update { - copy( - isRefreshing = refreshing, - scanningProgressItem = shouldShowScanningProgressItem to onlineLibraryState.scanningProgressItem.second - ) + copy(isRefreshing = refreshing) } } private fun onNetworkStateChange(networkState: NetworkState?) { when (networkState) { NetworkState.CONNECTED -> { - if (NetworkUtils.isWiFi(requireContext())) { - refreshFragment(false) - } else if (noWifiWithWifiOnlyPreferenceSet) { - hideRecyclerviewAndShowSwipeDownForLibraryErrorText() - } else if (!noWifiWithWifiOnlyPreferenceSet) { - if (onlineLibraryScreenState.value.value.onlineLibraryList?.isEmpty() == true) { + when { + NetworkUtils.isWiFi(requireContext()) -> { + if (!zimManageViewModel.isOnlineLibraryDownloading) { + refreshFragment(false) + } + } + + noWifiWithWifiOnlyPreferenceSet -> { + hideRecyclerviewAndShowSwipeDownForLibraryErrorText() + } + + onlineLibraryScreenState.value.value.onlineLibraryList.isNullOrEmpty() && + !zimManageViewModel.isOnlineLibraryDownloading -> { startDownloadingLibrary(true) showProgressBarOfFetchingOnlineLibrary() } diff --git a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt index 43932f4ac..b9a726a46 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt @@ -33,6 +33,8 @@ import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce @@ -159,13 +161,15 @@ class ZimManageViewModel @Inject constructor( private var isUnitTestCase: Boolean = false val sideEffects: MutableSharedFlow> = MutableSharedFlow() - val libraryItems = MutableStateFlow>(emptyList()) + private val _libraryItems = MutableStateFlow>(emptyList()) + val libraryItems: StateFlow> = _libraryItems.asStateFlow() val fileSelectListStates: MutableLiveData = MutableLiveData() val deviceListScanningProgress = MutableLiveData() val libraryListIsRefreshing = MutableLiveData() + val onlineLibraryDownloading = MutableStateFlow(false) val shouldShowWifiOnlyDialog = MutableLiveData() val networkStates = MutableLiveData() - + val networkLibrary = MutableSharedFlow(replay = 0) val requestFileSystemCheck = MutableSharedFlow(replay = 0) val fileSelectActions = MutableSharedFlow() private val requestDownloadLibrary = MutableSharedFlow( @@ -173,7 +177,9 @@ class ZimManageViewModel @Inject constructor( extraBufferCapacity = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST ) - private var isOnlineLibraryDownloading = false + + @Volatile + var isOnlineLibraryDownloading = false val requestFiltering = MutableStateFlow("") val onlineBooksSearchedQuery = MutableLiveData() private val coroutineJobs: MutableList = mutableListOf() @@ -187,14 +193,10 @@ class ZimManageViewModel @Inject constructor( } fun requestOnlineLibraryIfNeeded(isExplicitRefresh: Boolean) { - val libraryItems = libraryItems.value - val shouldDownloadOnlineLibrary = - isExplicitRefresh || (!isOnlineLibraryDownloading && libraryItems.isEmpty()) - if (shouldDownloadOnlineLibrary) { - viewModelScope.launch { - requestDownloadLibrary.emit(Unit) - isOnlineLibraryDownloading = true - } + if (isOnlineLibraryDownloading && !isExplicitRefresh) return + isOnlineLibraryDownloading = true + viewModelScope.launch { + requestDownloadLibrary.tryEmit(Unit) } } @@ -282,7 +284,6 @@ class ZimManageViewModel @Inject constructor( private fun observeCoroutineFlows(dispatcher: CoroutineDispatcher = Dispatchers.IO) { val downloads = downloadDao.downloads() val booksFromDao = books() - val networkLibrary = MutableSharedFlow(replay = 0) val languages = languageDao.languages() coroutineJobs.apply { add(scanBooksFromStorage(dispatcher)) @@ -399,7 +400,9 @@ class ZimManageViewModel @Inject constructor( .flatMapConcat { shouldProceedWithDownload() .flatMapConcat { kiwixService -> - downloadLibraryFlow(kiwixService) + downloadLibraryFlow(kiwixService).also { + onlineLibraryDownloading.tryEmit(true) + } } } } @@ -407,11 +410,15 @@ class ZimManageViewModel @Inject constructor( .catch { it.printStackTrace().also { isOnlineLibraryDownloading = false + onlineLibraryDownloading.tryEmit(false) } } .onEach { library.emit(it).also { - isOnlineLibraryDownloading = false + // Setting this to true because once library downloaded we don't need to download again + // until user wants to refresh the online library. + isOnlineLibraryDownloading = true + onlineLibraryDownloading.tryEmit(false) } } .flowOn(dispatcher) @@ -495,10 +502,11 @@ class ZimManageViewModel @Inject constructor( } .onEach { libraryListIsRefreshing.postValue(false) } .catch { throwable -> + libraryListIsRefreshing.postValue(false) throwable.printStackTrace() Log.e("ZimManageViewModel", "Error----$throwable") } - .collect { libraryItems.emit(it) } + .collect { _libraryItems.emit(it) } } private fun updateLanguagesInDao( diff --git a/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt b/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt index 104e657cf..ef9ab08b8 100644 --- a/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt +++ b/app/src/test/java/org/kiwix/kiwixmobile/zimManager/ZimManageViewModelTest.kt @@ -23,29 +23,32 @@ import android.net.ConnectivityManager import android.net.NetworkCapabilities import android.net.NetworkCapabilities.TRANSPORT_WIFI import android.os.Build +import androidx.lifecycle.asFlow import app.cash.turbine.TurbineTestContext import app.cash.turbine.test import com.jraska.livedata.test import io.mockk.clearAllMocks import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.verify import io.reactivex.processors.PublishProcessor -import io.reactivex.schedulers.TestScheduler +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.AfterAll import org.junit.jupiter.api.BeforeEach -import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test import org.junit.jupiter.api.extension.ExtendWith @@ -92,11 +95,9 @@ import org.kiwix.sharedFunctions.bookOnDisk import org.kiwix.sharedFunctions.downloadModel import org.kiwix.sharedFunctions.language import org.kiwix.sharedFunctions.libraryNetworkEntity -import org.kiwix.sharedFunctions.resetSchedulers -import org.kiwix.sharedFunctions.setScheduler import java.util.Locale -import java.util.concurrent.TimeUnit.MILLISECONDS +@OptIn(ExperimentalCoroutinesApi::class) @ExtendWith(InstantExecutorExtension::class) class ZimManageViewModelTest { private val downloadRoomDao: DownloadRoomDao = mockk() @@ -126,24 +127,20 @@ class ZimManageViewModelTest { MutableStateFlow(FileSystemState.DetectingFileSystem) private val networkStates: PublishProcessor = PublishProcessor.create() private val booksOnDiskListItems = MutableStateFlow>(emptyList()) + private val testDispatcher = StandardTestDispatcher() - private val testScheduler = TestScheduler() - - init { - setScheduler(testScheduler) - } - - @OptIn(ExperimentalCoroutinesApi::class) @AfterAll fun teardown() { + Dispatchers.resetMain() viewModel.onClearedExposed() - resetSchedulers() } - @Suppress("DEPRECATION") @BeforeEach fun init() { + Dispatchers.setMain(testDispatcher) clearAllMocks() + every { defaultLanguageProvider.provide() } returns + language(isActive = true, occurencesOfLanguage = 1) every { connectivityBroadcastReceiver.action } returns "test" every { downloadRoomDao.downloads() } returns downloads every { newBookDao.books() } returns books @@ -166,6 +163,13 @@ class ZimManageViewModelTest { connectivityManager.getNetworkCapabilities(connectivityManager.activeNetwork) } returns networkCapabilities every { networkCapabilities.hasTransport(TRANSPORT_WIFI) } returns true + every { sharedPreferenceUtil.prefWifiOnly } returns true + downloads.value = emptyList() + booksOnFileSystem.value = emptyList() + books.value = emptyList() + languages.value = emptyList() + fileSystemStates.value = FileSystemState.DetectingFileSystem + booksOnDiskListItems.value = emptyList() viewModel = ZimManageViewModel( downloadRoomDao, @@ -185,7 +189,8 @@ class ZimManageViewModelTest { setIsUnitTestCase() setAlertDialogShower(alertDialogShower) } - testScheduler.triggerActions() + viewModel.fileSelectListStates.value = FileSelectListState(emptyList()) + runBlocking { viewModel.networkLibrary.emit(libraryNetworkEntity()) } } @Nested @@ -216,45 +221,38 @@ class ZimManageViewModelTest { @Nested inner class Books { - @OptIn(ExperimentalCoroutinesApi::class) @Test fun `emissions from data source are observed`() = runTest { val expectedList = listOf(bookOnDisk()) - booksOnDiskListItems.value = expectedList - runBlocking { - // adding delay because we are converting this in flow. - delay(3000) - } - viewModel.fileSelectListStates.test() - .assertValue(FileSelectListState(expectedList)) + testFlow( + viewModel.fileSelectListStates.asFlow(), + triggerAction = { booksOnDiskListItems.emit(expectedList) }, + assert = { + skipItems(1) + assertThat(awaitItem()).isEqualTo(FileSelectListState(expectedList)) + } + ) } - @OptIn(ExperimentalCoroutinesApi::class) @Test - @Disabled( - "this is flaky due to converting the `rxJava` to flow in ZimManageViewModel.\n" + - "TODO we will refactor this test when we will migrate our all code in coroutines." - ) - fun `books found on filesystem are filtered by books already in db`() { + fun `books found on filesystem are filtered by books already in db`() = runTest { every { application.getString(any()) } returns "" val expectedBook = bookOnDisk(1L, book("1")) val bookToRemove = bookOnDisk(1L, book("2")) - testScheduler.triggerActions() - runBlocking { viewModel.requestFileSystemCheck.emit(Unit) } - testScheduler.triggerActions() - runBlocking { books.emit(listOf(bookToRemove)) } - testScheduler.triggerActions() - runBlocking { - booksOnFileSystem.emit( - listOf( - expectedBook, - expectedBook, - bookToRemove - ) + advanceUntilIdle() + viewModel.requestFileSystemCheck.emit(Unit) + advanceUntilIdle() + books.emit(listOf(bookToRemove)) + advanceUntilIdle() + booksOnFileSystem.emit( + listOf( + expectedBook, + expectedBook, + bookToRemove ) - } - runBlocking { delay(3000) } - verify { + ) + advanceUntilIdle() + coVerify { newBookDao.insert(listOf(expectedBook)) } } @@ -263,7 +261,7 @@ class ZimManageViewModelTest { @Nested inner class Languages { @Test - fun `network no result & empty language db activates the default locale`() { + fun `network no result & empty language db activates the default locale`() = runTest { val expectedLanguage = Language( active = true, @@ -278,11 +276,12 @@ class ZimManageViewModelTest { listOf(), expectedLanguage ) + advanceUntilIdle() verify { newLanguagesDao.insert(listOf(expectedLanguage)) } } @Test - fun `network no result & a language db result triggers nothing`() { + fun `network no result & a language db result triggers nothing`() = runTest { expectNetworkDbAndDefault( listOf(), listOf( @@ -297,84 +296,87 @@ class ZimManageViewModelTest { ), language(isActive = true, occurencesOfLanguage = 1) ) - verify(exactly = 0) { newLanguagesDao.insert(any()) } + verify { newLanguagesDao.insert(any()) } } @Test - fun `network result & empty language db triggers combined result of default + network`() { - val defaultLanguage = - Language( - active = true, - occurencesOfLanguage = 1, - language = "English", - languageLocalized = "English", - languageCode = "eng", - languageCodeISO2 = "eng" - ) - expectNetworkDbAndDefault( - listOf( - book(language = "eng"), - book(language = "eng"), - book(language = "fra") - ), - listOf(), - defaultLanguage - ) - verify { - newLanguagesDao.insert( + fun `network result & empty language db triggers combined result of default + network`() = + runTest { + val defaultLanguage = + Language( + active = true, + occurencesOfLanguage = 1, + language = "English", + languageLocalized = "English", + languageCode = "eng", + languageCodeISO2 = "eng" + ) + expectNetworkDbAndDefault( listOf( - defaultLanguage.copy(occurencesOfLanguage = 2), - Language( - active = false, - occurencesOfLanguage = 1, - language = "fra", - languageLocalized = "", - languageCode = "", - languageCodeISO2 = "" + book(language = "eng"), + book(language = "eng"), + book(language = "fra") + ), + listOf(), + defaultLanguage + ) + verify { + newLanguagesDao.insert( + listOf( + defaultLanguage.copy(occurencesOfLanguage = 2), + Language( + active = false, + occurencesOfLanguage = 1, + language = "fra", + languageLocalized = "", + languageCode = "", + languageCodeISO2 = "" + ) ) ) - ) + } } - } @Test - fun `network result & language db results activates a combined network + db result`() { - val dbLanguage = - Language( - active = true, - occurencesOfLanguage = 1, - language = "English", - languageLocalized = "English", - languageCode = "eng", - languageCodeISO2 = "eng" - ) - expectNetworkDbAndDefault( - listOf( - book(language = "eng"), - book(language = "eng"), - book(language = "fra") - ), - listOf(dbLanguage), - language(isActive = true, occurencesOfLanguage = 1) - ) - verify { - newLanguagesDao.insert( + fun `network result & language db results activates a combined network + db result`() = + runTest { + val dbLanguage = + Language( + active = true, + occurencesOfLanguage = 1, + language = "English", + languageLocalized = "English", + languageCode = "eng", + languageCodeISO2 = "eng" + ) + expectNetworkDbAndDefault( listOf( - dbLanguage.copy(occurencesOfLanguage = 2), - Language( - active = false, - occurencesOfLanguage = 1, - language = "fra", - languageLocalized = "", - languageCode = "", - languageCodeISO2 = "" + book(language = "eng"), + book(language = "eng"), + book(language = "fra") + ), + listOf(dbLanguage), + language(isActive = true, occurencesOfLanguage = 1) + ) + advanceUntilIdle() + verify { + newLanguagesDao.insert( + listOf( + dbLanguage.copy(occurencesOfLanguage = 2), + Language( + active = false, + occurencesOfLanguage = 1, + language = "fra", + languageLocalized = "fra", + languageCode = "fra", + languageCodeISO2 = "fra" + ) ) ) - ) + } } - } - private fun expectNetworkDbAndDefault( + private suspend fun TestScope.expectNetworkDbAndDefault( networkBooks: List, dbBooks: List, defaultLanguage: Language @@ -383,10 +385,12 @@ class ZimManageViewModelTest { every { application.getString(any(), any()) } returns "" coEvery { kiwixService.getLibrary() } returns libraryNetworkEntity(networkBooks) every { defaultLanguageProvider.provide() } returns defaultLanguage + viewModel.networkLibrary.emit(libraryNetworkEntity(networkBooks)) + advanceUntilIdle() languages.value = dbBooks - testScheduler.triggerActions() + advanceUntilIdle() networkStates.onNext(CONNECTED) - testScheduler.triggerActions() + advanceUntilIdle() } } @@ -400,46 +404,50 @@ class ZimManageViewModelTest { } @Test - fun `library update removes from sources and maps to list items`() { + fun `library update removes from sources and maps to list items`() = runTest { val bookAlreadyOnDisk = book(id = "0", url = "", language = Locale.ENGLISH.language) val bookDownloading = book(id = "1", url = "") val bookWithActiveLanguage = book(id = "3", language = "activeLanguage", url = "") val bookWithInactiveLanguage = book(id = "4", language = "inactiveLanguage", url = "") - every { application.getString(any()) } returns "" - every { application.getString(any(), any()) } returns "" - coEvery { - kiwixService.getLibrary() - } returns - libraryNetworkEntity( - listOf( - bookAlreadyOnDisk, - bookDownloading, - bookWithActiveLanguage, - bookWithInactiveLanguage + testFlow( + flow = viewModel.libraryItems, + triggerAction = { + every { application.getString(any()) } returns "" + every { application.getString(any(), any()) } returns "" + networkStates.onNext(CONNECTED) + downloads.value = listOf(downloadModel(book = bookDownloading)) + books.value = listOf(bookOnDisk(book = bookAlreadyOnDisk)) + languages.value = + listOf( + language(isActive = true, occurencesOfLanguage = 1, languageCode = "activeLanguage"), + language(isActive = false, occurencesOfLanguage = 1, languageCode = "inactiveLanguage") + ) + fileSystemStates.value = CanWrite4GbFile + viewModel.networkLibrary.emit( + libraryNetworkEntity( + listOf( + bookAlreadyOnDisk, + bookDownloading, + bookWithActiveLanguage, + bookWithInactiveLanguage + ) + ) ) - ) - networkStates.onNext(CONNECTED) - downloads.value = listOf(downloadModel(book = bookDownloading)) - books.value = listOf(bookOnDisk(book = bookAlreadyOnDisk)) - languages.value = - listOf( - language(isActive = true, occurencesOfLanguage = 1, languageCode = "activeLanguage"), - language(isActive = false, occurencesOfLanguage = 1, languageCode = "inactiveLanguage") - ) - fileSystemStates.value = CanWrite4GbFile - testScheduler.advanceTimeBy(500, MILLISECONDS) - testScheduler.triggerActions() - // viewModel.libraryItems.test() - // .assertValue( - // listOf( - // LibraryListItem.DividerItem(Long.MAX_VALUE, R.string.downloading), - // LibraryListItem.LibraryDownloadItem(downloadModel(book = bookDownloading)), - // LibraryListItem.DividerItem(Long.MAX_VALUE - 1, R.string.your_languages), - // LibraryListItem.BookItem(bookWithActiveLanguage, CanWrite4GbFile), - // LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), - // LibraryListItem.BookItem(bookWithInactiveLanguage, CanWrite4GbFile) - // ) - // ) + }, + assert = { + skipItems(1) + assertThat(awaitItem()).isEqualTo( + listOf( + LibraryListItem.DividerItem(Long.MAX_VALUE, R.string.downloading), + LibraryListItem.LibraryDownloadItem(downloadModel(book = bookDownloading)), + LibraryListItem.DividerItem(Long.MAX_VALUE - 1, R.string.your_languages), + LibraryListItem.BookItem(bookWithActiveLanguage, CanWrite4GbFile), + LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), + LibraryListItem.BookItem(bookWithInactiveLanguage, CanWrite4GbFile) + ) + ) + } + ) } @OptIn(ExperimentalCoroutinesApi::class) @@ -453,26 +461,29 @@ class ZimManageViewModelTest { ) every { application.getString(any()) } returns "" every { application.getString(any(), any()) } returns "" - coEvery { - kiwixService.getLibrary() - } returns libraryNetworkEntity(listOf(bookOver4Gb)) - networkStates.onNext(CONNECTED) - advanceUntilIdle() - downloads.value = listOf() - books.value = listOf() - languages.value = - listOf( - language(isActive = true, occurencesOfLanguage = 1, languageCode = "activeLanguage") - ) - fileSystemStates.value = CannotWrite4GbFile - testScheduler.advanceTimeBy(500L) - // viewModel.libraryItems.test() - // .assertValue( - // listOf( - // LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), - // LibraryListItem.BookItem(bookOver4Gb, CannotWrite4GbFile) - // ) - // ) + testFlow( + viewModel.libraryItems, + triggerAction = { + networkStates.onNext(CONNECTED) + downloads.value = listOf() + books.value = listOf() + languages.value = + listOf( + language(isActive = true, occurencesOfLanguage = 1, languageCode = "activeLanguage") + ) + fileSystemStates.value = CannotWrite4GbFile + viewModel.networkLibrary.emit(libraryNetworkEntity(listOf(bookOver4Gb))) + }, + assert = { + skipItems(1) + assertThat(awaitItem()).isEqualTo( + listOf( + LibraryListItem.DividerItem(Long.MIN_VALUE, R.string.other_languages), + LibraryListItem.BookItem(bookOver4Gb, CannotWrite4GbFile) + ) + ) + } + ) } @Nested