Update SelectableLazyColumn for key separation and update Gradle wrapper #216
Conversation
Refactor SelectableLazyList and related classes, separating `selectionKey` and `uiKey`. `selectionKey` is used for selection and `uiKey` triggers recompositions. Also, upgraded Gradle wrapper to 8.4. This change improves re-composition handling and provides an easier way to control item selection without unnecessary recompositions.
foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyListScope.kt
Outdated
Show resolved
Hide resolved
foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyListScope.kt
Outdated
Show resolved
Hide resolved
foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/SelectableLazyListScope.kt
Show resolved
Hide resolved
foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/tree/BuildTree.kt
Show resolved
Hide resolved
| LaunchedEffect(Unit) { | ||
| while (true) { | ||
| delay(1.seconds) | ||
| listOfItems = listOfItems.mapIndexed { index, s -> | ||
| if (index != 2) s else "WHOOO ${Random.nextInt(0, 10)}" | ||
| } | ||
| } |
There was a problem hiding this comment.
What is this? If it's test code — as it looks to be — please revert. If not please explain why you need it to be in the sample.
There was a problem hiding this comment.
It is a test for the changes. The objective of this PR is to allow to recompose when an item changes without loosing the selection. In other words, the selection should be on a subset of the data available in the item.
There was a problem hiding this comment.
Yeah it's not something I would include in the sample for everyone though, especially since it's not something we want anyone to actually be doing :P
There was a problem hiding this comment.
Please only keep this change locally while you develop.
| items( | ||
| count = listOfItems.size, | ||
| selectionKey = { index -> index }, | ||
| uiKey = { index -> listOfItems[index] }, |
There was a problem hiding this comment.
If the end goal is to use the item itself as data key for the recomposition, why don't we just do that?
There was a problem hiding this comment.
The objective of this PR is to allow to recompose when an item changes without loosing the selection. In other words, the selection should be on a subset of the data available in the item.
There was a problem hiding this comment.
the selection should be on a subset of the data available in the item
And yet here the selection key is not based on the item data 🤔 but the UI key is
| .onFocusChanged { | ||
| isFocused = it.isFocused || it.hasFocus | ||
| } | ||
| .focusGroup() |
There was a problem hiding this comment.
There shouldn't be any focusable elements inside SelectableLazyColumn, otherwise tab behavior will work incorrectly, so we need to check only isFocused state and don't make it focusGroup
There was a problem hiding this comment.
Probably I'm missing something here, the only selectable element is the LazyColumn (due SEL is a wrapper).
if you are going to remove focusable from the LC, then you will have no way to send keybindings event to the component.
BTW in most use cases, I doubt that users will not put something clickable inside the SLC, and if I'm not mind wrong, we cannot prevent that
Probably we can accept that and find a way to handle TAB key-event properly by moving in some way the focus to the next element
There was a problem hiding this comment.
I think I agree with @Walingar. On the other hand we need to warn the user to not use focusable or clickable modifiers on items inside the list. A solution could be to add such modifiers with all their overloads as extension members of SelectableLazyListScope and mark such modifiers as errors using annotations.
There was a problem hiding this comment.
I don't know how that scales. I reckon a Lint check would the right way to go, but since we don't have that, a Detekt check would be good as well.
| contentType: (index: Int) -> Any? = { null }, | ||
| selectable: (index: Int) -> Boolean = { true }, | ||
| itemContent: @Composable SelectableLazyItemScope.(index: Int) -> Unit, | ||
| ) | ||
|
|
||
| /** | ||
| * A method that enables sticky header behavior in a list or grid view. | ||
| * By default `[selectionKey] = [uiKey]`, but you can assign [uiKey] if you need to |
There was a problem hiding this comment.
But if we keep only uiKey as it was before, then with unchanged uiKey element can be recomposed if State is changed.
Key is used mostly for keeping scroll in a proper position on items modification, not for skiping recomposition in this case
Can you please provide a usecase where you need different uiKey and selectionKey? Since example for Chips and Tree is wrong, since you need to provide not string but objects that hold State if you want to change it.
There was a problem hiding this comment.
Can you please provide an usecase where you need different uiKey and selectionKey
I can try to give you a reason: imagine you have a not so small item in the dataset,
- probably you will handle the item key with a minimal key (as example, you can use a UUID as key)
- now something inside this item will change (or the whole item too)
- if you have the same key for both, your item will not be shown in the list; otherwise, you lose the selection because the key is not the same
There was a problem hiding this comment.
Sorry, I still don't get it. Let's just use selectionId as itemId then? It's the same.
you lose the selection because the key is not the same
And this is expected behaviour, item is not the same, it shouldn't be selected. For sure, it depends on requirements, but this should be handled by SelectableLazyColumn itself then. I mean how to handle selection when selected items are removed
There was a problem hiding this comment.
It is needed in case you want to decouple the selection from recompositions. Indeed having a state inside the item to pass would be optimal, but in the case of PKGS it is not possible. We have a huge complex model and all the data to construct the tree is not reactive (flows/states ecc...). Everytime there is a change in a single part of the model, the entirety of the model gets re-emitted.
We could transform a portion of the model from plain properties to reactive components but that would require a huge rewrite and would also mean a lot of issues with the IJ platform (not everything is easily transformed into a reactive component).
There was a problem hiding this comment.
So then you need to introduce ids for your items and use them as keys
There was a problem hiding this comment.
Otherwise, scrolling will be broken on recomposition also.
Refactor SelectableLazyColumn and related classes, separating
selectionKeyanduiKey.selectionKeyis used for selection anduiKeytriggers recompositions. Also, upgraded Gradle wrapper to 8.4. This change improves re-composition handling and provides an easier way to control item selection without unnecessary recompositions.