aashishksahu / SafeSpace

A safe place for your valuable information
GNU General Public License v3.0
144 stars 9 forks source link

LazyColumn recomposes everything on long press #60

Closed aashishksahu closed 1 month ago

aashishksahu commented 1 month ago

Hi @starry-shivam Hope you are doing well. I have been working on a redesign of main activity using Jetpack Compose, so far it was going good but now I am stuck at long press to select an item in a LazyColumn. Can you please help? Build from this commit

org.privacymatters.safespace.experimental.main.ui.ItemList.kt -> the composable with LazyColumn org.privacymatters.safespace.experimental.main.MainActivityViewModel -> viewmodel for MainActivity org.privacymatters.safespace.experimental.main.DataManager -> common singleton replacement for Operations.kt

DataManager instance is called using the ops variable. In DataManager, baseItemList has all the files and folders and itemStateList is the SnapShotState List. In MainActivityViewModel, itemList refers to baseItemList.

Everytime I am long pressing on an item, it recomposes the whole list or doesn't do anything at all. If there was a way to recompose only one item, that would be great. Thanks :)

starry-shivam commented 1 month ago

Hello!

I have been working on a redesign of main activity using Jetpack Compose,

Wow, very nice!

Everytime I am long pressing on an item, it recomposes the whole list or doesn't do anything at all. If there was a way to recompose only one item, that would be great

I will have a look in sometime.

starry-shivam commented 1 month ago

@aashishksahu Why do you have two different lists, ItemStateList and BaseItemList, where BaseItemList references ItemStateList but is also a mutable property? This means it can be changed to a completely different list, which doesn't reference ItemStateList anymore. Additionally, since BaseItemList is a static list, you can't observe it. So now you have to use derived state (which you're already doing, btw). This whole system has too many points of failure and creates state management issues, which I think is what is happening here. Both ItemStateList and BaseItemList are updated separately when the latter was supposed to reference ItemStateList. In general, you should not have this kind of design to start with. Always try to have a single source of truth that can be observed by any component (i.e., anything from the UI layer) and can be updated appropriately.

I’d suggest having one single list inside a DataManager singleton, which contains all of the file/folder items and is either a Kotlin Flow or LiveData, so it can be easily observed by the UI layer. DataManager would provide methods or update the list internally based on requirements. This way, you won't have to deal with state management issues like the one happening here due to having multiple sources of data with different/conflicting information. Some parts of the UI are observing one data source, while others are observing another, and you're trying to keep them in sync manually on the backend.

While we can probably fix this particular problem with more manual synchronization and checks, this is only delaying even worse problems that will occur as the project progresses further and It will cost a lot more time and energy to refactor it then.

aashishksahu commented 1 month ago

Thanks for this, so I should keep just the ItemStateList right? I actually tried live data but it wasn't working correctly, I tried to observe it but it wasn't updating, maybe I was doing something wrong, will have to try again. Will come back in a few days after refactoring :)

also, if I use remeber and snapshotstatelist, will it work?

starry-shivam commented 1 month ago

Thanks for this, so I should keep just the ItemStateList right? I actually tried live data but it wasn't working correctly, I tried to observe it but it wasn't updating, maybe I was doing something wrong, will have to try again. Will come back in a few days after refactoring :)

also, if I use remeber and snapshotstatelist, will it work?

Personally, I'd recommend using Flows instead of LiveData because, first, they're actually very easy to work with, especially in a Jetpack Compose app. You can observe them both normally and in a lifecycle-aware manner with .collectAsState() and .collectAsStateWithLifecycle() (though the latter is currently experimental, afaik). Second, they provide many helpful operators like filter, map, transform, flatMapLatest, flatMapConcat, and flatMapMerge, to name a few, which you'll find very helpful as you learn more about them. And third, flows are native to Kotlin itself while LiveData is a part of Android Jetpack libraries, so you'll have many language specific advantages when using flows compared to LiveData.

A minimal example of the implementation:

class DataManager<T> {
    // Internal mutable list
    private val _items = mutableListOf<T>()

    // Internal mutable flow list
    private val _itemsFlow = MutableStateFlow<List<T>>(emptyList())

    // Immutable flow list for UI layer and other components to observe (SSOT)
    val itemsFlow: Flow<List<T>> get() = _itemsFlow.asStateFlow()

    fun addItem(item: T) {
        _items.add(item)
        _itemsFlow.value = _items.toList()
    }

    fun removeItem(item: T) {
        _items.remove(item)
        _itemsFlow.value = _items.toList()
    }
}
aashishksahu commented 1 month ago

Alright thanks for this, will do the refactoring :)

aashishksahu commented 1 month ago

@starry-shivam Flow worked! commit 7d704f6

starry-shivam commented 1 month ago

@starry-shivam Flow worked! commit 7d704f6

Awesome!