From bb0d3be51e24eb714145bc3d013f826a97151347 Mon Sep 17 00:00:00 2001 From: Alessandro Sangiorgi Date: Wed, 22 Apr 2026 15:01:45 -0500 Subject: [PATCH 1/3] Handle cancellation in installation method selection - Reset selected installation method when file picking or URL entry is cancelled. - Fix a bug where `RadioGroup` would trigger spurious selection events when clearing the selection programmatically. - Use a static `LiveData` to safely communicate file selection results across `ViewModel` instances after process death. - Add `onActivityCancel` to `ContentResultCallback` to properly handle activity results with no data. --- .../topjohnwu/magisk/dialog/DownloadDialog.kt | 11 +++- .../magisk/ui/install/InstallViewModel.kt | 55 +++++++++++++++++-- .../magisk/core/base/BaseActivity.kt | 2 + 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/app/apk/src/main/java/com/topjohnwu/magisk/dialog/DownloadDialog.kt b/app/apk/src/main/java/com/topjohnwu/magisk/dialog/DownloadDialog.kt index d8d48f6f27d91..6a78c9b14c623 100644 --- a/app/apk/src/main/java/com/topjohnwu/magisk/dialog/DownloadDialog.kt +++ b/app/apk/src/main/java/com/topjohnwu/magisk/dialog/DownloadDialog.kt @@ -8,7 +8,10 @@ import com.topjohnwu.magisk.core.R import com.topjohnwu.magisk.events.DialogBuilder import com.topjohnwu.magisk.view.MagiskDialog -class DownloadDialog(private val callback: (Uri) -> Unit) : DialogBuilder { +class DownloadDialog( + private val callback: (Uri) -> Unit, + private val onCancel: (() -> Unit)? = null, +) : DialogBuilder { override fun build(dialog: MagiskDialog) { val editText = EditText(dialog.context).apply { @@ -17,6 +20,8 @@ class DownloadDialog(private val callback: (Uri) -> Unit) : DialogBuilder { requestFocus() } + var confirmed = false + dialog.apply { setTitle(R.string.download_dialog_title) setView(editText) @@ -26,6 +31,7 @@ class DownloadDialog(private val callback: (Uri) -> Unit) : DialogBuilder { val url = editText.text.toString().trim() isValidUrl(url)?.let { doNotDismiss = false + confirmed = true callback(it) } ?: run { doNotDismiss = true @@ -37,6 +43,9 @@ class DownloadDialog(private val callback: (Uri) -> Unit) : DialogBuilder { text = android.R.string.cancel } setCancelable(true) + setOnDismissListener { + if (!confirmed) onCancel?.invoke() + } } } diff --git a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt index b6a64dffd3470..e516371ba627e 100644 --- a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt +++ b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt @@ -9,6 +9,7 @@ import android.widget.Toast import androidx.databinding.Bindable import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData +import androidx.lifecycle.Observer import androidx.lifecycle.viewModelScope import com.topjohnwu.magisk.BR import com.topjohnwu.magisk.R @@ -46,17 +47,29 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() set(value) = set(value, field, { field = it }, BR.step) private var methodId = -1 + // RadioGroup fires its OnCheckedChangeListener with the previously-checked id + // right before firing it with -1 when we clear the selection programmatically. + // Track the id we expect to see in that spurious callback so the setter's + // side-effect lambda can ignore it. + private var spuriousMethodId = -1 @get:Bindable var method get() = methodId set(value) = set(value, methodId, { methodId = it }, BR.method) { + if (it == spuriousMethodId) { + spuriousMethodId = -1 + return@set + } when (it) { R.id.method_patch -> { GetContentEvent("*/*", UriCallback()).publish() } R.id.method_download -> { - DownloadDialog { url -> uri.value = url }.show() + DownloadDialog( + callback = { url -> _uri.value = url }, + onCancel = { resetMethod() }, + ).show() } R.id.method_inactive_slot -> { SecondSlotWarningDialog().show() @@ -64,13 +77,33 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() } } - val data: LiveData get() = uri + private fun resetMethod() { + spuriousMethodId = methodId + method = -1 + } + + private val _uri = MutableLiveData() + val data: LiveData get() = _uri + + // UriCallback is @Parcelize'd and may outlive any single ViewModel instance + // (it's restored from saved state after process death), so we route its + // results through the static `patchSource` and observe it here. This keeps + // the cross-instance coupling behind a single LiveData instead of a raw + // companion-object lambda. + private val sourceObserver = Observer { source -> + when (source) { + is PatchSource.File -> _uri.value = source.uri + PatchSource.Cancelled -> resetMethod() + null -> Unit + } + } @get:Bindable var notes: Spanned = SpannedString("") set(value) = set(value, field, { field = it }, BR.notes) init { + patchSource.observeForever(sourceObserver) viewModelScope.launch(Dispatchers.IO) { try { val noteFile = File(AppContext.cacheDir, "${APP_VERSION_CODE}.md") @@ -125,6 +158,16 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() } } + override fun onCleared() { + patchSource.removeObserver(sourceObserver) + super.onCleared() + } + + private sealed interface PatchSource { + data class File(val uri: Uri) : PatchSource + data object Cancelled : PatchSource + } + @Parcelize class UriCallback : ContentResultCallback { override fun onActivityLaunch() { @@ -132,7 +175,11 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() } override fun onActivityResult(result: Uri) { - uri.value = result + patchSource.value = PatchSource.File(result) + } + + override fun onActivityCancel() { + patchSource.value = PatchSource.Cancelled } } @@ -147,6 +194,6 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() companion object { private const val INSTALL_STATE_KEY = "install_state" - private val uri = MutableLiveData() + private val patchSource = MutableLiveData() } } diff --git a/app/core/src/main/java/com/topjohnwu/magisk/core/base/BaseActivity.kt b/app/core/src/main/java/com/topjohnwu/magisk/core/base/BaseActivity.kt index 7bd9e4440460a..74c1b5e2a056e 100644 --- a/app/core/src/main/java/com/topjohnwu/magisk/core/base/BaseActivity.kt +++ b/app/core/src/main/java/com/topjohnwu/magisk/core/base/BaseActivity.kt @@ -23,6 +23,7 @@ import com.topjohnwu.magisk.core.utils.RequestInstall interface ContentResultCallback: ActivityResultCallback, Parcelable { fun onActivityLaunch() {} + fun onActivityCancel() {} // Make the result type explicitly non-null override fun onActivityResult(result: Uri) } @@ -65,6 +66,7 @@ class ActivityExtension(private val activity: ComponentActivity) { private var contentCallback: ContentResultCallback? = null private val getContent = activity.registerForActivityResult(GetContent()) { if (it != null) contentCallback?.onActivityResult(it) + else contentCallback?.onActivityCancel() contentCallback = null } From 37f4f2dcf0bb24c8a21f610323b7b88cadd2aed1 Mon Sep 17 00:00:00 2001 From: Alessandro Sangiorgi Date: Wed, 22 Apr 2026 15:09:13 -0500 Subject: [PATCH 2/3] Reset patch source after selection in InstallViewModel --- .../java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt index e516371ba627e..de1c594c08277 100644 --- a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt +++ b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt @@ -94,8 +94,11 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() when (source) { is PatchSource.File -> _uri.value = source.uri PatchSource.Cancelled -> resetMethod() - null -> Unit + null -> return@Observer } + // Consume the result so a later VM instance doesn't replay a stale + // selection/cancel from a previous session. + patchSource.value = null } @get:Bindable From 91ebaf672030a43f080c33f9bb409b54677f415a Mon Sep 17 00:00:00 2001 From: Alessandro Sangiorgi Date: Wed, 22 Apr 2026 15:15:43 -0500 Subject: [PATCH 3/3] remove verbose comments --- .../topjohnwu/magisk/ui/install/InstallViewModel.kt | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt index de1c594c08277..40e32396291da 100644 --- a/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt +++ b/app/apk/src/main/java/com/topjohnwu/magisk/ui/install/InstallViewModel.kt @@ -47,10 +47,6 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() set(value) = set(value, field, { field = it }, BR.step) private var methodId = -1 - // RadioGroup fires its OnCheckedChangeListener with the previously-checked id - // right before firing it with -1 when we clear the selection programmatically. - // Track the id we expect to see in that spurious callback so the setter's - // side-effect lambda can ignore it. private var spuriousMethodId = -1 @get:Bindable @@ -85,19 +81,12 @@ class InstallViewModel(svc: NetworkService, markwon: Markwon) : BaseViewModel() private val _uri = MutableLiveData() val data: LiveData get() = _uri - // UriCallback is @Parcelize'd and may outlive any single ViewModel instance - // (it's restored from saved state after process death), so we route its - // results through the static `patchSource` and observe it here. This keeps - // the cross-instance coupling behind a single LiveData instead of a raw - // companion-object lambda. private val sourceObserver = Observer { source -> when (source) { is PatchSource.File -> _uri.value = source.uri PatchSource.Cancelled -> resetMethod() null -> return@Observer } - // Consume the result so a later VM instance doesn't replay a stale - // selection/cancel from a previous session. patchSource.value = null }