refactor: implement repository pattern for `NodeDB` (#835)

- enforce Unidirectional Data Flow removing nodeDB updates via `MainActivity`/`UIState` - merge `MyNodeInfoDao` into `NodeInfoDao` - move node list re-indexing to database
2024-02-06 20:03:15 -03:00 · 2024-02-06 20:03:15 -03:00 · c8f93db00d
commit c8f93db00d
--- a/app/src/androidTest/java/com/geeksville/mesh/NodeInfoDaoTest.kt
+++ b/app/src/androidTest/java/com/geeksville/mesh/NodeInfoDaoTest.kt
@ -0,0 +1,104 @@
+package com.geeksville.mesh
+
+import androidx.room.Room
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import com.geeksville.mesh.database.MeshtasticDatabase
+import com.geeksville.mesh.database.dao.NodeInfoDao
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.runBlocking
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class NodeDBTest {
+    private lateinit var database: MeshtasticDatabase
+    private lateinit var nodeInfoDao: NodeInfoDao
+
+    private val testNodeNoPosition = NodeInfo(
+        8,
+        MeshUser(
+            "+16508765308".format(8),
+            "Kevin MesterNoLoc",
+            "KLO",
+            MeshProtos.HardwareModel.ANDROID_SIM,
+            false
+        ),
+        null
+    )
+
+    private val myNodeInfo: MyNodeInfo = MyNodeInfo(
+        myNodeNum = testNodeNoPosition.num,
+        hasGPS = false,
+        model = null,
+        firmwareVersion = null,
+        couldUpdate = false,
+        shouldUpdate = false,
+        currentPacketId = 1L,
+        messageTimeoutMsec = 5 * 60 * 1000,
+        minAppVersion = 1,
+        maxChannels = 8,
+        hasWifi = false,
+        channelUtilization = 0f,
+        airUtilTx = 0f,
+    )
+
+    private val testPositions = arrayOf(
+        Position(32.776665, -96.796989, 35, 123), // dallas
+        Position(32.960758, -96.733521, 35, 456), // richardson
+        Position(32.912901, -96.781776, 35, 789), // north dallas
+    )
+
+    private val testNodes = listOf(testNodeNoPosition) + testPositions.mapIndexed { index, it ->
+        NodeInfo(
+            9 + index,
+            MeshUser(
+                "+165087653%02d".format(9 + index),
+                "Kevin Mester$index",
+                "KM$index",
+                MeshProtos.HardwareModel.ANDROID_SIM,
+                false
+            ),
+            it
+        )
+    }
+
+    @Before
+    fun createDb(): Unit = runBlocking {
+        val context = InstrumentationRegistry.getInstrumentation().targetContext
+        database = Room.inMemoryDatabaseBuilder(context, MeshtasticDatabase::class.java).build()
+        nodeInfoDao = database.nodeInfoDao()
+
+        nodeInfoDao.apply{
+            putAll(testNodes)
+            setMyNodeInfo(myNodeInfo)
+        }
+    }
+
+    @After
+    fun closeDb() {
+        database.close()
+    }
+
+    @Test // node list size
+    fun testNodeListSize() = runBlocking {
+        val nodes = nodeInfoDao.nodeDBbyNum().first()
+        assertEquals(nodes.size, 4)
+    }
+
+    @Test // nodeDBbyNum() re-orders our node at the top of the list
+    fun testOurNodeIntoIsFirst() = runBlocking {
+        val nodes = nodeInfoDao.nodeDBbyNum().first()
+        assertEquals(nodes.values.first(), testNodeNoPosition)
+    }
+
+    @Test // getNodeInto()
+    fun testGetNodeInto() = runBlocking {
+        for (node in nodeInfoDao.getNodes().first()) {
+            assertEquals(nodeInfoDao.getNodeInfo(node.num), node)
+        }
+    }
+}
--- a/app/src/main/java/com/geeksville/mesh/MainActivity.kt
+++ b/app/src/main/java/com/geeksville/mesh/MainActivity.kt
@ -340,8 +340,6 @@ class MainActivity : AppCompatActivity(), Logging {
                                else {
                                    // If our app is too old/new, we probably don't understand the new DeviceConfig messages, so we don't read them until here

-                                    model.updateNodesFromDevice()
-
                                    // we have a connection to our device now, do the channel change
                                    perhapsChangeChannel()
                                }
@ -453,11 +451,6 @@ class MainActivity : AppCompatActivity(), Logging {
                    val connectionState =
                        MeshService.ConnectionState.valueOf(service.connectionState())

-                    // if we are not connected, onMeshConnectionChange won't fetch nodes from the service
-                    // in that case, we do it here - because the service certainly has a better idea of node db that we have
-                    if (connectionState != MeshService.ConnectionState.CONNECTED)
-                        model.updateNodesFromDevice()
-
                    // We won't receive a notify for the initial state of connection, so we force an update here
                    onMeshConnectionChanged(connectionState)
                } catch (ex: RemoteException) {
--- a/app/src/main/java/com/geeksville/mesh/database/DatabaseModule.kt
+++ b/app/src/main/java/com/geeksville/mesh/database/DatabaseModule.kt
@ -2,7 +2,6 @@ package com.geeksville.mesh.database

 import android.app.Application
 import com.geeksville.mesh.database.dao.MeshLogDao
-import com.geeksville.mesh.database.dao.MyNodeInfoDao
 import com.geeksville.mesh.database.dao.NodeInfoDao
 import com.geeksville.mesh.database.dao.PacketDao
 import com.geeksville.mesh.database.dao.QuickChatActionDao
@ -20,11 +19,6 @@ class DatabaseModule {
    fun provideDatabase(app: Application): MeshtasticDatabase =
        MeshtasticDatabase.getDatabase(app)

-    @Provides
-    fun provideMyNodeInfoDao(database: MeshtasticDatabase): MyNodeInfoDao {
-        return database.myNodeInfoDao()
-    }
-
    @Provides
    fun provideNodeInfoDao(database: MeshtasticDatabase): NodeInfoDao {
        return database.nodeInfoDao()
--- a/app/src/main/java/com/geeksville/mesh/database/MeshtasticDatabase.kt
+++ b/app/src/main/java/com/geeksville/mesh/database/MeshtasticDatabase.kt
@ -10,7 +10,6 @@ import com.geeksville.mesh.MyNodeInfo
 import com.geeksville.mesh.NodeInfo
 import com.geeksville.mesh.database.dao.PacketDao
 import com.geeksville.mesh.database.dao.MeshLogDao
-import com.geeksville.mesh.database.dao.MyNodeInfoDao
 import com.geeksville.mesh.database.dao.NodeInfoDao
 import com.geeksville.mesh.database.dao.QuickChatActionDao
 import com.geeksville.mesh.database.entity.MeshLog
@ -33,7 +32,6 @@ import com.geeksville.mesh.database.entity.QuickChatAction
 )
@TypeConverters(Converters::class)
 abstract class MeshtasticDatabase : RoomDatabase() {
-    abstract fun myNodeInfoDao(): MyNodeInfoDao
    abstract fun nodeInfoDao(): NodeInfoDao
    abstract fun packetDao(): PacketDao
    abstract fun meshLogDao(): MeshLogDao
--- a/app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt
+++ b/app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt
@ -1,21 +0,0 @@
-package com.geeksville.mesh.database.dao
-
-import androidx.room.Dao
-import androidx.room.Insert
-import androidx.room.OnConflictStrategy
-import androidx.room.Query
-import com.geeksville.mesh.MyNodeInfo
-import kotlinx.coroutines.flow.Flow
-
-@Dao
-interface MyNodeInfoDao {
-
-    @Query("SELECT * FROM MyNodeInfo")
-    fun getMyNodeInfo(): Flow<MyNodeInfo?>
-
-    @Insert(onConflict = OnConflictStrategy.REPLACE)
-    fun setMyNodeInfo(myInfo: MyNodeInfo)
-
-    @Query("DELETE FROM MyNodeInfo")
-    fun clearMyNodeInfo()
-}
--- a/app/src/main/java/com/geeksville/mesh/database/dao/NodeInfoDao.kt
+++ b/app/src/main/java/com/geeksville/mesh/database/dao/NodeInfoDao.kt
@ -2,18 +2,35 @@ package com.geeksville.mesh.database.dao

 import androidx.room.Dao
 import androidx.room.Insert
+import androidx.room.MapColumn
 import androidx.room.OnConflictStrategy
 import androidx.room.Query
 import androidx.room.Upsert
+import com.geeksville.mesh.MyNodeInfo
 import com.geeksville.mesh.NodeInfo
 import kotlinx.coroutines.flow.Flow

@Dao
 interface NodeInfoDao {

+    @Query("SELECT * FROM MyNodeInfo")
+    fun getMyNodeInfo(): Flow<MyNodeInfo?>
+
+    @Insert(onConflict = OnConflictStrategy.REPLACE)
+    fun setMyNodeInfo(myInfo: MyNodeInfo)
+
+    @Query("DELETE FROM MyNodeInfo")
+    fun clearMyNodeInfo()
+
    @Query("SELECT * FROM NodeInfo")
    fun getNodes(): Flow<List<NodeInfo>>

+    @Query("SELECT * FROM NodeInfo ORDER BY CASE WHEN num = (SELECT myNodeNum FROM MyNodeInfo LIMIT 1) THEN 0 ELSE 1 END, num ASC")
+    fun nodeDBbyNum(): Flow<Map<@MapColumn(columnName = "num") Int, NodeInfo>>
+
+    @Query("SELECT * FROM NodeInfo")
+    fun nodeDBbyID(): Flow<Map<@MapColumn(columnName = "user_id") String, NodeInfo>>
+
    @Insert(onConflict = OnConflictStrategy.REPLACE)
    fun insert(node: NodeInfo)

--- a/app/src/main/java/com/geeksville/mesh/model/NodeDB.kt
+++ b/app/src/main/java/com/geeksville/mesh/model/NodeDB.kt
@ -1,100 +1,74 @@
 package com.geeksville.mesh.model

-import com.geeksville.mesh.MeshProtos
-import com.geeksville.mesh.MeshUser
+import androidx.lifecycle.Lifecycle
+import androidx.lifecycle.coroutineScope
 import com.geeksville.mesh.MyNodeInfo
 import com.geeksville.mesh.NodeInfo
-import com.geeksville.mesh.Position
-import com.geeksville.mesh.database.dao.MyNodeInfoDao
 import com.geeksville.mesh.database.dao.NodeInfoDao
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.launchIn
+import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.withContext
 import javax.inject.Inject
 import javax.inject.Singleton

@Singleton
 class NodeDB @Inject constructor(
-    private val myNodeInfoDao: MyNodeInfoDao,
+    processLifecycle: Lifecycle,
    private val nodeInfoDao: NodeInfoDao,
 ) {

-    fun myNodeInfoFlow(): Flow<MyNodeInfo?> = myNodeInfoDao.getMyNodeInfo()
-    private suspend fun setMyNodeInfo(myInfo: MyNodeInfo) = withContext(Dispatchers.IO) {
-        myNodeInfoDao.setMyNodeInfo(myInfo)
-    }
+    // hardware info about our local device (can be null)
+    private val _myNodeInfo = MutableStateFlow<MyNodeInfo?>(null)
+    val myNodeInfo: StateFlow<MyNodeInfo?> get() = _myNodeInfo

-    fun nodeInfoFlow(): Flow<List<NodeInfo>> = nodeInfoDao.getNodes()
-    suspend fun upsert(node: NodeInfo) = withContext(Dispatchers.IO) {
-        nodeInfoDao.upsert(node)
-    }
-
-    suspend fun installNodeDB(mi: MyNodeInfo, nodes: List<NodeInfo>) {
-        myNodeInfoDao.clearMyNodeInfo()
-        nodeInfoDao.clearNodeInfo()
-        nodeInfoDao.putAll(nodes)
-        setMyNodeInfo(mi) // set MyNodeInfo last
-    }
-
-    private val testPositions = arrayOf(
-        Position(32.776665, -96.796989, 35, 123), // dallas
-        Position(32.960758, -96.733521, 35, 456), // richardson
-        Position(32.912901, -96.781776, 35, 789), // north dallas
-    )
-
-    private val testNodeNoPosition = NodeInfo(
-        8,
-        MeshUser(
-            "+16508765308".format(8),
-            "Kevin MesterNoLoc",
-            "KLO",
-            MeshProtos.HardwareModel.ANDROID_SIM,
-            false
-        ),
-        null
-    )
-
-    private val testNodes = (listOf(testNodeNoPosition) + testPositions.mapIndexed { index, it ->
-        NodeInfo(
-            9 + index,
-            MeshUser(
-                "+165087653%02d".format(9 + index),
-                "Kevin Mester$index",
-                "KM$index",
-                MeshProtos.HardwareModel.ANDROID_SIM,
-                false
-            ),
-            it
-        )
-    }).associateBy { it.user?.id!! }
-
-    private val seedWithTestNodes = false
+    // our node info
+    private val _ourNodeInfo = MutableStateFlow<NodeInfo?>(null)
+    val ourNodeInfo: StateFlow<NodeInfo?> get() = _ourNodeInfo

    // The unique userId of our node
-    private val _myId = MutableStateFlow(if (seedWithTestNodes) "+16508765309" else null)
+    private val _myId = MutableStateFlow<String?>(null)
    val myId: StateFlow<String?> get() = _myId

-    fun setMyId(myId: String?) {
-        _myId.value = myId
-    }
-
    // A map from nodeNum to NodeInfo
    private val _nodeDBbyNum = MutableStateFlow<Map<Int, NodeInfo>>(mapOf())
    val nodeDBbyNum: StateFlow<Map<Int, NodeInfo>> get() = _nodeDBbyNum
    val nodesByNum get() = nodeDBbyNum.value

    // A map from userId to NodeInfo
-    private val _nodes = MutableStateFlow(if (seedWithTestNodes) testNodes else mapOf())
-    val nodes: StateFlow<Map<String, NodeInfo>> get() = _nodes
+    private val _nodeDBbyID = MutableStateFlow<Map<String, NodeInfo>>(mapOf())
+    val nodeDBbyID: StateFlow<Map<String, NodeInfo>> get() = _nodeDBbyID
+    val nodes get() = nodeDBbyID

-    fun setNodes(nodes: Map<String, NodeInfo>) {
-        _nodes.value = nodes
+    init {
+        nodeInfoDao.getMyNodeInfo().onEach { _myNodeInfo.value = it }
+            .launchIn(processLifecycle.coroutineScope)
+
+        nodeInfoDao.nodeDBbyNum().onEach { _nodeDBbyNum.value = it }
+            .launchIn(processLifecycle.coroutineScope)
+
+        nodeInfoDao.nodeDBbyID().onEach { _nodeDBbyID.value = it }
+            .launchIn(processLifecycle.coroutineScope)
    }

-    fun setNodes(list: List<NodeInfo>) {
-        setNodes(list.associateBy { it.user?.id!! })
-        _nodeDBbyNum.value = list.associateBy { it.num }
+    fun myNodeInfoFlow(): Flow<MyNodeInfo?> = nodeInfoDao.getMyNodeInfo()
+    fun nodeInfoFlow(): Flow<List<NodeInfo>> = nodeInfoDao.getNodes()
+    suspend fun upsert(node: NodeInfo) = withContext(Dispatchers.IO) {
+        nodeInfoDao.upsert(node)
+    }
+
+    suspend fun installNodeDB(mi: MyNodeInfo, nodes: List<NodeInfo>) = withContext(Dispatchers.IO) {
+        nodeInfoDao.apply {
+            clearNodeInfo()
+            clearMyNodeInfo()
+            putAll(nodes)
+            setMyNodeInfo(mi) // set MyNodeInfo last
+        }
+        val ourNodeInfo = nodes.find { it.num == mi.myNodeNum }
+        _ourNodeInfo.value = ourNodeInfo
+        _myId.value = ourNodeInfo?.user?.id
    }
 }
--- a/app/src/main/java/com/geeksville/mesh/model/UIState.kt
+++ b/app/src/main/java/com/geeksville/mesh/model/UIState.kt
@ -102,6 +102,7 @@ internal fun getChannelList(
@HiltViewModel
 class UIViewModel @Inject constructor(
    private val app: Application,
+    val nodeDB: NodeDB,
    private val radioConfigRepository: RadioConfigRepository,
    private val radioInterfaceService: RadioInterfaceService,
    private val meshLogRepository: MeshLogRepository,
@ -112,7 +113,6 @@ class UIViewModel @Inject constructor(

    var actionBarMenu: Menu? = null
    val meshService: IMeshService? get() = radioConfigRepository.meshService
-    val nodeDB = radioConfigRepository.nodeDB

    val bondedAddress get() = radioInterfaceService.getBondedDeviceAddress()
    val selectedBluetooth get() = radioInterfaceService.getDeviceAddress()?.getOrNull(0) == 'x'
@ -139,11 +139,8 @@ class UIViewModel @Inject constructor(
    val quickChatActions: StateFlow<List<QuickChatAction>> = _quickChatActions

    // hardware info about our local device (can be null)
-    private val _myNodeInfo = MutableStateFlow<MyNodeInfo?>(null)
-    val myNodeInfo: StateFlow<MyNodeInfo?> get() = _myNodeInfo
-
-    private val _ourNodeInfo = MutableStateFlow<NodeInfo?>(null)
-    val ourNodeInfo: StateFlow<NodeInfo?> = _ourNodeInfo
+    val myNodeInfo: StateFlow<MyNodeInfo?> get() = nodeDB.myNodeInfo
+    val ourNodeInfo: StateFlow<NodeInfo?> get() = nodeDB.ourNodeInfo

    private val requestIds = MutableStateFlow<HashMap<Int, Boolean>>(hashMapOf())

@ -156,13 +153,6 @@ class UIViewModel @Inject constructor(
            radioInterfaceService.clearErrorMessage()
        }.launchIn(viewModelScope)

-        radioConfigRepository.myNodeInfoFlow().onEach {
-            _myNodeInfo.value = it
-        }.launchIn(viewModelScope)
-
-        radioConfigRepository.nodeInfoFlow().onEach(nodeDB::setNodes)
-            .launchIn(viewModelScope)
-
        viewModelScope.launch {
            meshLogRepository.getAllLogs().collect { logs ->
                _meshLog.value = logs
@ -221,7 +211,7 @@ class UIViewModel @Inject constructor(
    }.asLiveData()

    private val _destNode = MutableStateFlow<NodeInfo?>(null)
-    val destNode: StateFlow<NodeInfo?> get() = if (_destNode.value != null) _destNode else _ourNodeInfo
+    val destNode: StateFlow<NodeInfo?> get() = if (_destNode.value != null) _destNode else ourNodeInfo

    /**
     * Sets the destination [NodeInfo] used in Radio Configuration.
@ -366,24 +356,6 @@ class UIViewModel @Inject constructor(
        debug("ViewModel cleared")
    }

-    /// Pull our latest node db from the device
-    fun updateNodesFromDevice() {
-        meshService?.let { service ->
-            // Update our nodeinfos based on data from the device
-            val nodes = service.nodes.associateBy { it.user?.id!! }
-            nodeDB.setNodes(nodes)
-
-            try {
-                // Pull down our real node ID - This must be done AFTER reading the nodedb because we need the DB to find our nodeinof object
-                val myId = service.myId
-                nodeDB.setMyId(myId)
-                _ourNodeInfo.value = nodes[myId]
-            } catch (ex: Exception) {
-                warn("Ignoring failure to get myId, service is probably just uninited... ${ex.message}")
-            }
-        }
-    }
-
    private inline fun updateLoraConfig(crossinline body: (Config.LoRaConfig) -> Config.LoRaConfig) {
        val data = body(config.lora)
        setConfig(config { lora = data })
--- a/app/src/main/java/com/geeksville/mesh/repository/datastore/RadioConfigRepository.kt
+++ b/app/src/main/java/com/geeksville/mesh/repository/datastore/RadioConfigRepository.kt
@ -44,15 +44,15 @@ class RadioConfigRepository @Inject constructor(
     */
    fun myNodeInfoFlow(): Flow<MyNodeInfo?> = nodeDB.myNodeInfoFlow()
    suspend fun getMyNodeInfo(): MyNodeInfo? = myNodeInfoFlow().firstOrNull()
+    val myNodeInfo: StateFlow<MyNodeInfo?> get() = nodeDB.myNodeInfo

    val nodeDBbyNum: StateFlow<Map<Int, NodeInfo>> get() = nodeDB.nodeDBbyNum
-    val nodeDBbyID: StateFlow<Map<String, NodeInfo>> get() = nodeDB.nodes
+    val nodeDBbyID: StateFlow<Map<String, NodeInfo>> get() = nodeDB.nodeDBbyID

    /**
     * Flow representing the [NodeInfo] database.
     */
-    fun nodeInfoFlow(): Flow<List<NodeInfo>> = nodeDB.nodeInfoFlow()
-    suspend fun getNodes(): List<NodeInfo>? = nodeInfoFlow().firstOrNull()
+    suspend fun getNodes(): List<NodeInfo>? = nodeDB.nodeInfoFlow().firstOrNull()

    suspend fun upsert(node: NodeInfo) = nodeDB.upsert(node)
    suspend fun installNodeDB(mi: MyNodeInfo, nodes: List<NodeInfo>) {
--- a/app/src/main/java/com/geeksville/mesh/ui/UsersFragment.kt
+++ b/app/src/main/java/com/geeksville/mesh/ui/UsersFragment.kt
@ -320,16 +320,8 @@ class UsersFragment : ScreenFragment("Users"), Logging {
        binding.nodeListView.adapter = nodesAdapter
        binding.nodeListView.layoutManager = LinearLayoutManager(requireContext())

-        // ensure our local node is first (index 0)
-        fun Map<String, NodeInfo>.perhapsReindexBy(nodeNum: Int?): Array<NodeInfo> =
-            if (size > 1 && nodeNum != null && values.firstOrNull()?.num != nodeNum) {
-                values.partition { node -> node.num == nodeNum }.let { it.first + it.second }
-            } else {
-                values
-            }.toTypedArray()
-
-        model.nodeDB.nodes.asLiveData().observe(viewLifecycleOwner) {
-            nodesAdapter.onNodesChanged(it.perhapsReindexBy(model.myNodeNum))
+        model.nodeDB.nodeDBbyNum.asLiveData().observe(viewLifecycleOwner) {
+            nodesAdapter.onNodesChanged(it.values.toTypedArray())
        }

        model.localConfig.asLiveData().observe(viewLifecycleOwner) { config ->