Fix custom dns toggle not working after resume

Fixes an issue with the custom dns toggle and server list not not
working after the app is resumed, due to the logic not being aware
of new service connections.

The issue was fixed by moving some responsibility from the adapter
to the fragment and making sure new service connections triggers new
data subscriptions to be set up.

2 files changed, 92 insertions, 64 deletions

diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AdvancedFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AdvancedFragment.kt
index 6f1568327d..33fbd295ea 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AdvancedFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AdvancedFragment.kt
@@ -11,6 +11,7 @@ import net.mullvad.mullvadvpn.R
 import net.mullvad.mullvadvpn.model.Settings
 import net.mullvad.mullvadvpn.ui.customdns.CustomDnsAdapter
 import net.mullvad.mullvadvpn.ui.fragments.SplitTunnelingFragment
+import net.mullvad.mullvadvpn.ui.serviceconnection.ServiceConnection
 import net.mullvad.mullvadvpn.ui.widget.CellSwitch
 import net.mullvad.mullvadvpn.ui.widget.CustomRecyclerView
 import net.mullvad.mullvadvpn.ui.widget.MtuCell
@@ -21,8 +22,11 @@ import net.mullvad.mullvadvpn.util.AdapterWithHeader
 class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
     private var isAllowLanEnabled = false
 
-    private lateinit var customDnsAdapter: CustomDnsAdapter
-    private lateinit var customDnsToggle: ToggleCell
+    // Both customDnsAdapter and customDnsToggle are nullable since onNewServiceConnection,
+    // which sets up custom dns subscriptions, is called before onSafelyCreateView.
+    private var customDnsAdapter: CustomDnsAdapter? = null
+    private var customDnsToggle: ToggleCell? = null
+
     private lateinit var wireguardMtuInput: MtuCell
     private lateinit var titleController: CollapsibleTitleController
 
@@ -34,31 +38,45 @@ class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
         val view = inflater.inflate(R.layout.advanced, container, false)
 
         view.findViewById<View>(R.id.back).setOnClickListener {
-            customDnsAdapter.stopEditing()
+            customDnsAdapter?.stopEditing()
             parentActivity.onBackPressed()
         }
 
         titleController = CollapsibleTitleController(view, R.id.contents)
 
-        customDnsAdapter = CustomDnsAdapter(customDns).apply {
-            confirmAddAddress = ::confirmAddAddress
-        }
+        customDnsAdapter = CustomDnsAdapter(
+            onAddServer = { address -> customDns.addDnsServer(address) },
+            onRemoveDnsServer = { address -> customDns.removeDnsServer(address) },
+            onSetCustomDnsEnabled = { isEnabled ->
+                if (isEnabled) {
+                    customDns.enable()
+                } else {
+                    customDns.disable()
+                }
+            },
+            onReplaceDnsServer = { oldServer, newServer ->
+                customDns.replaceDnsServer(oldServer, newServer)
+            }
+        ).also { newCustomDnsAdapter ->
+            newCustomDnsAdapter.confirmAddAddress = ::confirmAddAddress
 
-        view.findViewById<CustomRecyclerView>(R.id.contents).apply {
-            layoutManager = LinearLayoutManager(parentActivity)
+            view.findViewById<CustomRecyclerView>(R.id.contents).apply {
+                layoutManager = LinearLayoutManager(parentActivity)
 
-            adapter = AdapterWithHeader(customDnsAdapter, R.layout.advanced_header).apply {
-                onHeaderAvailable = { headerView ->
-                    configureHeader(headerView)
-                    titleController.expandedTitleView = headerView.findViewById(R.id.expanded_title)
+                adapter = AdapterWithHeader(newCustomDnsAdapter, R.layout.advanced_header).apply {
+                    onHeaderAvailable = { headerView ->
+                        configureHeader(headerView)
+                        titleController.expandedTitleView =
+                            headerView.findViewById(R.id.expanded_title)
+                    }
                 }
-            }
 
-            addItemDecoration(
-                ListItemDividerDecoration(
-                    topOffset = resources.getDimensionPixelSize(R.dimen.list_item_divider)
+                addItemDecoration(
+                    ListItemDividerDecoration(
+                        topOffset = resources.getDimensionPixelSize(R.dimen.list_item_divider)
+                    )
                 )
-            )
+            }
         }
 
         attachBackButtonHandler()
@@ -66,9 +84,14 @@ class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
         return view
     }
 
+    override fun onNewServiceConnection(serviceConnection: ServiceConnection) {
+        super.onNewServiceConnection(serviceConnection)
+        subscribeToCustomDnsChanges()
+    }
+
     override fun onSafelyDestroyView() {
         detachBackButtonHandler()
-        customDnsAdapter.onDestroy()
+        customDnsAdapter?.onDestroy()
         titleController.onDestroy()
         settingsListener.settingsNotifier.unsubscribe(this)
     }
@@ -98,16 +121,6 @@ class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
             }
         }
 
-        customDns.onEnabledChanged.subscribe(this) { enabled ->
-            jobTracker.newUiJob("updateEnabled") {
-                if (enabled) {
-                    customDnsToggle.state = CellSwitch.State.ON
-                } else {
-                    customDnsToggle.state = CellSwitch.State.OFF
-                }
-            }
-        }
-
         settingsListener.settingsNotifier.subscribe(this) { maybeSettings ->
             maybeSettings?.let { settings ->
                 updateUi(settings)
@@ -115,6 +128,30 @@ class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
 
             isAllowLanEnabled = maybeSettings?.allowLan ?: false
         }
+
+        subscribeToCustomDnsChanges()
+    }
+
+    private fun subscribeToCustomDnsChanges() {
+        // Ensure there are no previous subscriptions as this function might be called either when
+        // there view has been created or when there is a new service connection.
+        customDns.onEnabledChanged.unsubscribe(this)
+        customDns.onDnsServersChanged.unsubscribe(this)
+
+        customDns.onEnabledChanged.subscribe(this) { isEnabled ->
+            customDnsAdapter?.updateState(isEnabled)
+            jobTracker.newUiJob("updateEnabled") {
+                if (isEnabled) {
+                    customDnsToggle?.state = CellSwitch.State.ON
+                } else {
+                    customDnsToggle?.state = CellSwitch.State.OFF
+                }
+            }
+        }
+
+        customDns.onDnsServersChanged.subscribe(this) { servers ->
+            customDnsAdapter?.updateServers(servers)
+        }
     }
 
     private fun updateUi(settings: Settings) {
@@ -150,8 +187,8 @@ class AdvancedFragment : ServiceDependentFragment(OnNoService.GoBack) {
 
     private fun attachBackButtonHandler() {
         parentActivity.backButtonHandler = {
-            if (customDnsAdapter.isEditing) {
-                customDnsAdapter.stopEditing()
+            if (customDnsAdapter?.isEditing == true) {
+                customDnsAdapter?.stopEditing()
             }
             false
         }
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/customdns/CustomDnsAdapter.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/customdns/CustomDnsAdapter.kt
index 1c8b36e062..1ab1429b49 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/customdns/CustomDnsAdapter.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/customdns/CustomDnsAdapter.kt
@@ -8,11 +8,15 @@ import kotlin.properties.Delegates.observable
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import net.mullvad.mullvadvpn.R
-import net.mullvad.mullvadvpn.ui.serviceconnection.CustomDns
 import net.mullvad.mullvadvpn.util.JobTracker
 import org.apache.commons.validator.routines.InetAddressValidator
 
-class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>() {
+class CustomDnsAdapter(
+    val onSetCustomDnsEnabled: (Boolean) -> Unit,
+    val onAddServer: (InetAddress) -> Boolean,
+    val onRemoveDnsServer: (InetAddress) -> Unit,
+    val onReplaceDnsServer: (InetAddress, InetAddress) -> Boolean
+) : Adapter<CustomDnsItemHolder>() {
     private enum class ViewTypes {
         ADD_SERVER,
         EDIT_SERVER,
@@ -40,10 +44,6 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
         if (oldValue != newValue) {
             if (newValue == true) {
                 notifyItemRangeInserted(0, cachedCustomDnsServers.size + 1)
-
-                if (cachedCustomDnsServers.isEmpty()) {
-                    editingPosition = 0
-                }
             } else {
                 notifyItemRangeRemoved(0, cachedCustomDnsServers.size + 1)
                 editingPosition = null
@@ -57,28 +57,24 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
     // By default, refuse the address so that the dialog can be recreated by the user if needed
     var confirmAddAddress: suspend (InetAddress) -> Boolean = { false }
 
-    init {
-        customDns.apply {
-            onDnsServersChanged.subscribe(this) { dnsServers ->
-                jobTracker.newBackgroundJob("toggleCustomDns") {
-                    if (dnsServers.isEmpty()) {
-                        customDns.disable()
-                    }
-                }
+    fun updateServers(servers: List<InetAddress>) {
+        jobTracker.newBackgroundJob("toggleCustomDns") {
+            if (servers.isEmpty()) {
+                onSetCustomDnsEnabled(false)
+            }
+        }
 
-                jobTracker.newUiJob("updateDnsServers") {
-                    customDnsServersLock.withLock {
-                        activeCustomDnsServers = dnsServers
-                    }
-                }
+        jobTracker.newUiJob("updateDnsServers") {
+            customDnsServersLock.withLock {
+                activeCustomDnsServers = servers
             }
+        }
+    }
 
-            onEnabledChanged.subscribe(this) { value ->
-                jobTracker.newUiJob("updateEnabled") {
-                    customDnsServersLock.withLock {
-                        enabled = value
-                    }
-                }
+    fun updateState(isEnabled: Boolean) {
+        jobTracker.newUiJob("updateEnabled") {
+            customDnsServersLock.withLock {
+                enabled = isEnabled
             }
         }
     }
@@ -108,7 +104,6 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
 
     override fun onCreateViewHolder(parentView: ViewGroup, type: Int): CustomDnsItemHolder {
         val inflater = LayoutInflater.from(parentView.context)
-
         when (ViewTypes.values()[type]) {
             ViewTypes.FOOTER -> {
                 val view = inflater.inflate(R.layout.custom_dns_footer, parentView, false)
@@ -144,13 +139,9 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
     fun onDestroy() {
         jobTracker.newBackgroundJob("toggleCustomDns") {
             if (cachedCustomDnsServers.isEmpty()) {
-                customDns.disable()
+                onSetCustomDnsEnabled(false)
             }
         }
-        customDns.apply {
-            onDnsServersChanged.unsubscribe(this)
-            onEnabledChanged.unsubscribe(this)
-        }
     }
 
     fun newDnsServer() {
@@ -227,7 +218,7 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
                 val position = jobTracker.runOnBackground {
                     val index = cachedCustomDnsServers.indexOf(address)
                     cachedCustomDnsServers.removeAt(index)
-                    customDns.removeDnsServer(address)
+                    onRemoveDnsServer(address)
                     index
                 }
 
@@ -246,7 +237,7 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
         var added = false
 
         withValidAddress(addressText) { address ->
-            if (customDns.addDnsServer(address)) {
+            if (onAddServer(address)) {
                 cachedCustomDnsServers.add(address)
                 added = true
             }
@@ -270,7 +261,7 @@ class CustomDnsAdapter(val customDns: CustomDns) : Adapter<CustomDnsItemHolder>(
         withValidAddress(address) { newAddress ->
             val oldAddress = cachedCustomDnsServers[position]
 
-            if (customDns.replaceDnsServer(oldAddress, newAddress)) {
+            if (onReplaceDnsServer(oldAddress, newAddress)) {
                 cachedCustomDnsServers[position] = newAddress
                 replaced = true
             }