From 79aa261ca2a9d4af43f84f9f4afaa3b2d043a313 Mon Sep 17 00:00:00 2001
From: Viktor De Pasquale <info@depasquale.wiki>
Date: Wed, 12 Aug 2020 18:42:54 +0200
Subject: [PATCH] Fixed manager beginning to hide immediately on field change

Bug was caused by lenient usage of "value" property defined in the "line item" in settings. Developer error allowed to use the internal value, that was not properly protected, in a way that did not conform with the latest "Observer" rewrite.

Additional comments were added to hopefully prevent bugs of this kind in the future. The property is now properly protected so it gives away clues that this access is considered "not cool".
---
 .../model/entity/recycler/SettingsItem.kt     | 15 +++++++++-
 .../magisk/ui/settings/SettingsItems.kt       | 28 +++++++++++--------
 .../res/layout/dialog_settings_app_name.xml   |  2 +-
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/SettingsItem.kt b/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/SettingsItem.kt
index 0a4b679e2..1ed2d4269 100644
--- a/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/SettingsItem.kt
+++ b/app/src/main/java/com/topjohnwu/magisk/model/entity/recycler/SettingsItem.kt
@@ -60,7 +60,20 @@ sealed class SettingsItem : ObservableItem<SettingsItem>() {
 
     abstract class Value<T> : SettingsItem() {
 
+        /**
+         * Represents last agreed-upon value by the validation process and the user for current
+         * child. Be very aware that this shouldn't be **set** unless both sides agreed that _that_
+         * is the new value.
+         *
+         * Annotating [value] as [Bindable] property should raise red flags immediately. If you
+         * need a [Bindable] property create another one. Seriously.
+         * */
         abstract var value: T
+            /**
+             * We don't want this to be accessible to be set from outside the instances. It will
+             * introduce unwanted bugs!
+             * */
+            protected set
 
         protected var callbackVars: Pair<View, Callback>? = null
 
@@ -76,7 +89,7 @@ sealed class SettingsItem : ObservableItem<SettingsItem>() {
 
         protected inline fun <reified T> setV(
             new: T, old: T, setter: (T) -> Unit, afterChanged: (T) -> Unit = {}) {
-            set(new, old, setter, BR.value, BR.description, BR.checked) {
+            set(new, old, setter, BR.description, BR.checked) {
                 afterChanged(it)
                 callbackVars?.let { (view, callback) ->
                     callbackVars = null
diff --git a/app/src/main/java/com/topjohnwu/magisk/ui/settings/SettingsItems.kt b/app/src/main/java/com/topjohnwu/magisk/ui/settings/SettingsItems.kt
index 4060650c9..bd9277818 100644
--- a/app/src/main/java/com/topjohnwu/magisk/ui/settings/SettingsItems.kt
+++ b/app/src/main/java/com/topjohnwu/magisk/ui/settings/SettingsItems.kt
@@ -80,19 +80,23 @@ object Hide : SettingsItem.Input() {
     override val title = R.string.settings_hide_manager_title.asTransitive()
     override val description = R.string.settings_hide_manager_summary.asTransitive()
 
-    @get:Bindable
     override var value = "Manager"
-        set(value) = setV(value, field, { field = it }) {
-            notifyPropertyChanged(BR.error)
-        }
+        set(value) = setV(value, field, { field = it })
+
+    override val inputResult
+        get() = if (isError) null else result
 
     @get:Bindable
-    val isError get() = value.length > 14 || value.isBlank()
+    var result = value
+        set(value) = set(value, field, { field = it }, BR.result, BR.error)
 
-    override val inputResult get() = if (isError) null else value
+    @get:Bindable
+    val isError
+        get() = value.length > 14 || value.isBlank()
 
     override fun getView(context: Context) = DialogSettingsAppNameBinding
         .inflate(LayoutInflater.from(context)).also { it.data = this }.root
+
 }
 
 object Restore : SettingsItem.Blank() {
@@ -119,20 +123,22 @@ object DownloadPath : SettingsItem.Input() {
         set(value) = set(value, field, { field = it }, BR.result, BR.path)
 
     @get:Bindable
-    val path get() = File(Environment.getExternalStorageDirectory(), result).absolutePath.orEmpty()
+    val path
+        get() = File(Environment.getExternalStorageDirectory(), result).absolutePath.orEmpty()
 
     override fun getView(context: Context) = DialogSettingsDownloadPathBinding
         .inflate(LayoutInflater.from(context)).also { it.data = this }.root
 }
 
 object UpdateChannel : SettingsItem.Selector() {
-    override var value  = Config.updateChannel
+    override var value = Config.updateChannel
         set(value) = setV(value, field, { field = it }) { Config.updateChannel = it }
 
     override val title = R.string.settings_update_channel_title.asTransitive()
-    override val entries get() = resources.getStringArray(R.array.update_channel).let {
-        if (BuildConfig.DEBUG) it.toMutableList().apply { add("Canary") }.toTypedArray() else it
-    }
+    override val entries
+        get() = resources.getStringArray(R.array.update_channel).let {
+            if (BuildConfig.DEBUG) it.toMutableList().apply { add("Canary") }.toTypedArray() else it
+        }
     override val entryValRes = R.array.value_array
 }
 
diff --git a/app/src/main/res/layout/dialog_settings_app_name.xml b/app/src/main/res/layout/dialog_settings_app_name.xml
index 872616bf8..eb2266465 100644
--- a/app/src/main/res/layout/dialog_settings_app_name.xml
+++ b/app/src/main/res/layout/dialog_settings_app_name.xml
@@ -40,7 +40,7 @@
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
                 android:inputType="textCapWords"
-                android:text="@={data.value}"
+                android:text="@={data.result}"
                 android:textAppearance="@style/AppearanceFoundation.Body"
                 android:textColor="?colorOnSurface"
                 tools:text="@tools:sample/lorem" />