From 8d0e1bca58753649acfa4e099f3ec42e20d6deda Mon Sep 17 00:00:00 2001 From: Nikolay Rykunov Date: Thu, 2 Nov 2023 17:02:49 +0100 Subject: [PATCH] Rewrite multiple selection, speeding it up Also, fix DefaultSelectableLazyColumnKeyActions check for multiple selection --- .../lazy/SelectableColumnOnKeyEvent.kt | 85 +++++------ .../jewel/foundation/lazy/tree/KeyActions.kt | 2 +- .../lazy/SelectableLazyColumnTest.kt | 140 +++++++++++++++++- 3 files changed, 175 insertions(+), 52 deletions(-) diff --git a/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableColumnOnKeyEvent.kt b/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableColumnOnKeyEvent.kt index 857b5d4f6..52d269b5c 100644 --- a/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableColumnOnKeyEvent.kt +++ b/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableColumnOnKeyEvent.kt @@ -1,6 +1,7 @@ package org.jetbrains.jewel.foundation.lazy import org.jetbrains.jewel.foundation.lazy.SelectableLazyListKey.Selectable +import java.util.Collections.addAll import kotlin.math.max import kotlin.math.min @@ -26,25 +27,18 @@ interface SelectableColumnOnKeyEvent { * Extend Selection to First Node inherited from Move Caret to Text Start with Selection */ fun onExtendSelectionToFirst(keys: List, state: SelectableLazyListState) { - state.lastActiveItemIndex - ?.let { - val iterator = keys.listIterator(it) - val list = buildList { - while (iterator.hasPrevious()) { - val previous = iterator.previous() - if (previous is Selectable) { - add(previous.key) - state.lastActiveItemIndex = (iterator.previousIndex() + 1).coerceAtMost(keys.size) - } - } - } - if (list.isNotEmpty()) { - state.selectedKeys = - state.selectedKeys - .toMutableList() - .also { selectionList -> selectionList.addAll(list) } - } + val initialIndex = state.lastActiveItemIndex ?: return + val newSelection = ArrayList(max(initialIndex, state.selectedKeys.size)).apply { + addAll(state.selectedKeys) + } + for (index in initialIndex - 1 downTo 0) { + val key = keys[index] + if (key is Selectable) { + newSelection.add(key.key) + state.lastActiveItemIndex = index } + } + state.selectedKeys = newSelection } /** @@ -65,16 +59,18 @@ interface SelectableColumnOnKeyEvent { * Extend Selection to Last Node inherited from Move Caret to Text End with Selection */ fun onExtendSelectionToLastItem(keys: List, state: SelectableLazyListState) { - state.lastActiveItemIndex?.let { - val list = mutableListOf(state.selectedKeys) - keys.subList(it, keys.lastIndex).forEachIndexed { index, selectableLazyListKey -> - if (selectableLazyListKey is Selectable) { - list.add(selectableLazyListKey.key) - } + val initialIndex = state.lastActiveItemIndex ?: return + val newSelection = ArrayList(max(keys.size - initialIndex, state.selectedKeys.size)).apply { + addAll(state.selectedKeys) + } + for (index in initialIndex + 1..keys.lastIndex) { + val key = keys[index] + if (key is Selectable) { + newSelection.add(key.key) state.lastActiveItemIndex = index } - state.selectedKeys = list } + state.selectedKeys = newSelection } /** @@ -96,18 +92,15 @@ interface SelectableColumnOnKeyEvent { * Extend Selection with Previous Node inherited from Up with Selection */ fun onExtendSelectionWithPreviousItem(keys: List, state: SelectableLazyListState) { - state.lastActiveItemIndex?.let { lastActiveIndex -> - if (lastActiveIndex == 0) return@let - keys - .withIndex() - .toList() - .dropLastWhile { it.index >= lastActiveIndex } - .reversed() - .firstOrNull { it.value is Selectable } - ?.let { (index, selectableKey) -> - state.selectedKeys = state.selectedKeys + selectableKey.key - state.lastActiveItemIndex = index - } + // todo we need deselect if we are changing direction + val initialIndex = state.lastActiveItemIndex ?: return + for (index in initialIndex - 1 downTo 0) { + val key = keys[index] + if (key is Selectable) { + state.selectedKeys += key.key + state.lastActiveItemIndex = index + return + } } } @@ -131,16 +124,14 @@ interface SelectableColumnOnKeyEvent { */ fun onExtendSelectionWithNextItem(keys: List, state: SelectableLazyListState) { // todo we need deselect if we are changing direction - state.lastActiveItemIndex?.let { lastActiveIndex -> - if (lastActiveIndex == keys.lastIndex) return@let - keys - .withIndex() - .dropWhile { it.index <= lastActiveIndex } - .firstOrNull { it.value is Selectable } - ?.let { (index, selectableKey) -> - state.selectedKeys = state.selectedKeys + selectableKey.key - state.lastActiveItemIndex = index - } + val initialIndex = state.lastActiveItemIndex ?: return + for (index in initialIndex + 1..keys.lastIndex) { + val key = keys[index] + if (key is Selectable) { + state.selectedKeys += key.key + state.lastActiveItemIndex = index + return + } } } diff --git a/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt b/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt index f252120a5..5053fbbf6 100644 --- a/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt +++ b/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/KeyActions.kt @@ -286,7 +286,7 @@ open class DefaultSelectableLazyColumnKeyActions( isSelectLastItem -> onSelectLastItem(keys, state) isEdit -> onEdit() } - if (selectionMode == SelectionMode.Single) { + if (selectionMode == SelectionMode.Multiple) { when { isExtendSelectionToFirstItem -> onExtendSelectionToFirst(keys, state) isExtendSelectionToLastItem -> onExtendSelectionToLastItem(keys, state) diff --git a/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnTest.kt b/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnTest.kt index 77ae43254..28cce64b9 100644 --- a/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnTest.kt +++ b/foundation/src/test/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyColumnTest.kt @@ -12,6 +12,8 @@ import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performKeyInput +import androidx.compose.ui.test.pressKey +import androidx.compose.ui.test.withKeyDown import androidx.compose.ui.unit.dp import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals @@ -92,8 +94,7 @@ internal class SelectableLazyColumnTest { // press arrow up and check that selected key is changed repeat(20) { step -> composeRule.onNodeWithTag("list").performKeyInput { - keyDown(Key.DirectionUp) - keyUp(Key.DirectionUp) + pressKey(Key.DirectionUp) } // check that previous element is selected @@ -110,8 +111,7 @@ internal class SelectableLazyColumnTest { // press arrow down and check that selected key is changed repeat(40) { step -> composeRule.onNodeWithTag("list").performKeyInput { - keyDown(Key.DirectionDown) - keyUp(Key.DirectionDown) + pressKey(Key.DirectionDown) } // check that next element is selected @@ -124,4 +124,136 @@ internal class SelectableLazyColumnTest { assertTrue(state.selectedKeys.size == 1) assertEquals(items.last(), state.selectedKeys.single()) } + + @OptIn(ExperimentalTestApi::class) + @Test + fun `multiple items selection`() = runBlocking { + val items = (0..10).toList() + val state = SelectableLazyListState(LazyListState()) + composeRule.setContent { + Box(modifier = Modifier.requiredHeight(300.dp)) { + SelectableLazyColumn(state = state, modifier = Modifier.testTag("list")) { + items( + items.size, + key = { + items[it] + }, + ) { + val itemText = "Item ${items[it]}" + BasicText(itemText, modifier = Modifier.testTag(itemText)) + } + } + } + } + composeRule.awaitIdle() + // select item 5 by click + composeRule.onNodeWithTag("Item 5").assertExists() + composeRule.onNodeWithTag("Item 5").performClick() + + // check that 5th element is selected + assertEquals(1, state.selectedKeys.size) + assertEquals(items[5], state.selectedKeys.single()) + + // press arrow up with pressed Shift and check that selected keys are changed + repeat(20) { step -> + composeRule.onNodeWithTag("list").performKeyInput { + withKeyDown(Key.ShiftLeft) { + pressKey(Key.DirectionUp) + } + } + + // check that previous element is added to selection + // when started from 5th element + val expectedFirstSelectedIndex = (5 - step - 1).takeIf { it >= 0 } ?: 0 + val elements = items.subList(expectedFirstSelectedIndex, 6) + assertEquals(elements.size, state.selectedKeys.size) + assertEquals(elements.toSet(), state.selectedKeys.toSet()) + } + + // select first item by click + composeRule.onNodeWithTag("Item 0").assertExists() + composeRule.onNodeWithTag("Item 0").performClick() + + // check that first element is selected + assertEquals(1, state.selectedKeys.size) + assertEquals(items[0], state.selectedKeys.single()) + + // press arrow down with pressed Shift and check that selected keys are changed + repeat(20) { step -> + composeRule.onNodeWithTag("list").performKeyInput { + withKeyDown(Key.ShiftLeft) { + pressKey(Key.DirectionDown) + } + } + + // check that next element is added to selection + val expectedFirstSelectedIndex = (step + 1).takeIf { it in items.indices } ?: items.lastIndex + val elements = items.subList(0, expectedFirstSelectedIndex + 1) + assertEquals(elements.size, state.selectedKeys.size) + assertEquals(elements.toSet(), state.selectedKeys.toSet()) + } + + // all elements should be selected in the end + assertEquals(items.size, state.selectedKeys.size) + assertEquals(items.toSet(), state.selectedKeys.toSet()) + } + + @OptIn(ExperimentalTestApi::class) + @Test + fun `select to first and last`() = runBlocking { + val items = (0..50).toList() + val state = SelectableLazyListState(LazyListState()) + composeRule.setContent { + Box(modifier = Modifier.requiredHeight(300.dp)) { + SelectableLazyColumn(state = state, modifier = Modifier.testTag("list")) { + items( + items.size, + key = { + items[it] + }, + ) { + val itemText = "Item ${items[it]}" + BasicText(itemText, modifier = Modifier.testTag(itemText)) + } + } + } + } + composeRule.awaitIdle() + // select item 5 by click + composeRule.onNodeWithTag("Item 5").assertExists() + composeRule.onNodeWithTag("Item 5").performClick() + + // check that 5th element is selected + assertEquals(1, state.selectedKeys.size) + assertEquals(items[5], state.selectedKeys.single()) + + // perform home with shift, so all items until 5th should be selected + composeRule.onNodeWithTag("list").performKeyInput { + withKeyDown(Key.ShiftLeft) { + pressKey(Key.MoveHome) + } + } + val expectedElementsAfterPageUp = items.subList(0, 6) + assertEquals(expectedElementsAfterPageUp.size, state.selectedKeys.size) + assertEquals(expectedElementsAfterPageUp.toSet(), state.selectedKeys.toSet()) + + + // select item 5 by click + composeRule.onNodeWithTag("Item 5").assertExists() + composeRule.onNodeWithTag("Item 5").performClick() + + // check that 5th element is selected + assertEquals(1, state.selectedKeys.size) + assertEquals(items[5], state.selectedKeys.single()) + + // perform end with shift, so all items after 5th should be selected + composeRule.onNodeWithTag("list").performKeyInput { + withKeyDown(Key.ShiftLeft) { + pressKey(Key.MoveEnd) + } + } + val expectedElementsAfterPageDown = items.subList(5, items.lastIndex + 1) + assertEquals(expectedElementsAfterPageDown.size, state.selectedKeys.size) + assertEquals(expectedElementsAfterPageDown.toSet(), state.selectedKeys.toSet()) + } }