-
Notifications
You must be signed in to change notification settings - Fork 5.7k
[JEWEL-849] Add support to filter items with FilterableLazyColumn #3392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
5e0f63e to
2df48f8
Compare
| if (undecorated) { | ||
| TextFieldDecorator { innerTextField -> | ||
| UndecoratedTextFieldDecorationBox( | ||
| innerTextField = innerTextField, | ||
| textStyle = textStyle, | ||
| placeholderTextColor = style.colors.placeholder, | ||
| placeholder = if (state.text.isEmpty()) placeholder else null, | ||
| ) | ||
| } | ||
| } else { | ||
| TextFieldDecorator { innerTextField -> | ||
| val minSize = style.metrics.minSize | ||
| TextFieldDecorator { innerTextField -> | ||
| val minSize = style.metrics.minSize | ||
|
|
||
| TextFieldDecorationBox( | ||
| modifier = | ||
| Modifier.defaultMinSize(minWidth = minSize.width, minHeight = minSize.height) | ||
| .padding(style.metrics.contentPadding), | ||
| innerTextField = innerTextField, | ||
| textStyle = textStyle, | ||
| placeholderTextColor = style.colors.placeholder, | ||
| placeholder = if (state.text.isEmpty()) placeholder else null, | ||
| leadingIcon = leadingIcon, | ||
| trailingIcon = trailingIcon, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, but this implementation was using two different decoration boxes, while the others used only the TextFieldDecorationBox. I've changed to always use the same to ensure we are rendering the leading/trailing icons correctly.
2df48f8 to
69b6ef0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot to unpack here, I'll wrap up my review shortly, until then, these are the questions I have so far 😄
| // If the last active index is not among the selected indices, the SpeedSearch | ||
| // has selected a different value and an update must be triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it. How come SpeedSearch can select a different value that's not in the selected indices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Returns a [MatchResult.Match] with a list of ranges where the pattern matches, or [MatchResult.NoMatch] |
| @@ -12,6 +14,12 @@ public fun interface SpeedSearchMatcher { | |||
| */ | |||
| public fun matches(text: String?): MatchResult | |||
|
|
|||
| /** | |||
| * Returns a [MatchResult.Match] with a list of ranges from the where the pattern matches, or [MatchResult.NoMatch] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Returns a [MatchResult.Match] with a list of ranges from the where the pattern matches, or [MatchResult.NoMatch] | |
| * Returns a [MatchResult.Match] with a list of ranges where the pattern matches, or [MatchResult.NoMatch] |
| val result = users.filter(matcher) { it.name } | ||
|
|
||
| assertEquals(0, result.size) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have SubstringSpeedSearchMatcherTest, what do you think of moving these tests over there?
| @@ -104,6 +104,7 @@ internal fun createBridgeComponentStyling(theme: ThemeDefinition): ComponentStyl | |||
| undecoratedDropdownStyle = readUndecoratedDropdownStyle(menuStyle), | |||
| speedSearchStyle = readSpeedSearchStyle(), | |||
| searchMatchStyle = readSearchMatchStyle(theme.isDark), | |||
| searchTextFieldStyle = readSearchTextFieldStyle(), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No filterableLazyColumn? How can users customize it? By adjusting SearchArea and SelectableLazyColumn?
| InlineWarningBanner( | ||
| text = | ||
| "One of the samples is using the SpeedSearch feature for filtering the content. " + | ||
| "Despite being possible, we strongly recommend using a different component for this behavior." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we strongly recommend using a different component for this behavior.
Why? 😯
Would be cool to let the user the reasons why and which component they should use instead. Judging solely by the name seems like Speed Search would be the best one. And maybe some reason why this one could be used
|
|
||
| internal fun TextFieldState.handleKeyEvent( | ||
| event: KeyEvent, | ||
| onTextChange: (CharSequence) -> Unit = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's move onTextChange param to the last place so we can call this extension function with a trailing lambda:
handleKeyEvent(...) { }
| event: KeyEvent, | ||
| onTextChange: (CharSequence) -> Unit = {}, | ||
| allowNavigationWithArrowKeys: Boolean = true, | ||
| allowedSymbols: String? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit strange that allowed symbols is a String. I would expect some kind of an array or collection.
| public fun SearchArea( | ||
| state: SearchAreaState, | ||
| modifier: Modifier = Modifier, | ||
| searchModifier: Modifier = Modifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I feel it's bit risky to have two separate modifiers here. I would expect that modifier dictating size of container should also influence on the size of the SearchTextField. SearchTextField should match the container width. The API is designed in such way that there is a lots of area for misuse.
Let's simply pass Modifier.fillMaxWidth() to SearchTextField.
| LaunchedEffect(itemToScrollTo) { | ||
| val index = itemToScrollTo ?: return@LaunchedEffect | ||
|
|
||
| state.scrollToItem(index + 2, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why + 2? Do we have to check for possible index out of bounds error here?
| ) : ComponentStyling { | ||
| @Deprecated("Use the variant with speedSearchStyle.", level = DeprecationLevel.HIDDEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also update other deprecated constructor so that their deprecation message points to the constructor with searchTextFieldStyle
| public fun FilterableLazyColumn( | ||
| modifier: Modifier = Modifier, | ||
| lazyColumnModifier: Modifier = Modifier, | ||
| selectionMode: SelectionMode = SelectionMode.Single, | ||
| state: SelectableLazyListState = rememberSelectableLazyListState(), | ||
| searchState: SearchAreaState = rememberSearchAreaState(), | ||
| contentPadding: PaddingValues = PaddingValues(0.dp), | ||
| reverseLayout: Boolean = false, | ||
| onSelectedIndexesChange: (List<Int>) -> Unit = {}, | ||
| verticalArrangement: Arrangement.Vertical = if (!reverseLayout) Arrangement.Top else Arrangement.Bottom, | ||
| horizontalAlignment: Alignment.Horizontal = Alignment.Start, | ||
| flingBehavior: FlingBehavior = ScrollableDefaults.flingBehavior(), | ||
| keyActions: KeyActions = DefaultSelectableLazyColumnKeyActions, | ||
| pointerEventActions: PointerEventActions = DefaultSelectableLazyColumnEventAction(), | ||
| interactionSource: MutableInteractionSource? = null, | ||
| dispatcher: CoroutineDispatcher = Dispatchers.Default, | ||
| content: FilterableLazyColumnScope.() -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should start grouping a common params into a data class? 🤔
Number of params in our surface API is getting out of hand.
| val filteredList = mutableListOf<T>() | ||
|
|
||
| for (item in items) { | ||
| if (filterText.isEmpty() || textContent(item)?.contains(filterText, ignoreCase = true) ?: false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is constant O(n) filtering of items. Maybe we should dispatching to the Default dispatcher?
| EmptySpeedSearchMatcher | ||
| } else { | ||
| buildMatcher(text).cached() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a feature envy code smell. Can we delegate initialization of EmptySpeedSearchMatcher to buildMatcher lambda implementation? I assume implementation comes for foundation module. This way we can mark EmptySpeedSearchMatcher private to foundation module.
nebojsa-vuksic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for changes
- Fixed index emission on SelectableLazyColumn when the selection changes by the speed search - Created 'EmptySpeedSearchMatcher' to easily identify when the filter text is empty - This matches is automatically returned in the SpeedSearchState.matcher when the text is empty - Added 'dismissOnLoseFocus' to SpeedSearchArea to keep the filter when the focus is left from the component - Added 'currentMatcher' to the 'SpeedSearchState', allowing the user use it for filtering purposes - Created convenience function on top of 'SpeedSearchMatcher' to check if the given text matches or not - Created convenience functions to support filtering collections based on the speed search matcher
69b6ef0 to
0b4b679
Compare
| ) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index tracking returns filtered position instead of original
High Severity
The Entry.index field incorrectly tracks the count of filtered items rather than the original position in the unfiltered list. The lastIndex variable only increments for items that pass the filter, but the documentation for onSelectedIndexesChange explicitly states "Indexes correspond to the original unfiltered list." When an item at original index 5 is selected after filtering, the callback incorrectly reports a lower index based on how many items passed the filter before it, not its true position in the source list.
Additional Locations (1)
- The IDE has one pattern for searching on lists that use a TextField on top of the content - The search text is then used to filter the content from the list bellow - To support that feature, we will introduce a few new components to Jewel - The first one is the SearchTextField - The main goal is to render the search text input, with the correct icons and behaviors - About behavior, for now, only the history popup and the clear actions are supported - The second one is the SearchArea - The main goal is to render the search input, with a divider and an additional content passed in the slot - The idea is to replicate SpeedSearchArea so users could expand the idea as needed, with their own components - The third and last is the FilterableLazyColumn - The goal is to render the selectable lazy list with additional support for filtering - It automatically handles callbacks (such as onSelectedIndexesChange) that should return the external index, instead of the filtered one - It also handles auto scroll to the selected entry on typing, and if none of the selected items is in the filtered content, select a new one to ensure the user has a valid selection
0b4b679 to
670ac31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| LaunchedEffect(itemToScrollTo) { | ||
| val index = itemToScrollTo ?: return@LaunchedEffect | ||
|
|
||
| state.scrollToItem(index + 2, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scroll offset corrupts lastActiveItemIndex with invalid value
Medium Severity
state.scrollToItem(index + 2, true) passes index + 2 as the target item index. The scrollToItem implementation sets lastActiveItemIndex = itemIndex, so lastActiveItemIndex ends up 2 positions past the actual selected item. For items near the end of a filtered list, this can point beyond valid bounds. This corrupts keyboard navigation, which uses lastActiveItemIndex as the starting point for arrow key movement.


Warning
This PR is the continuation of #3361. The only valid commit from this PR is the last one
Evidences
SearchTextField
FilterableLazyColumn
Speed Search
Release notes
New features
SearchTextFieldcomponentSearchAreacomponentSpeedSearchAreafor places where you need to filter the content as you type the textFilterableLazyColumncomponentSearchAreaand theSelectableLazyColumnNote
Medium Risk
Touches core list selection/keyboard navigation and expands public APIs around search/filtering, which can regress UX and selection/index mapping; changes are covered by new unit/UI tests but impact widely-used components.
Overview
Introduces a new filter-as-you-type list pattern via
FilterableLazyColumn/SearchArea/SearchTextField(new experimental UI API surface), plus standalone and IDE bridge styling plumbing forSearchTextFieldStyle.Extends
SpeedSearchMatcherto supportCharSequenceinputs, adds an internalEmptySpeedSearchMatcher, and provides conveniencematches/filterextensions (with fast-path for empty matcher); the matcher implementations are updated accordingly.Fixes selection/index synchronization in
SelectableLazyColumnto re-emit selection changes when SpeedSearch updateslastActiveItemIndex, and adds substantial new UI + unit tests plus a new showcase screen and icons demonstrating SpeedSearch highlighting vs filtering.Written by Cursor Bugbot for commit 670ac31. This will update automatically on new commits. Configure here.