From 332c1a6c59071b6429028e024ef21d6a3e38a2c9 Mon Sep 17 00:00:00 2001 From: Viktor De Pasquale Date: Mon, 18 Nov 2019 17:20:54 +0100 Subject: [PATCH] Removed overcomplicated updates loading The mechanism was replaced by loading updated directly by id to the initial list. There are two factors why yesterday-me was dumb: 1) By asynchronously loading update state, you have no control over it - hence no search 2) It's incredibly wasteful; running that hardcore search on every query? Not cool ...and from UX stand-point having updates inlined right under installed modules is by far better than nitpicking it from the list or in the search --- .../topjohnwu/magisk/data/database/Repo.kt | 3 + .../model/entity/recycler/ModuleRvItem.kt | 31 ++++-- .../magisk/redesign/module/ModuleViewModel.kt | 96 ++++++------------- app/src/main/res/layout/item_repo_md2.xml | 2 +- app/src/main/res/values/strings_md2.xml | 3 +- build.gradle | 2 +- 6 files changed, 62 insertions(+), 75 deletions(-) diff --git a/app/src/main/java/com/topjohnwu/magisk/data/database/Repo.kt b/app/src/main/java/com/topjohnwu/magisk/data/database/Repo.kt index 457f969b3..c97dd8ed7 100644 --- a/app/src/main/java/com/topjohnwu/magisk/data/database/Repo.kt +++ b/app/src/main/java/com/topjohnwu/magisk/data/database/Repo.kt @@ -11,6 +11,9 @@ interface RepoBase { fun getRepos(offset: Int, limit: Int = LIMIT): List fun searchRepos(query: String, offset: Int, limit: Int = LIMIT): List + @Query("SELECT * FROM repos WHERE id = :id AND versionCode > :versionCode LIMIT 1") + fun getUpdatableRepoById(id: String, versionCode: Int): Repo? + companion object { const val LIMIT = 10 } diff --git a/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/ModuleRvItem.kt b/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/ModuleRvItem.kt index 8a76ee431..a6c1e4037 100644 --- a/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/ModuleRvItem.kt +++ b/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/ModuleRvItem.kt @@ -95,20 +95,24 @@ class SectionTitle( ) : ObservableItem() { override val layoutRes = R.layout.item_section_md2 - @Bindable var button = _button + @Bindable get set(value) { field = value notifyChange(BR.button) } - @Bindable var icon = _icon + @Bindable get set(value) { field = value notifyChange(BR.icon) } - - val hasButton = KObservableField(button != 0 || icon != 0) + var hasButton = button != 0 || icon != 0 + @Bindable get + set(value) { + field = value + notifyChange(BR.hasButton) + } override fun onBindingBound(binding: ViewDataBinding) { super.onBindingBound(binding) @@ -120,14 +124,27 @@ class SectionTitle( override fun contentSameAs(other: SectionTitle): Boolean = this === other } -class RepoItem(val item: Repo) : ComparableRvItem() { +sealed class RepoItem(val item: Repo) : ObservableItem() { override val layoutRes: Int = R.layout.item_repo_md2 val progress = KObservableField(0) - val isUpdate = KObservableField(false) + var isUpdate = false + @Bindable get + protected set(value) { + field = value + notifyChange(BR.update) + } override fun contentSameAs(other: RepoItem): Boolean = item == other.item override fun itemSameAs(other: RepoItem): Boolean = item.id == other.item.id + + class Update(item: Repo) : RepoItem(item) { + init { + isUpdate = true + } + } + + class Remote(item: Repo) : RepoItem(item) } class ModuleItem(val item: Module) : ObservableItem(), Observable { @@ -158,7 +175,7 @@ class ModuleItem(val item: Module) : ObservableItem(), Observable { fun delete(viewModel: ModuleViewModel) { isRemoved = !isRemoved - viewModel.moveToState() + viewModel.updateActiveState() } override fun contentSameAs(other: ModuleItem): Boolean = item.version == other.item.version diff --git a/app/src/main/java/com/topjohnwu/magisk/redesign/module/ModuleViewModel.kt b/app/src/main/java/com/topjohnwu/magisk/redesign/module/ModuleViewModel.kt index 585ffb8bf..ef0ab6a8a 100644 --- a/app/src/main/java/com/topjohnwu/magisk/redesign/module/ModuleViewModel.kt +++ b/app/src/main/java/com/topjohnwu/magisk/redesign/module/ModuleViewModel.kt @@ -33,7 +33,6 @@ import io.reactivex.Single import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.disposables.Disposable import me.tatarka.bindingcollectionadapter2.BindingRecyclerViewAdapter -import org.jetbrains.annotations.NotNull import timber.log.Timber import kotlin.math.roundToInt @@ -70,13 +69,23 @@ class ModuleViewModel( } companion object { - private val sectionRemote = - SectionTitle(R.string.module_section_remote, R.string.sorting_order) + private val sectionRemote = SectionTitle( + R.string.module_section_remote, + R.string.sorting_order + ) + + private val sectionUpdate = SectionTitle( + R.string.module_section_pending, + R.string.module_section_pending_action, + R.drawable.ic_update_md2 + // enable with implementation of https://github.com/topjohnwu/Magisk/issues/2036 + ).also { it.hasButton = false } + private val sectionActive = SectionTitle( R.string.module_section_installed, R.string.reboot, R.drawable.ic_restart - ).also { it.hasButton.value = false } + ).also { it.hasButton = false } init { updateOrderIcon() @@ -94,11 +103,13 @@ class ModuleViewModel( // --- private val itemsInstalled - @WorkerThread get() = items.asSequence() - .filterIsInstance() - .toList() + @WorkerThread get() = items.filterIsInstance() + + private val itemsUpdatable: List + @WorkerThread get() = items.filterIsInstance() + private val itemsRemote - @WorkerThread get() = items.filterIsInstance() + @WorkerThread get() = items.filterIsInstance() private var remoteJob: Disposable? = null private val dao @@ -126,19 +137,15 @@ class ModuleViewModel( override fun refresh() = Single.fromCallable { Module.loadModules() } .map { it.map { ModuleItem(it) } } .map { it.order() } - .map { build(active = it) } + .map { build(active = it, updatable = loadUpdates(it)) } .map { it to items.calculateDiff(it) } .applyViewModel(this) .subscribeK { items.update(it.first, it.second) if (!items.contains(sectionRemote)) { loadRemote() - } else { - Single.fromCallable { itemsRemote } - .subscribeK { it.ensureUpdateState() } - .add() } - moveToState() + updateActiveState() } fun loadRemoteImplicit() = let { items.clear(); itemsSearch.clear() } @@ -181,8 +188,7 @@ class ModuleViewModel( .subscribeOn(AndroidSchedulers.mainThread()) } return Single.fromCallable { dao.searchRepos(query, offset) } - .map { it.map { RepoItem(it) } } - .doOnSuccess { it.ensureUpdateState() } + .map { it.map { RepoItem.Remote(it) } } } private fun query(query: String = this.query, offset: Int = 0) { @@ -207,7 +213,7 @@ class ModuleViewModel( offset: Int = 0, downloadRepos: Boolean = offset == 0 ): Single> = Single.fromCallable { dao.getRepos(offset) } - .map { it.map { RepoItem(it) } } + .map { it.map { RepoItem.Remote(it) } } .flatMap { when { // in case we find result empty and offset is initial we need to refresh the repos. @@ -216,7 +222,6 @@ class ModuleViewModel( else -> Single.just(it) } } - .doOnSuccess { it.ensureUpdateState() } private fun downloadRepos() = Single.just(Unit) .flatMap { repoUpdater() } @@ -237,56 +242,15 @@ class ModuleViewModel( // --- - /** - * Dynamically allocated list of [itemsInstalled]. It might be invalidated any time on any - * thread hence it needs to be volatile. - * - * There might be a state where this field gets assigned `null` whilst being used by another - * instance of any job, so the list will be immediately reinstated back. - * - * ### Note: - * - * It is caller's responsibility to invalidate this variable at the end of every job to save - * memory. - * */ - @Volatile - private var cachedItemsInstalled: List? = null - @WorkerThread @NotNull get() = field ?: itemsInstalled.also { field = it } - - private val Repo.isUpdatable: Boolean - @WorkerThread get() { - val installed = cachedItemsInstalled!! - .firstOrNull { it.item.id == id } - ?: return false - return installed.item.versionCode < versionCode - } - - /** - * Asynchronously updates state of all repo items so the loading speed is not impaired by this - * seemingly unnecessary operation. Because of the nature of this operation, the "update" status - * is not guaranteed for all items and can change any time. - * - * It is permitted running this function in parallel; it will also attempt to run in parallel - * by itself to finish the job as quickly as possible. - * - * No list manipulations should be done in this method whatsoever! By being heavily parallelized - * is will inevitably throw exceptions by simultaneously accessing the same list. - * - * In order to save time it uses helper [cachedItemsInstalled]. - * */ - private fun List.ensureUpdateState() = Single.just(this) - .flattenAsFlowable { it } - .parallel() - .map { it to it.item.isUpdatable } - .sequential() - .doOnComplete { cachedItemsInstalled = null } - .subscribeK { it.first.isUpdate.value = it.second } - .add() + @WorkerThread + private fun loadUpdates(installed: List) = installed + .mapNotNull { dao.getUpdatableRepoById(it.item.id, it.item.versionCode) } + .map { RepoItem.Update(it) } // --- - fun moveToState() = Single.fromCallable { itemsInstalled.any { it.isModified } } - .subscribeK { sectionActive.hasButton.value = it } + fun updateActiveState() = Single.fromCallable { itemsInstalled.any { it.isModified } } + .subscribeK { sectionActive.hasButton = it } .add() fun sectionPressed(item: SectionTitle) = when (item) { @@ -318,8 +282,10 @@ class ModuleViewModel( @WorkerThread private fun build( active: List = itemsInstalled, + updatable: List = itemsUpdatable, remote: List = itemsRemote ) = (active + InstallModule).prependIfNotEmpty { sectionActive } + + updatable.prependIfNotEmpty { sectionUpdate } + remote.prependIfNotEmpty { sectionRemote } private fun List.prependIfNotEmpty(item: () -> T) = diff --git a/app/src/main/res/layout/item_repo_md2.xml b/app/src/main/res/layout/item_repo_md2.xml index 3c76aed55..0f01b4092 100644 --- a/app/src/main/res/layout/item_repo_md2.xml +++ b/app/src/main/res/layout/item_repo_md2.xml @@ -110,7 +110,7 @@ android:paddingStart="@dimen/l_50" isEnabled="@{!(item.progress == -100 || (item.progress > 0 && item.progress < 100))}" android:contentDescription="@string/download" - srcCompat="@{item.isUpdate() ? R.drawable.ic_update_md2 : R.drawable.ic_download_md2}" + srcCompat="@{item.isUpdate ? R.drawable.ic_update_md2 : R.drawable.ic_download_md2}" app:layout_constraintBottom_toBottomOf="parent" app:layout_constraintEnd_toEndOf="parent" app:layout_constraintTop_toBottomOf="@+id/module_divider" diff --git a/app/src/main/res/values/strings_md2.xml b/app/src/main/res/values/strings_md2.xml index 1a9fa8ab5..11bca52c2 100644 --- a/app/src/main/res/values/strings_md2.xml +++ b/app/src/main/res/values/strings_md2.xml @@ -75,7 +75,8 @@ You\'re in safe mode. None of user modules will work.\nThis message will disappear once safe mode is disabled. %1$s by %2$s - Pending changes + Updates + Update all Installed Remote Remove diff --git a/build.gradle b/build.gradle index f0236679e..13d3e1626 100644 --- a/build.gradle +++ b/build.gradle @@ -7,7 +7,7 @@ if (configPath.exists()) configPath.withInputStream { is -> props.load(is) } buildscript { - ext.vKotlin = '1.3.50' + ext.vKotlin = '1.3.60' repositories { google()