-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[JEWEL-61] LazyTable Component #3312
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
...n/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasuredItemProvider.kt
Outdated
Show resolved
Hide resolved
...dation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableIntervalContent.kt
Outdated
Show resolved
Hide resolved
|
That's fine, this PR isn't going to be merged soon. Gustavo will rebase. |
76a99e3 to
71a1a1a
Compare
...undation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableAnimateScroll.kt
Show resolved
Hide resolved
|
I tried it and it looks very impressive, but one thing is very bad and it is performance. On pretty medium harware on linux it feels very very laggy. 🙁 Anyway thank you for this awesome work 🙂 |
|
@faogustavo any hope you can do a quick pass with a profiler to see what's going on? |
71a1a1a to
092f094
Compare
platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/OverflowBox.kt
Outdated
Show resolved
Hide resolved
...c/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/draggable/LazyTableDraggableState.kt
Outdated
Show resolved
Hide resolved
092f094 to
7898241
Compare
So, I've run it and found a few issues that could be improved. The biggest problem was happening in the measurement/placement phase. These are the fixes I've applied (which had significant performance improvement):
Please let me know if you still feel the component is laggy, and I can perform a more thorough review. |
...undation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableAnimateScroll.kt
Show resolved
Hide resolved
7898241 to
d9af3c2
Compare
|
|
||
| override suspend fun snapToLine(lineIndex: Int, scrollOffset: Int) { | ||
| scrollState.scrollToColumn(lineIndex, scrollOffset) | ||
| } |
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.
Bug: Scrollbar snap uses relative index without adding pinned offset
The snapToLine methods in both LazyTableHorizontalScrollbarAdapter and LazyTableVerticalScrollbarAdapter pass a relative index (0-based for floating items) directly to scrollToColumn/scrollToRow, which expect absolute indices. The scrollbar adapter's totalLineCount() returns columns - pinnedColumns (or rows - pinnedRows), so its calculated indices are relative to floating items. When the user drags the scrollbar far enough to trigger snapTo, passing index 0 should scroll to column pinnedColumns, not column 0. The measurement code clamps indices below pinnedColumns to pinnedColumns and discards the scroll offset, resulting in incorrect scroll positions. The fix requires adding pinnedColumns/pinnedRows to the lineIndex before calling the scroll methods.
Additional Locations (1)
|
@faogustavo I tried it and congratulations to the speedup and interactive components, very nice :) But it is still very "laggy" to use, maybe it is just linux and something with SW rendering, but it worse with bigger window size. showcase.mp4 |
|
Given how Compose rendering works right now, it's probably to be expected that a full screen window would have a lower frame rate. You can probably also see the progress bars page lagging when you go fullscreen |
d9af3c2 to
755389d
Compare
...wel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasure.kt
Show resolved
Hide resolved
755389d to
c7e6555
Compare
|
Pushed a few more fixes to improve scroll behavior for lager data sets (tables with more than 300 visible cells also had some lag even on MacOS). A few of the fixes I've implemented are:
Here are a few results I was able to gather using JFR:
Something you can notice is that the time to run the test cases has reduced drastically with less frame jank. Also worth mentioning that this is only happening for tables rendering ~300 cells (which are the cases in the samples). A more realistic scenario will probably have larger cells and less of them showing on screen (which may help with the performance). You can check it by applying the following patch file and running the .sh scripts:
I'll probably work on it a bit more to improve a bit more (I think there is still space for improvements), so if you notice that it still freezes, please let me know. Including the |
|
Nice job! I tried and it still feels laggy (i mean normal sized tables, not the extreme example matrix in first example). But i think maybe it is only perceived laggines now, because of the "buggy" behavior of the fixed column while scrolling: Screencast.From.2025-12-05.15-08-47.mp4 |
|
@morki thanks for keeping an eye on this PR. It definitely looks like the pinned column scroll is buggy, and needs to be fixed |
|
That's definetly a bug and related to the improvements I made yesterday. I think it's not getting applied to the fixed colums/rows. I'm already looking at it. Thanks for catching That was a quick fix. The branch already has the fix for it, but I'm still investigating other issues |
|
That was fast fix :) another bug i spotted is with selection when scrolling: Screencast.From.2025-12-05.15-35-17.mp4The main issue is at the end of the video, the beginning is just me trying to reproduce it. The selection jumps to stay in viewport and it feels unintuitive and only happens sometimes. |
| (scrollOffsetCoerced / averageVisibleLineSizeWithSpacing) | ||
| .toInt() | ||
| .coerceAtLeast(0) | ||
| .coerceAtMost(totalLineCount() - 1) |
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.
Bug: Scrollbar crashes when snapping on empty table
When the table is empty (totalLineCount() returns 0), the snapTo function computes a line index by calling coerceAtMost(totalLineCount() - 1) which evaluates to coerceAtMost(-1). Since the value is already coerced to at least 0, this results in index = -1. The subsequent call to snapToLine(-1, offset) leads to scrollToRow(-1, ...) or scrollToColumn(-1, ...), which eventually fails the require(row >= 0f && column >= 0f) check in LazyTableScrollPosition.update(), throwing an exception. This can occur if scrollTo is invoked when the scroll distance exceeds the viewport size on an empty table.
|
Saw that the cursor is blinking during scrolling (changes to a different pointer type). Not sure whether it's okay or not, but didn't see that in your videos Screen.Recording.2025-12-08.at.17.57.26.mov |
bba00d8 to
8eb53a9
Compare
|
@morki I'm almost wrapping up a new batch of improvements that should fix that issue. Let me know if you still see that happening. The changed I've made are adding support for LookAhead and support Prefetch (default to prefetch 4 items) and created CacheWindow similar to LazyList/Grid components. Both should help in the performance and the feeling of "lag". @daaria-s this seems to be related to the "drag handle" that allow resise the table. I'll check if I can maybe disable it during scroll. |
platform/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/OverflowBox.kt
Show resolved
Hide resolved
...ain/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableCacheWindowPrefetchStrategy.kt
Outdated
Show resolved
Hide resolved
|
I cannot reproduce the selection bug anymore, but now i cannot drag the columns :) |
8eb53a9 to
6f679b3
Compare
...undation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasureResult.kt
Show resolved
Hide resolved
...undation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasureResult.kt
Show resolved
Hide resolved
I'm sorry about that, I was checking if remembering the modifiers would help to reduce memory usage and forgot to revert 🤦♂️ The branch is correct now |
6f679b3 to
beb9678
Compare
...undation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasureResult.kt
Show resolved
Hide resolved
beb9678 to
8614c8e
Compare
...ation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTablePrefetchStrategy.kt
Outdated
Show resolved
Hide resolved
...n/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasuredItemProvider.kt
Show resolved
Hide resolved
|
Every iteration it feels a bit faster, very good job :) Still somewhat laggy but better :) One more observation - it feels more laggy scrolling top then bottom (I scroll the same speed using mouse dragging on scrollbar). Maybe prefetching only in one direction? Screencast.From.2025-12-09.17-04-09.mp4 |
I'll take a second look at the prefetch logic, but I think I is keeping a window of 4 items before and after in the state by default. Thanks for the hint :) The problem was actually something else. One of the logics to optimize scroll was not using the It should be good now. I'm also testing to replace the 'HashMap's used on cache with the 'androidx.collection.LruCache'. The benchmark tests are running (they take ~1h). I'll check the results tomorrow 🤞 |
8614c8e to
2ad9621
Compare
...form/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTable.kt
Show resolved
Hide resolved
2ad9621 to
e51029c
Compare
...form/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTable.kt
Outdated
Show resolved
Hide resolved
|
LRU caches helped, but it has a slight bigger memory footprint (however, IMO, it worth when thinking about the performance). I also pushed some more guard-rails for parameters and fixed some types that were incorrect (ints using float types). Also pushed more changes to the benchmark gist with changes to include more checks (GC times, CPU usage, Heap Summary, etc.) |
042f40a to
7e271da
Compare
...n/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasuredItemProvider.kt
Outdated
Show resolved
Hide resolved
...n/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableMeasuredItemProvider.kt
Outdated
Show resolved
Hide resolved
7e271da to
4ca517c
Compare
...ion/src/main/kotlin/org/jetbrains/jewel/foundation/lazy/table/LazyTableItemKeyPositionMap.kt
Show resolved
Hide resolved
4ca517c to
5cf16f4
Compare
| .then(modifier), | ||
| contentAlignment = contentAlignment, | ||
| ) { | ||
| rememberOverflowBoxScope(isOverflowVisible, this).content() |
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.
Bug: OverflowBox isOverflowing reports incorrect value when content fits
The OverflowBoxScope.isOverflowing value passed to the content scope reflects isOverflowVisible (whether the hover delay elapsed), not whether content actually overflows the constraints. The actual overflow check (predictWidth > constraintWith or predictHeight > constraintHeight) happens during measurement but isn't propagated to the scope. This causes isOverflowing to be true when a user hovers over content that fits within constraints, making any visual overflow indicators (shadows, fades) appear incorrectly.
- Add LazyTable component for large datasets - Create OverflowBox utility for smart cell content overflow on hover - Provide themed TableViewCell and TableViewHeader components with IntelliJ theme integration - Added theme classes based on IJ table Co-authored-by: James Rose <[email protected]>
5cf16f4 to
97df4e2
Compare
|
|
||
| // Trick for overflow layout: compensate layout alignment by offsetting half of the extra space. | ||
| val xOffset = if (overflowingX) (predictWidth - constraintWith) / 2 else 0 | ||
| val yOffset = if (overflowingY) (predictHeight - constraintHeight) / 2 else 0 |
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.
Bug: OverflowBox offset calculation uses intrinsic size instead of measured size
The xOffset and yOffset calculations use predictWidth and predictHeight (intrinsic sizes from lines 73-74), but the layout reports placements.width and placements.height (measured sizes from line 97). When these differ—which can happen with content that fills available space—the centering calculation produces incorrect offsets. For example, if intrinsic width is 200px, constraint is 100px, and measured width is 150px, the offset would be 50px based on intrinsic size, but this doesn't properly center a 150px measured child. The offset calculation should use placements.width and placements.height instead of predictWidth and predictHeight.


Note
This PR includes the
platform/jewel/docs/lazy-table.mdfile. It's a base documentation of features and how to implement them to help during reviewMy plan was to use it mainly to support review, but I can put more effort into it if we want to store it somewhere
Note
Most of the work was imported from another branch/fork that had an implementation of it almost complete.
This PR only addresses a few remaining items and adapts the code to Jewel code-style/standards and update to match proposal from the ticket comments
Original Patch File
Evidences
Release notes
New features
Note
Introduces an experimental LazyTable (2D lazy grid) with selection/drag/resize, themed table cells/styles, OverflowBox, scrollbar adapter overloads, docs, and showcase samples.
org.jetbrains.jewel.foundation.lazy.table.*: 2D lazy layout, pinned rows/columns, bi‑directional scrolling, cache-window prefetch, animate scroll, selection managers, drag & drop (rows/columns), resize hooks, scrollbar adapters (rememberTable*ScrollbarAdapter).OverflowBoxfor hover‑triggered content overflow.lazy.selectableandlazy.draggableutilities.TableViewCell,TableViewHeaderCell,SimpleTextTableViewCell,SimpleTextTableViewHeaderCellwith resize handles, selection/drag integration.TableCellState, table styling (TableStyle,TableColors,TableMetrics,TableCellColors) andJewelTheme.tableStyle.Scrollbarnow supports adapter-provided overloads for vertical/horizontal.TableStyle.platform/jewel/docs/lazy-table.mdwith usage and feature guide.androidx.collectiondependency; API dumps updated.Written by Cursor Bugbot for commit 97df4e2. This will update automatically on new commits. Configure here.