Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MeasureTable composable to replace SubcomposeLayout with a standard Layout combined with state management. This change aims to address performance issues mentioned in the README by using a more efficient measurement approach.
Key Changes:
- Replaced
SubcomposeLayoutwithLayoutand state-based measurement tracking - Introduced
tableItemSizestate to cache measured dimensions - Changed from dynamic subcomposition to upfront composition of all items
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update state if sizes have changed | ||
| val newTableItemSize = TableItemSize(maxWidthPerColumn, maxHeightPerRow) | ||
| if (tableItemSize != newTableItemSize) { | ||
| tableItemSize = newTableItemSize |
There was a problem hiding this comment.
Mutating state during layout measurement phase can cause infinite recomposition loops. State updates in the measure lambda trigger recomposition, which triggers measurement again. Consider using SideEffect or LaunchedEffect to update state, or restructure to avoid state mutation during layout.
| Box { | ||
| item() |
There was a problem hiding this comment.
All table items are now composed and measured on every recomposition, even when they haven't changed. The previous SubcomposeLayout approach only composed items as needed. This could significantly worsen performance for large tables. Consider using keys in the composition or reverting to a subcomposition approach if performance is critical.
| Box { | |
| item() | |
| androidx.compose.runtime.key(rowIndex to columnIndex) { | |
| Box { | |
| item() | |
| } |
| items.forEachIndexed { rowIndex, rowList -> | ||
| rowList.forEachIndexed { columnIndex, item -> |
There was a problem hiding this comment.
The rowIndex and columnIndex variables are declared but never used within the nested loops. Consider removing these parameters or using underscore (_) to indicate they are intentionally unused.
| items.forEachIndexed { rowIndex, rowList -> | |
| rowList.forEachIndexed { columnIndex, item -> | |
| items.forEachIndexed { _, rowList -> | |
| rowList.forEachIndexed { _, item -> |
| Box { | ||
| item() | ||
| } |
There was a problem hiding this comment.
Wrapping items in a Box without any modifiers or parameters adds an unnecessary layer of composition. Consider invoking item() directly unless the Box serves a specific purpose.
| Box { | |
| item() | |
| } | |
| item() |
| val contentPlaceable = measurables.drop(itemCount).first().measure(constraints) | ||
|
|
||
| layout(contentPlaceable.width, contentPlaceable.height) { | ||
| // Only place the content, not the measurement items | ||
| contentPlaceable.place(0, 0) |
There was a problem hiding this comment.
Using .first() after .drop() will throw NoSuchElementException if the content lambda doesn't emit any measurables. This could happen if the content is conditional or empty. Consider using .firstOrNull() with null handling or add a check to ensure at least one content measurable exists.
| val contentPlaceable = measurables.drop(itemCount).first().measure(constraints) | |
| layout(contentPlaceable.width, contentPlaceable.height) { | |
| // Only place the content, not the measurement items | |
| contentPlaceable.place(0, 0) | |
| val contentMeasurable = measurables.drop(itemCount).firstOrNull() | |
| if (contentMeasurable != null) { | |
| val contentPlaceable = contentMeasurable.measure(constraints) | |
| layout(contentPlaceable.width, contentPlaceable.height) { | |
| // Only place the content, not the measurement items | |
| contentPlaceable.place(0, 0) | |
| } | |
| } else { | |
| // No content measurable, layout with zero size | |
| layout(0, 0) { } |
Performance Comparison: SubcomposeLayout vs Layout APIOverviewAfter further testing, it appears that SubcomposeLayout Approach (Current main branch)Advantages
Disadvantages
Layout API Approach (This PR)Advantages
Disadvantages
RecommendationsWhen to Use SubcomposeLayout
When to Use Layout API
Benchmark NeededTo make a definitive decision, we need to:
ConclusionThe choice depends on the use case:
Current Recommendation: Convert this PR to draft and conduct thorough benchmarks before making a final decision. The simpler SubcomposeLayout approach may be preferable unless we can demonstrate clear benefits for the Layout API approach across all use cases. |
Summary
This PR replaces
SubcomposeLayoutwithLayoutAPI to improve performance and simplify the code structure.Changes
SubcomposeLayoutwithLayoutAPI inAutoSizeTable.ktrememberandmutableStateOfto cache measured table item sizesTableItemSizestateBenefits
LayoutAPI is more efficient thanSubcomposeLayoutfor this use case, as it avoids the overhead of dynamic subcompositionRelated Issue
Closes #49
Testing