I have a scaffold with a TopBar.
the topBar should display a list of items (Devices list). Therefore I created a viewmodel for it.
I also have a composable that lets you add an item to that list (adding a new Device to the list). and I want the topBar to show the updated list.
I use the same view model for getting the list and to add an item to the list.
The problem is that the addDevice method is being called, but it looks like the list is being recreated everytime, so the Device is not really being saved in the list.
This is the viewmodel:
class ConnectableTopAppBarViewModel : ViewModel() {
private val _uiState = MutableStateFlow(ConnectableTopAppBarUiState())
val uiState: StateFlow<ConnectableTopAppBarUiState> = _uiState.asStateFlow()
fun addDevice() {
_uiState.update { currentState ->
val updatedDevices = ArrayList<Device>()
for (device in currentState.devices) {
device.isSelected = false
updatedDevices.add(device)
}
updatedDevices.add(Device("Device ${updatedDevices.size}", "123.158.14.92", true))
Log.d("TAG", "total: ${updatedDevices.size}")
currentState.copy(
selectedDeviceIndex = updatedDevices.size - 1,
devices = updatedDevices
)
}
}
fun selectDevice(deviceIndex: Int) {
_uiState.update { currentState ->
var i = 0
val updatedDevices = ArrayList<Device>()
for (device in currentState.devices) {
device.isSelected = i == deviceIndex
updatedDevices.add(device)
i += 1
}
currentState.copy(
selectedDeviceIndex = deviceIndex,
devices = updatedDevices
)
}
Log.d("TAG", uiState.value.devices.joinToString(" "))
}
}
This is ConnectableTopAppBarUiState:
data class ConnectableTopAppBarUiState(
val selectedDeviceIndex: Int = 0,
val devices: List<Device> = mutableListOf()
)
This is the topBar to show devices and navigate to the screen where we can add Devices:
@Composable
fun ConnectableTopAppBar(
appState: CustomAppState,
modifier: Modifier = Modifier,
connectableTopAppBarViewModel: ConnectableTopAppBarViewModel = ConnectableTopAppBarViewModel()
) {
val deviceUiState by connectableTopAppBarViewModel.uiState.collectAsState()
Row(
modifier = modifier
.fillMaxWidth()
) {
LazyRow(
modifier = modifier.weight(1f),
contentPadding = PaddingValues(horizontal = 16.dp),
horizontalArrangement = Arrangement.spacedBy(8.dp)
) {
// Here I display the devices
itemsIndexed(deviceUiState.devices) { index, device ->
CustomButton(
onClick = { connectableTopAppBarViewModel.selectDevice(index) },
text = {
Text(
text = device.name,
style = MaterialTheme.typography.labelLarge
)
},
leadingIcon = {
Icon(
painter = painterResource(id = R.drawable.ic_wifi),
contentDescription = null
)
},
)
}
// Here I navigate to the screen where we can add more devices
item {
CustomOutlinedButton(
onClick = {
appState.navController.navigate(SubLevelDestination.CONNECT.route)
},
text = {
Text(
text = "Add Device",
style = MaterialTheme.typography.labelLarge
)
},
leadingIcon = {
Icon(imageVector = AppIcons.Add, contentDescription = null)
},
)
}
}
}
}
and this is the compose where I add a Device:
@Composable
fun ConnectScreen(
upPress: () -> Unit,
modifier: Modifier = Modifier,
connectableTopAppBarViewModel: ConnectableTopAppBarViewModel = ConnectableTopAppBarViewModel(),
) {
Column(
modifier = modifier
) {
ChooseRemoteSectionTitle("Select Device To Add")
LazyColumn(
modifier = modifier,
contentPadding = PaddingValues(horizontal = 16.dp),
verticalArrangement = Arrangement.spacedBy(8.dp)
) {
item {
ConnectableDeviceItem(
addDevice = connectableTopAppBarViewModel::addDevice,
modifier = modifier,
upPress = upPress,
name = "Device Example 1"
)
}
item {
ConnectableDeviceItem(
addDevice = connectableTopAppBarViewModel::addDevice,
modifier = modifier,
upPress = upPress,
name = "Device Example 2"
)
}
}
}
}
@Composable
@OptIn(ExperimentalMaterial3Api::class)
private fun ConnectableDeviceItem(
addDevice: () -> Unit,
modifier: Modifier,
upPress: () -> Unit,
name: String
) {
Card(
modifier = modifier.fillMaxWidth(),
onClick = {
addDevice()
upPress()
}
) {
Box(
modifier = modifier
.fillMaxWidth()
.background(MaterialTheme.colorScheme.surface)
.padding(5.dp),
contentAlignment = Alignment.Center
) {
Text(text = name)
}
}
}
EDIT:
I now created the viewmodel and passed it to the relevant composables, so they both use the same instance of the viewmodel:
@Composable
fun RemoteControlApp() {
val connectableTopAppBarViewModel = ConnectableTopAppBarViewModel()
....
ConnectableTopAppBar(
viewModel = connectableTopAppBarViewModel,
appState = appState
)
....
remoteControlNavGraph(
connectableTopAppBarViewModel = connectableTopAppBarViewModel,
upPress = appState::upPress,
paddingValues = paddingValues
)
}
is it a good approache?
I see the primary issue...you must never call a ViewModel class's constructor yourself, except in its factory if it has one. (In this case, there's no need for a factory since your ViewModel class has an empty constructor.)
Since you are directly calling the ViewModel's constructor, you have multiple instances of your ViewModel. Changes in one instance will not affect the other instance(s).
Replace your calls to ConnectableTopAppBarViewModel() with calling viewModel(). This is the proper way to get a reference to your ViewModel, and get the same instance used elsewhere in your composition if they are all using this same function.
I see another lurking problem...your Device class is mutable, but you're trying to use it as part of your State. This is extremely error-prone. I'll give an example of the problem you can create. To simplify, please imagine the selectedDeviceIndex property doesn't exist, and the only way you are tracking which item is selected is with the isSelected property of each Device in the List. Your function for setting the selected device could be reduced to:
fun selectDevice(deviceIndex: Int) {
_uiState.update { currentState ->
var i = 0
val updatedDevices = ArrayList<Device>()
for (device in currentState.devices) {
device.isSelected = i++ == deviceIndex
updatedDevices.add(device)
}
currentState.copy(
devices = updatedDevices
)
}
}
Notice that you are reusing the same instances of Device in your new list as were in your old list. You have only mutated them without copying them.
Now when the MutableState object compares the old and new Lists, it will not detect that anything has changed, because the contents of the old list are pointing to the same instances of Device, so the old list already has the new state in it. So, no recomposition will occur when you change the selected device!
But, you are also storing the selected device in a separate property selectedDeviceIndex, so this is probably avoiding the above issue to an extent. Recomposition will occur, but other possible optimizations in Compose possibly will not work correctly.
And the other problem with this is that you have two sources of truth. You can find which item is selected either by looking at selectedDeviceIndex or by checking isSelected on each Device. This results in you having to change both sources of truth every time you want to change this aspect of your state, which is very error prone. It just gives your code more opportunities for bugs, where perhaps the two sources won't agree with each other.
My recommendation is to remove the isSelected property from the Device class, and add an extension function:
fun Device.isSelected(owningState: ConnectableTopAppBarUiState) =
owningState.devices[owningState.selectedDeviceIndex] === this
Then your ViewModel functions can be simplified to:
fun addDevice() {
_uiState.update { currentState ->
val newDeviceIndex = currentState.devices.size
val newDevice = Device("Device $newDeviceIndex", "123.158.14.92", true))
Log.d("TAG", "total: ${updatedDevices.size}")
currentState.copy(
selectedDeviceIndex = newDeviceIndex,
devices = currentState.devices + newDevice
)
}
}
fun selectDevice(deviceIndex: Int) {
_uiState.update { currentState ->
currentState.copy(
selectedDeviceIndex = deviceIndex,
)
}
Log.d("TAG", uiState.value.devices.joinToString(" "))
}