From 0bc1392f1b2a4bc2243c4c9cbc6c5c9299cb0c98 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Mon, 11 Dec 2023 18:04:04 +0530 Subject: [PATCH 1/3] Fixed the app crashes that occurred when deleting letters in a search. * Removed the unnecessary estimation matches call on `libkiwix`, as we do not need this since `libkiwix` can handle this automatically. * Cancelled the previously running job if a new searchTerm is in progress; this will avoid unnecessary memory allocation and data load on libkiwix. --- .../kiwixmobile/core/search/SearchFragment.kt | 2 +- .../core/search/viewmodel/SearchState.kt | 24 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt index d0fa06af50..caa8f0979f 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt @@ -260,7 +260,7 @@ class SearchFragment : BaseFragment() { fragmentSearchBinding?.searchLoadingIndicator?.isShowing(true) renderingJob = searchViewModel.viewModelScope.launch(Dispatchers.Main) { val searchResult = withContext(Dispatchers.IO) { - state.getVisibleResults(0) + state.getVisibleResults(0, renderingJob) } fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false) diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchState.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchState.kt index 89c3cfbea3..21bd3bf4bd 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchState.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchState.kt @@ -18,6 +18,7 @@ package org.kiwix.kiwixmobile.core.search.viewmodel +import kotlinx.coroutines.Job import org.kiwix.kiwixmobile.core.search.adapter.SearchListItem data class SearchState( @@ -26,18 +27,29 @@ data class SearchState( val recentResults: List, val searchOrigin: SearchOrigin ) { - fun getVisibleResults(startIndex: Int): List? = + @Suppress("NestedBlockDepth") + fun getVisibleResults( + startIndex: Int, + job: Job? = null + ): List? = if (searchTerm.isEmpty()) { recentResults } else { searchResultsWithTerm.suggestionSearch?.let { - val maximumResults = it.estimatedMatches - val safeEndIndex = - if (startIndex + 100 < maximumResults) startIndex + 100 else maximumResults - val searchIterator = - it.getResults(startIndex, safeEndIndex.toInt()) val searchResults = mutableListOf() + if (job?.isActive == false) { + // if the previous job is cancel then do not execute the code + return@getVisibleResults searchResults + } + val safeEndIndex = startIndex + 100 + val searchIterator = + it.getResults(startIndex, safeEndIndex) while (searchIterator.hasNext()) { + if (job?.isActive == false) { + // check if the previous job is cancel while retrieving the data for previous searched item + // then break the execution of code. + break + } val entry = searchIterator.next() searchResults.add(SearchListItem.RecentSearchListItem(entry.title, entry.path)) } From 6d5e50d0ef35b4ca2c88a964f71a9e5c8fd14728 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Mon, 11 Dec 2023 19:12:18 +0530 Subject: [PATCH 2/3] Fixed the `SearchStateTest` was failing due to improvements in the search loading functionality. --- .../kiwix/kiwixmobile/core/search/viewmodel/SearchStateTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchStateTest.kt b/core/src/test/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchStateTest.kt index 8f39f5cb5d..9bc9428470 100644 --- a/core/src/test/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchStateTest.kt +++ b/core/src/test/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchStateTest.kt @@ -35,7 +35,7 @@ internal class SearchStateTest { val suggestionSearchWrapper: SuggestionSearchWrapper = mockk() val searchIteratorWrapper: SuggestionIteratorWrapper = mockk() val entryWrapper: SuggestionItemWrapper = mockk() - val estimatedMatches = 1 + val estimatedMatches = 100 every { suggestionSearchWrapper.estimatedMatches } returns estimatedMatches.toLong() // Settings list to hasNext() to ensure it returns true only for the first call. // Otherwise, if we do not set this, the method will always return true when checking if the iterator has a next value, From a07bb680c32ea84f57b5141e4a9f2489d5853d1e Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Tue, 12 Dec 2023 15:31:53 +0530 Subject: [PATCH 3/3] Introduced a mutex to manage concurrency in the search functionality. This enables proper clearing of the first running job before executing a new one. By implementing this approach, we ensure that access to the libzim search results occurs one at a time, resolving the crashing issue caused by multiple attempts to access libzim resources. * Replaced the `cancel` function for the Job with `cancelAndJoin`, ensuring that it thoroughly cancels the first job before initiating the new task. --- .../kiwixmobile/core/search/SearchFragment.kt | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt b/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt index caa8f0979f..77c29dd4a5 100644 --- a/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt +++ b/core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt @@ -43,7 +43,10 @@ import androidx.recyclerview.widget.RecyclerView import io.reactivex.android.schedulers.AndroidSchedulers import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.kiwix.kiwixmobile.core.R import org.kiwix.kiwixmobile.core.base.BaseActivity @@ -86,6 +89,7 @@ class SearchFragment : BaseFragment() { private var searchAdapter: SearchAdapter? = null private var isDataLoading = false private var renderingJob: Job? = null + private val searchMutex = Mutex() override fun inject(baseActivity: BaseActivity) { baseActivity.cachedComponent.inject(this) @@ -252,22 +256,25 @@ class SearchFragment : BaseFragment() { ) } - private fun render(state: SearchState) { - renderingJob?.cancel() - isDataLoading = false - searchInTextMenuItem?.isVisible = state.searchOrigin == FromWebView - searchInTextMenuItem?.isEnabled = state.searchTerm.isNotBlank() - fragmentSearchBinding?.searchLoadingIndicator?.isShowing(true) - renderingJob = searchViewModel.viewModelScope.launch(Dispatchers.Main) { - val searchResult = withContext(Dispatchers.IO) { - state.getVisibleResults(0, renderingJob) - } + private suspend fun render(state: SearchState) { + searchMutex.withLock { + // `cancelAndJoin` cancels the previous running job and waits for it to completely cancel. + renderingJob?.cancelAndJoin() + isDataLoading = false + searchInTextMenuItem?.isVisible = state.searchOrigin == FromWebView + searchInTextMenuItem?.isEnabled = state.searchTerm.isNotBlank() + fragmentSearchBinding?.searchLoadingIndicator?.isShowing(true) + renderingJob = searchViewModel.viewModelScope.launch(Dispatchers.Main) { + val searchResult = withContext(Dispatchers.IO) { + state.getVisibleResults(0, renderingJob) + } - fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false) + fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false) - searchResult?.let { - fragmentSearchBinding?.searchNoResults?.isVisible = it.isEmpty() - searchAdapter?.items = it + searchResult?.let { + fragmentSearchBinding?.searchNoResults?.isVisible = it.isEmpty() + searchAdapter?.items = it + } } } }