From c8f93db00d321ae6b329a017ccbba0bba3ef894b Mon Sep 17 00:00:00 2001 From: Andre K Date: Tue, 6 Feb 2024 20:03:15 -0300 Subject: [PATCH] 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 --- .../com/geeksville/mesh/NodeInfoDaoTest.kt | 104 +++++++++++++++++ .../java/com/geeksville/mesh/MainActivity.kt | 7 -- .../mesh/database/DatabaseModule.kt | 6 - .../mesh/database/MeshtasticDatabase.kt | 2 - .../mesh/database/dao/MyNodeInfoDao.kt | 21 ---- .../mesh/database/dao/NodeInfoDao.kt | 17 +++ .../java/com/geeksville/mesh/model/NodeDB.kt | 106 +++++++----------- .../java/com/geeksville/mesh/model/UIState.kt | 36 +----- .../datastore/RadioConfigRepository.kt | 6 +- .../com/geeksville/mesh/ui/UsersFragment.kt | 12 +- 10 files changed, 170 insertions(+), 147 deletions(-) create mode 100644 app/src/androidTest/java/com/geeksville/mesh/NodeInfoDaoTest.kt delete mode 100644 app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt diff --git a/app/src/androidTest/java/com/geeksville/mesh/NodeInfoDaoTest.kt b/app/src/androidTest/java/com/geeksville/mesh/NodeInfoDaoTest.kt new file mode 100644 index 000000000..b4901fe07 --- /dev/null +++ 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) + } + } +} diff --git a/app/src/main/java/com/geeksville/mesh/MainActivity.kt b/app/src/main/java/com/geeksville/mesh/MainActivity.kt index e671c3d64..95cba78b0 100644 --- 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) { diff --git a/app/src/main/java/com/geeksville/mesh/database/DatabaseModule.kt b/app/src/main/java/com/geeksville/mesh/database/DatabaseModule.kt index 3552093f1..2c2909f2c 100644 --- 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() diff --git a/app/src/main/java/com/geeksville/mesh/database/MeshtasticDatabase.kt b/app/src/main/java/com/geeksville/mesh/database/MeshtasticDatabase.kt index 8b7eb289c..73384833f 100644 --- 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 diff --git a/app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt b/app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt deleted file mode 100644 index e81c2afb4..000000000 --- a/app/src/main/java/com/geeksville/mesh/database/dao/MyNodeInfoDao.kt +++ /dev/null @@ -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 - - @Insert(onConflict = OnConflictStrategy.REPLACE) - fun setMyNodeInfo(myInfo: MyNodeInfo) - - @Query("DELETE FROM MyNodeInfo") - fun clearMyNodeInfo() -} diff --git a/app/src/main/java/com/geeksville/mesh/database/dao/NodeInfoDao.kt b/app/src/main/java/com/geeksville/mesh/database/dao/NodeInfoDao.kt index 7db5c6f8d..01d1683e5 100644 --- 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 + + @Insert(onConflict = OnConflictStrategy.REPLACE) + fun setMyNodeInfo(myInfo: MyNodeInfo) + + @Query("DELETE FROM MyNodeInfo") + fun clearMyNodeInfo() + @Query("SELECT * FROM NodeInfo") fun getNodes(): Flow> + @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> + + @Query("SELECT * FROM NodeInfo") + fun nodeDBbyID(): Flow> + @Insert(onConflict = OnConflictStrategy.REPLACE) fun insert(node: NodeInfo) diff --git a/app/src/main/java/com/geeksville/mesh/model/NodeDB.kt b/app/src/main/java/com/geeksville/mesh/model/NodeDB.kt index be1ad97e3..1e8a44f00 100644 --- 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 = 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(null) + val myNodeInfo: StateFlow get() = _myNodeInfo - fun nodeInfoFlow(): Flow> = nodeInfoDao.getNodes() - suspend fun upsert(node: NodeInfo) = withContext(Dispatchers.IO) { - nodeInfoDao.upsert(node) - } - - suspend fun installNodeDB(mi: MyNodeInfo, nodes: List) { - 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(null) + val ourNodeInfo: StateFlow get() = _ourNodeInfo // The unique userId of our node - private val _myId = MutableStateFlow(if (seedWithTestNodes) "+16508765309" else null) + private val _myId = MutableStateFlow(null) val myId: StateFlow get() = _myId - fun setMyId(myId: String?) { - _myId.value = myId - } - // A map from nodeNum to NodeInfo private val _nodeDBbyNum = MutableStateFlow>(mapOf()) val nodeDBbyNum: StateFlow> 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> get() = _nodes + private val _nodeDBbyID = MutableStateFlow>(mapOf()) + val nodeDBbyID: StateFlow> get() = _nodeDBbyID + val nodes get() = nodeDBbyID - fun setNodes(nodes: Map) { - _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) { - setNodes(list.associateBy { it.user?.id!! }) - _nodeDBbyNum.value = list.associateBy { it.num } + fun myNodeInfoFlow(): Flow = nodeInfoDao.getMyNodeInfo() + fun nodeInfoFlow(): Flow> = nodeInfoDao.getNodes() + suspend fun upsert(node: NodeInfo) = withContext(Dispatchers.IO) { + nodeInfoDao.upsert(node) + } + + suspend fun installNodeDB(mi: MyNodeInfo, nodes: List) = 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 } } diff --git a/app/src/main/java/com/geeksville/mesh/model/UIState.kt b/app/src/main/java/com/geeksville/mesh/model/UIState.kt index 6d3b0ba97..8a4a9f038 100644 --- 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> = _quickChatActions // hardware info about our local device (can be null) - private val _myNodeInfo = MutableStateFlow(null) - val myNodeInfo: StateFlow get() = _myNodeInfo - - private val _ourNodeInfo = MutableStateFlow(null) - val ourNodeInfo: StateFlow = _ourNodeInfo + val myNodeInfo: StateFlow get() = nodeDB.myNodeInfo + val ourNodeInfo: StateFlow get() = nodeDB.ourNodeInfo private val requestIds = MutableStateFlow>(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(null) - val destNode: StateFlow get() = if (_destNode.value != null) _destNode else _ourNodeInfo + val destNode: StateFlow 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 }) diff --git a/app/src/main/java/com/geeksville/mesh/repository/datastore/RadioConfigRepository.kt b/app/src/main/java/com/geeksville/mesh/repository/datastore/RadioConfigRepository.kt index 0fa029442..94ed57f44 100644 --- 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 = nodeDB.myNodeInfoFlow() suspend fun getMyNodeInfo(): MyNodeInfo? = myNodeInfoFlow().firstOrNull() + val myNodeInfo: StateFlow get() = nodeDB.myNodeInfo val nodeDBbyNum: StateFlow> get() = nodeDB.nodeDBbyNum - val nodeDBbyID: StateFlow> get() = nodeDB.nodes + val nodeDBbyID: StateFlow> get() = nodeDB.nodeDBbyID /** * Flow representing the [NodeInfo] database. */ - fun nodeInfoFlow(): Flow> = nodeDB.nodeInfoFlow() - suspend fun getNodes(): List? = nodeInfoFlow().firstOrNull() + suspend fun getNodes(): List? = nodeDB.nodeInfoFlow().firstOrNull() suspend fun upsert(node: NodeInfo) = nodeDB.upsert(node) suspend fun installNodeDB(mi: MyNodeInfo, nodes: List) { diff --git a/app/src/main/java/com/geeksville/mesh/ui/UsersFragment.kt b/app/src/main/java/com/geeksville/mesh/ui/UsersFragment.kt index b11aea4dc..751aa43f5 100644 --- 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.perhapsReindexBy(nodeNum: Int?): Array = - 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 ->