From 98f3fa8a4a8b56397cedf6f7470204e3de8641c3 Mon Sep 17 00:00:00 2001 From: Jonas Kalderstam Date: Wed, 12 Jun 2024 10:06:39 +0200 Subject: [PATCH] Fixed crash with table spans --- .../ui/compose/html/LinearArticleContent.kt | 4 +- .../feeder/ui/compose/layouts/Table.kt | 63 ++++++++++++++----- .../ui/compose/layouts/TableDataTest.kt | 21 +++++++ 3 files changed, 71 insertions(+), 17 deletions(-) create mode 100644 app/src/test/java/com/nononsenseapps/feeder/ui/compose/layouts/TableDataTest.kt diff --git a/app/src/main/java/com/nononsenseapps/feeder/ui/compose/html/LinearArticleContent.kt b/app/src/main/java/com/nononsenseapps/feeder/ui/compose/html/LinearArticleContent.kt index c0e79cc5a..e807b886a 100644 --- a/app/src/main/java/com/nononsenseapps/feeder/ui/compose/html/LinearArticleContent.kt +++ b/app/src/main/java/com/nononsenseapps/feeder/ui/compose/html/LinearArticleContent.kt @@ -728,8 +728,8 @@ fun LinearTableContent( val borderColor = MaterialTheme.colorScheme.outlineVariant Table( tableData = linearTable.toTableData(), - allowHorizontalScroll = allowHorizontalScroll, modifier = modifier, + allowHorizontalScroll = allowHorizontalScroll, content = { row, column -> Column( verticalArrangement = Arrangement.spacedBy(4.dp), @@ -1387,7 +1387,7 @@ private fun PreviewColSpanningTable() { } fun LinearTable.toTableData(): TableData { - return TableData( + return TableData.fromCells( cells = cells .asSequence() diff --git a/app/src/main/java/com/nononsenseapps/feeder/ui/compose/layouts/Table.kt b/app/src/main/java/com/nononsenseapps/feeder/ui/compose/layouts/Table.kt index a82cae027..8f1af7d21 100644 --- a/app/src/main/java/com/nononsenseapps/feeder/ui/compose/layouts/Table.kt +++ b/app/src/main/java/com/nononsenseapps/feeder/ui/compose/layouts/Table.kt @@ -28,7 +28,6 @@ fun Table( tableData: TableData, modifier: Modifier = Modifier, allowHorizontalScroll: Boolean = true, - caption: @Composable (() -> Unit)? = null, content: @Composable (row: Int, column: Int) -> Unit, ) { val columnWidths = remember { mutableStateMapOf() } @@ -57,7 +56,7 @@ fun Table( } } }, - ) { measurables, constraints -> + ) { measurables, _ -> val placeables = measurables.mapIndexed { index, measurable -> val tableCell = tableData.cells[index] @@ -170,9 +169,7 @@ private fun TableWithPaddingPreview() { @Composable private fun TableCaptionPreview() { Surface { - Table(tableData = TableData(3, 3), caption = { - Text("Table caption") - }) { row, column -> + Table(tableData = TableData(3, 3)) { row, column -> Box( modifier = Modifier @@ -183,7 +180,8 @@ private fun TableCaptionPreview() { } } -data class TableData( +@Suppress("DataClassPrivateConstructor") +data class TableData private constructor( val cells: List, ) { constructor(row: Int, column: Int) : this( @@ -198,22 +196,57 @@ data class TableData( ) init { - var lastRowSpan = 0 - var lastColSpan = 0 + var lastSpanned = false for (cell in cells) { - check(cell.rowSpan >= lastRowSpan) { - "Cells must be sorted in order of increasing spans" + val isSpanned = cell.rowSpan > 1 || cell.colSpan > 1 + check(!lastSpanned && !isSpanned || isSpanned) { + "Spanned cells should come after non-spanned cells" } - check(cell.colSpan >= lastColSpan) { - "Cells must be sorted in order of increasing spans" - } - lastRowSpan = cell.rowSpan - lastColSpan = cell.colSpan + lastSpanned = isSpanned } } val rows: Int = cells.maxOf { it.row + it.rowSpan } val columns: Int = cells.maxOf { it.column + it.colSpan } + + companion object { + /** + * Ensures the cells are correctly sorted + */ + fun fromCells(cells: List): TableData { + return TableData( + cells = + cells + .sortedWith { a, b -> + // Spanned in both dimensions should come last of all + val aIsDoubleSpanned = a.rowSpan > 1 && a.colSpan > 1 + val bIsDoubleSpanned = b.rowSpan > 1 && b.colSpan > 1 + + if (aIsDoubleSpanned && !bIsDoubleSpanned) { + return@sortedWith 1 + } else if (!aIsDoubleSpanned && bIsDoubleSpanned) { + return@sortedWith -1 + } + + // Spanned cells should come after non-spanned cells + val aIsSpanned = a.rowSpan > 1 || a.colSpan > 1 + val bIsSpanned = b.rowSpan > 1 || b.colSpan > 1 + + if (aIsSpanned && !bIsSpanned) { + return@sortedWith 1 + } else if (!aIsSpanned && bIsSpanned) { + return@sortedWith -1 + } + + // Then sort by location + if (a.row != b.row) { + return@sortedWith a.row.compareTo(b.row) + } + return@sortedWith a.column.compareTo(b.column) + }, + ) + } + } } data class TableCell( diff --git a/app/src/test/java/com/nononsenseapps/feeder/ui/compose/layouts/TableDataTest.kt b/app/src/test/java/com/nononsenseapps/feeder/ui/compose/layouts/TableDataTest.kt new file mode 100644 index 000000000..967339568 --- /dev/null +++ b/app/src/test/java/com/nononsenseapps/feeder/ui/compose/layouts/TableDataTest.kt @@ -0,0 +1,21 @@ +package com.nononsenseapps.feeder.ui.compose.layouts + +import org.junit.Test +import kotlin.test.assertEquals + +class TableDataTest { + @Test + fun `sorting is done correctly`() { + val tableData = + TableData.fromCells( + listOf( + TableCell(row = 4, column = 0, rowSpan = 2, colSpan = 2), + TableCell(row = 2, column = 0, rowSpan = 2, colSpan = 1), + TableCell(row = 1, column = 0, rowSpan = 1, colSpan = 2), + TableCell(row = 0, column = 0, rowSpan = 1, colSpan = 1), + ), + ) + + assertEquals(6, tableData.rows, "Should not have crashed in constructor") + } +}