Performance improvements for CollectionsScreen and CollectionPickerSc…

…reen by refactoring key composables on those screens to be “restartable”, “skippable” and their arguments “stable” from the Compose’s perspective. Upgrading gradle to 8.7 Upping versionCode to 67
zotero · May 28, 2024 · b3fb421 · b3fb421
1 parent 965ca48
commit b3fb421
Show file tree

Hide file tree

Showing 12 changed files with 191 additions and 146 deletions.
diff --git a/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerScreen.kt b/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerScreen.kt
@@ -46,10 +46,10 @@ internal fun CollectionPickerScreen(
             backgroundColor = backgroundColor,
             topBar = {
                 CollectionPickerTopBar(
+                    title = viewState.title,
+                    multipleSelectionAllowed = viewModel.multipleSelectionAllowed,
                     onCancelClicked = onBack,
                     onAdd = viewModel::confirmSelection,
-                    viewState = viewState,
-                    viewModel = viewModel,
                 )
             },
         ) {

diff --git a/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerTopBar.kt b/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerTopBar.kt
@@ -8,21 +8,22 @@ import org.zotero.android.uicomponents.topbar.NewHeadingTextButton
 
 @Composable
 internal fun CollectionPickerTopBar(
+    title: String,
+    multipleSelectionAllowed: Boolean,
     onCancelClicked: () -> Unit,
     onAdd: () -> Unit,
-    viewState: CollectionPickerViewState,
-    viewModel: CollectionPickerViewModel
+
 ) {
     NewCustomTopBar(
-        title = viewState.title,
+        title = title,
         leftContainerContent = listOf {
             NewHeadingTextButton(
                 text = stringResource(id = Strings.cancel),
                 onClick = onCancelClicked
             )
         },
         rightContainerContent = listOf {
-            if (viewModel.multipleSelectionAllowed) {
+            if (multipleSelectionAllowed) {
                 NewHeadingTextButton(
                     text = stringResource(id = Strings.add),
                     onClick = onAdd

diff --git a/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerViewModel.kt b/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionPickerViewModel.kt
@@ -5,11 +5,17 @@ import dagger.hilt.android.lifecycle.HiltViewModel
 import io.realm.OrderedCollectionChangeSet
 import io.realm.OrderedRealmCollectionChangeListener
 import io.realm.RealmResults
+import kotlinx.collections.immutable.ImmutableList
+import kotlinx.collections.immutable.ImmutableSet
+import kotlinx.collections.immutable.persistentListOf
+import kotlinx.collections.immutable.toImmutableList
+import kotlinx.collections.immutable.toImmutableSet
 import org.greenrobot.eventbus.EventBus
 import org.zotero.android.architecture.BaseViewModel2
 import org.zotero.android.architecture.ScreenArguments
 import org.zotero.android.architecture.ViewEffect
 import org.zotero.android.architecture.ViewState
+import org.zotero.android.architecture.emptyImmutableSet
 import org.zotero.android.database.DbWrapper
 import org.zotero.android.database.objects.RCollection
 import org.zotero.android.database.requests.ReadCollectionsDbRequest
@@ -25,7 +31,6 @@ import org.zotero.android.sync.LibraryIdentifier
 import org.zotero.android.uicomponents.Plurals
 import org.zotero.android.uicomponents.Strings
 import timber.log.Timber
-import java.util.concurrent.ConcurrentHashMap
 import javax.inject.Inject
 
 @HiltViewModel
@@ -38,13 +43,16 @@ internal class CollectionPickerViewModel @Inject constructor(
     var multipleSelectionAllowed: Boolean = false
     private lateinit var results: RealmResults<RCollection>
 
+    private var excludedKeys: Set<String> = emptySet()
+    var libraryId: LibraryIdentifier = LibraryIdentifier.group(0)
+
     fun init() = initOnce {
         val args = ScreenArguments.collectionPickerArgs
+        this.excludedKeys = args.excludedKeys
+        this.libraryId = args.libraryId
         updateState {
             copy(
-                libraryId = args.libraryId,
-                excludedKeys = args.excludedKeys,
-                selected = args.selected
+                selected = args.selected.toImmutableSet()
             )
         }
 
@@ -77,10 +85,10 @@ internal class CollectionPickerViewModel @Inject constructor(
 
     private fun loadData() {
         try {
-            val libraryId = viewState.libraryId
+            val libraryId = this.libraryId
             val collectionsRequest = ReadCollectionsDbRequest(
                 libraryId = libraryId,
-                excludedKeys = viewState.excludedKeys
+                excludedKeys = this.excludedKeys
             )
             this.results = dbWrapper.realmDbStorage.perform(request = collectionsRequest)
             val collectionTree = CollectionTreeBuilder.collections(
@@ -118,7 +126,7 @@ internal class CollectionPickerViewModel @Inject constructor(
     private fun update(results: RealmResults<RCollection>) {
         val tree = CollectionTreeBuilder.collections(
             results,
-            libraryId = viewState.libraryId,
+            libraryId = this.libraryId,
             includeItemCounts = false
         )
         tree.sortNodes()
@@ -131,9 +139,9 @@ internal class CollectionPickerViewModel @Inject constructor(
             removed.add(key)
         }
 
-        if (!removed.isEmpty()) {
+        if (removed.isNotEmpty()) {
             updateState {
-                copy(selected = viewState.selected.subtract(removed))
+                copy(selected = viewState.selected.subtract(removed).toImmutableSet())
             }
             updateTitle(viewState.selected.size)
         }
@@ -173,23 +181,21 @@ internal class CollectionPickerViewModel @Inject constructor(
 
     private fun select(key: String) {
         updateState {
-            copy(selected = selected + key)
+            copy(selected = (selected + key).toImmutableSet())
         }
     }
 
     private fun deselect(key: String) {
         updateState {
-            copy(selected = selected - key)
+            copy(selected = (selected - key).toImmutableSet())
         }
     }
 
     private fun updateCollectionTree(collectionTree: CollectionTree) {
         collectionTree.expandAllCollections()
-
         updateState {
             copy(
-                collectionTree = collectionTree,
-                collectionItemsToDisplay = collectionTree.createSnapshot()
+                collectionItemsToDisplay = collectionTree.createSnapshot().toImmutableList()
             )
         }
         triggerEffect(CollectionPickerViewEffect.ScreenRefresh)
@@ -208,15 +214,8 @@ internal class CollectionPickerViewModel @Inject constructor(
 }
 
 internal data class CollectionPickerViewState(
-    val libraryId: LibraryIdentifier = LibraryIdentifier.group(0),
-    val excludedKeys: Set<String> = emptySet(),
-    val collectionTree: CollectionTree = CollectionTree(
-        nodes = mutableListOf(),
-        collections = ConcurrentHashMap(),
-        collapsed = ConcurrentHashMap()
-    ),
-    val collectionItemsToDisplay: List<CollectionItemWithChildren> = emptyList(),
-    val selected: Set<String> = emptySet(),
+    val collectionItemsToDisplay: ImmutableList<CollectionItemWithChildren> = persistentListOf(),
+    val selected: ImmutableSet<String> = emptyImmutableSet(),
     val title: String = ""
 ) : ViewState
 

diff --git a/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionsPickerTable.kt b/app/src/main/java/org/zotero/android/screens/collectionpicker/CollectionsPickerTable.kt
@@ -1,5 +1,6 @@
 package org.zotero.android.screens.collectionpicker
 
+import androidx.annotation.DrawableRes
 import androidx.compose.foundation.background
 import androidx.compose.foundation.interaction.MutableInteractionSource
 import androidx.compose.foundation.layout.Column
@@ -23,6 +24,7 @@ import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.unit.Dp
 import androidx.compose.ui.unit.dp
 import androidx.compose.ui.unit.sp
+import kotlinx.collections.immutable.ImmutableList
 import org.zotero.android.architecture.ui.CustomLayoutSize
 import org.zotero.android.screens.collections.data.CollectionItemWithChildren
 import org.zotero.android.uicomponents.checkbox.CircleCheckBox
@@ -42,51 +44,52 @@ internal fun CollectionsPickerTable(
         state = rememberLazyListState(),
     ) {
         recursiveCollectionItem(
-            viewState = viewState,
-            viewModel = viewModel,
             layoutType = layoutType,
-            collectionItems = viewState.collectionItemsToDisplay
+            collectionItems = viewState.collectionItemsToDisplay,
+            isChecked = { viewState.selected.contains(it.collection.identifier.keyGet) },
+            onClick = { viewModel.selectOrDeselect(it.collection) },
         )
     }
 }
 
 private fun LazyListScope.recursiveCollectionItem(
-    viewState: CollectionPickerViewState,
-    viewModel: CollectionPickerViewModel,
     layoutType: CustomLayoutSize.LayoutType,
-    collectionItems: List<CollectionItemWithChildren>,
-    levelPadding: Dp = 12.dp
+    levelPadding: Dp = 12.dp,
+    collectionItems: ImmutableList<CollectionItemWithChildren>,
+    isChecked: (item: CollectionItemWithChildren) -> Boolean,
+    onClick: (item: CollectionItemWithChildren) -> Unit
 ) {
     for (item in collectionItems) {
         item {
             CollectionItem(
-                item = item,
+                iconName = item.collection.iconName,
+                collectionName = item.collection.name,
                 layoutType = layoutType,
-                viewState = viewState,
-                viewModel = viewModel,
                 levelPadding = levelPadding,
+                isChecked = isChecked(item),
+                onClick = { onClick(item) },
             )
         }
 
         recursiveCollectionItem(
-            viewState = viewState,
-            viewModel = viewModel,
             layoutType = layoutType,
+            levelPadding = levelPadding + levelPaddingConst,
             collectionItems = item.children,
-            levelPadding = levelPadding + levelPaddingConst
+            isChecked = isChecked,
+            onClick = onClick,
         )
     }
 }
 
 @Composable
 private fun CollectionItem(
-    viewState: CollectionPickerViewState,
-    viewModel: CollectionPickerViewModel,
-    item: CollectionItemWithChildren,
+    @DrawableRes iconName: Int,
+    collectionName: String,
     layoutType: CustomLayoutSize.LayoutType,
-    levelPadding: Dp
+    levelPadding: Dp,
+    isChecked: Boolean,
+    onClick: () -> Unit,
 ) {
-    val isChecked = viewState.selected.contains(item.collection.identifier.keyGet)
     var rowModifier: Modifier = Modifier
     if (isChecked) {
         rowModifier = rowModifier.background(color = CustomTheme.colors.popupSelectedRow)
@@ -98,7 +101,7 @@ private fun CollectionItem(
             .safeClickable(
                 interactionSource = remember { MutableInteractionSource() },
                 indication = rememberRipple(),
-                onClick = { viewModel.selectOrDeselect(item.collection) },
+                onClick = onClick,
             )
     ) {
         Spacer(modifier = Modifier.width(levelPadding))
@@ -109,7 +112,7 @@ private fun CollectionItem(
         Spacer(modifier = Modifier.width(16.dp))
         Icon(
             modifier = Modifier.size(layoutType.calculateItemsRowMainIconSize()),
-            painter = painterResource(id = item.collection.iconName),
+            painter = painterResource(id = iconName),
             contentDescription = null,
             tint = CustomTheme.colors.zoteroDefaultBlue
         )
@@ -121,7 +124,7 @@ private fun CollectionItem(
                     modifier = Modifier.weight(1f)
                 ) {
                     Text(
-                        text = item.collection.name,
+                        text = collectionName,
                         fontSize = 14.sp,
                         maxLines = 1,
                         overflow = TextOverflow.Ellipsis

diff --git a/app/src/main/java/org/zotero/android/screens/collections/CollectionsScreen.kt b/app/src/main/java/org/zotero/android/screens/collections/CollectionsScreen.kt
@@ -61,8 +61,9 @@ internal fun CollectionsScreen(
         CustomScaffold(
             topBar = {
                 CollectionsTopBar(
-                    viewState = viewState,
-                    viewModel = viewModel,
+                    libraryName = viewState.libraryName,
+                    navigateToLibraries = viewModel::navigateToLibraries,
+                    onAdd = viewModel::onAdd,
                 )
             },
         ) {