From eba4d2a6de846cb37f5db1798b3767d30bbfe441 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 5 Sep 2026 22:07:22 +0200 Subject: [PATCH 1/2] Say what a sheet leaves out, and cut it to what the device can show A sheet past the html limits was truncated with nothing on screen to say so, which is what a free 2 star of 23 Aug reported as "won't load more than 10,000 lines". odrcore 6.11.0 added HtmlView.sheetCut(), so the app can now name what was left out: CoreLoader carries the core's own answer on HostedView, it rides through LoadedDocument beside the part titles, and DocumentFragment raises a bar for the sheet on screen - "Showing the first 5,000 of 20,000 rows" - taking it down again when the reader moves to a sheet that is whole. The limit itself was measured and is now the device's, in SpreadsheetBudget: 50,000 cells below 3 GB of memory, 100,000 below 6 GB, 150,000 above. It was 500,000 for every phone. That is a cut rather than a raise because the budget is the WebView's, not the core's. Measured on a 2 GB API 31 emulator and an 8 GB API 36 one, a rendered cell costs 10-20 KB in the renderer process against some 226 bytes of html: the 4.5 MB odr-public register renders 500,000 cells as 113 MB of html, which climbs to 4.7 GB in the renderer on the big device - and on the small one the load dies mid-stream after 80 seconds and the user is told the file could not be opened. A limit above what can be laid out does not show more of the document, it shows none of it. Which is also why there is no button offering to relax the cut, as #637 proposed: there is no step between what is rendered and what the sheet holds that a phone survives. The lever that would move the ceiling is core-side and filed as OpenDocument.core#822 - 55% of a sheet's html is style attributes drawn from 26 distinct strings. Measurements and the failure modes are in the issue. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BG8LdxWJMbkZXwurh91Wu2 --- .../droid/background/DocumentParcelTest.kt | 15 ++++ .../app/opendocument/droid/test/CoreTest.kt | 71 ++++++++++++++++++ .../droid/background/CoreLoader.kt | 31 +++++--- .../droid/background/LoadedDocument.kt | 9 +++ .../opendocument/droid/background/SheetCut.kt | 51 +++++++++++++ .../droid/background/SpreadsheetBudget.kt | 57 ++++++++++++++ .../opendocument/droid/ui/SnackbarHelper.kt | 18 +++++ .../droid/ui/activity/DocumentFragment.kt | 74 +++++++++++++++++++ app/src/main/res/values/strings.xml | 5 ++ .../droid/background/SpreadsheetBudgetTest.kt | 51 +++++++++++++ 10 files changed, 370 insertions(+), 12 deletions(-) create mode 100644 app/src/main/java/app/opendocument/droid/background/SheetCut.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt create mode 100644 app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt diff --git a/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt index 1bc2662f7707..0e996f45f478 100644 --- a/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt @@ -104,6 +104,8 @@ class DocumentParcelTest { Uri.parse("http://localhost:29665/file/odr/1.html"), Uri.parse("http://localhost:29665/file/odr/2.html"), ), + // the middle sheet is the only one the budget cut + listOf(null, SheetCut(80000, 12, 8333, 12), null), isEditable = true, readsAsDocument = true, ) @@ -116,6 +118,17 @@ class DocumentParcelTest { assertEquals(document.partUris, restored.partUris) assertTrue(restored.isEditable) assertTrue(restored.readsAsDocument) + + assertNull(restored.partCuts[0]) + assertNull(restored.partCuts[2]) + + val cut = checkNotNull(restored.partCuts[1]) + assertEquals(80000, cut.contentRows) + assertEquals(12, cut.contentColumns) + assertEquals(8333, cut.renderedRows) + assertEquals(12, cut.renderedColumns) + assertTrue(cut.rowsWereCut) + assertEquals(false, cut.columnsWereCut) } /** Everything but a spreadsheet: one part, and the core does not name it. */ @@ -133,6 +146,7 @@ class DocumentParcelTest { ), listOf(null), listOf(Uri.parse("http://localhost:29665/file/odr/document.html")), + listOf(null), isEditable = false, readsAsDocument = true, ), @@ -141,6 +155,7 @@ class DocumentParcelTest { assertEquals(1, restored.partTitles.size) assertNull(restored.partTitles[0]) + assertNull(restored.partCuts[0]) assertEquals(false, restored.isEditable) assertTrue(restored.readsAsDocument) } diff --git a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt index 7c5739120214..a92f1352d5df 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt @@ -6,6 +6,7 @@ import androidx.test.platform.app.InstrumentationRegistry import app.opendocument.core.FileType import app.opendocument.core.OdrException import app.opendocument.droid.background.CoreLoader +import app.opendocument.droid.background.SpreadsheetBudget import app.opendocument.droid.nonfree.CrashManager import java.io.File import java.io.FileOutputStream @@ -286,6 +287,51 @@ class CoreTest { return URL(views.first().url).readText() } + /** + * A sheet past what the device can lay out says how much of it was written - the numbers the + * bar over the document names. + * + * The sheet is generated rather than shipped: `SpreadsheetBudget` answers per device, so a + * fixture big enough to be cut on one would be too small on the next. + */ + @Test + fun aSheetPastTheBudgetSaysWhatItLeftOut() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + val budget = SpreadsheetBudget.cells(context) + + val columns = 10 + val expectedRows = (budget / columns).toInt() + val rows = expectedRows + 500 + + val views = + coreLoader.host( + prefix = "big-sheet", + inputPath = generateCsv(rows, columns).absolutePath, + cachePath = File(cacheDir(), "big_sheet_cache").path, + ) + + val cut = + checkNotNull(views.first().sheetCut) { "a sheet past the budget should report a cut" } + + Assert.assertEquals("every row written should be counted", rows, cut.contentRows) + Assert.assertEquals(columns, cut.contentColumns) + Assert.assertEquals("the budget decides the rows", expectedRows, cut.renderedRows) + Assert.assertEquals("a narrow sheet loses no columns", columns, cut.renderedColumns) + } + + /** The other side of it: a sheet written whole reports nothing to say. */ + @Test + fun aSheetInsideTheBudgetReportsNoCut() { + val views = + coreLoader.host( + prefix = "whole-sheet", + inputPath = spreadsheetTestFile.absolutePath, + cachePath = File(cacheDir(), "whole_sheet_cache").path, + ) + + views.forEach { Assert.assertNull("nothing was cut from " + it.name, it.sheetCut) } + } + @Test fun testSpreadsheetSheetNames() { val views = @@ -372,6 +418,31 @@ class CoreTest { private fun cacheDir(): File = InstrumentationRegistry.getInstrumentation().targetContext.cacheDir + /** A csv of [rows] x [columns] cells, each one a short string. */ + private fun generateCsv(rows: Int, columns: Int): File { + val target = File(cacheDir(), "generated-sheet.csv") + + target.bufferedWriter().use { writer -> + for (row in 1..rows) { + for (column in 1..columns) { + if (column > 1) { + writer.write(",") + } + + writer.write("r") + writer.write(row.toString()) + writer.write("c") + writer.write(column.toString()) + } + + writer.write("\n") + } + } + extracted += target + + return target + } + private fun extract(name: String): File { val instrumentation = InstrumentationRegistry.getInstrumentation() val target = File(instrumentation.targetContext.cacheDir, name) diff --git a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt index f025d4734c9b..102ad880b18a 100644 --- a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt @@ -99,6 +99,7 @@ class CoreLoader(private val context: Context) { file, views.map { it.name }, views.map { Uri.parse(it.url) }, + views.map { it.sheetCut }, isDocumentEditable, readsAsDocument, ) @@ -186,10 +187,12 @@ class CoreLoader(private val context: Context) { // document. PageView.setDarkeningAllowed picks between them htmlConfig.colorScheme = HtmlColorScheme.SYSTEM - // stated rather than inherited: a sheet past it is cut off silently + // stated rather than inherited, and this device's answer rather than one number for + // every phone - see SpreadsheetBudget. What is cut is reported rather than dropped + // silently: HostedView carries the core's own account of it htmlConfig.spreadsheetLimit = - TableDimensions(SPREADSHEET_LIMIT_ROWS, SPREADSHEET_LIMIT_COLUMNS) - htmlConfig.spreadsheetCellLimit = SPREADSHEET_LIMIT_CELLS + TableDimensions(SpreadsheetBudget.ROWS, SpreadsheetBudget.COLUMNS) + htmlConfig.spreadsheetCellLimit = SpreadsheetBudget.cells(context) htmlConfig.spreadsheetLimitByContent = true val cacheDirectory = File(cachePath) @@ -204,6 +207,14 @@ class CoreLoader(private val context: Context) { HostedView( view.name(), "http://$SERVER_URL_HOST:$sharedServerPort/file/$prefix/" + view.path(), + view.sheetCut()?.let { + SheetCut( + it.content.rows, + it.content.columns, + it.rendered.rows, + it.rendered.columns, + ) + }, ) } } @@ -330,8 +341,11 @@ class CoreLoader(private val context: Context) { document = null } - /** A translated view of a document, ready to be opened in the WebView. */ - data class HostedView(val name: String, val url: String) + /** + * A translated view of a document, ready to be opened in the WebView. [sheetCut] is set only + * where the budget cut the sheet this view renders. + */ + data class HostedView(val name: String, val url: String, val sheetCut: SheetCut?) /** An encrypted file whose format odrcore cannot decrypt, whatever the password. */ class UndecryptableFile(path: String) : IOException("cannot be decrypted: $path") @@ -339,13 +353,6 @@ class CoreLoader(private val context: Context) { companion object { private const val TAG = "CoreLoader" - /** The largest sheet region translated - every cell in it becomes a ``. */ - private const val SPREADSHEET_LIMIT_ROWS = 100000 - private const val SPREADSHEET_LIMIT_COLUMNS = 500 - - /** Bounds the rows by the sheet's width. */ - private const val SPREADSHEET_LIMIT_CELLS = 500000L - /** * The one http server of the process, started on the first [initialize] and never stopped. * diff --git a/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt b/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt index 283a3fdc9d97..b733c5caf11c 100644 --- a/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt +++ b/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt @@ -9,6 +9,9 @@ import android.os.Parcelable * be, and one uri per part (spreadsheets have one per sheet, everything else a single one with a * null title). * + * [partCuts] runs alongside those: the entry for a sheet the budget cut says how much of it the + * markup carries, and every other part's is null. + * * [isEditable] and [readsAsDocument] are the core's own answers about this document, never a guess * from its mime type - see `CoreLoader.isDocumentEditable` and `CoreLoader.readsAsDocument`. */ @@ -17,6 +20,7 @@ class LoadedDocument( val file: IdentifiedFile, val partTitles: List, val partUris: List, + val partCuts: List, val isEditable: Boolean, val readsAsDocument: Boolean, ) : Parcelable { @@ -28,6 +32,7 @@ class LoadedDocument( parcel.writeParcelable(file, 0) parcel.writeList(partTitles) parcel.writeList(partUris) + parcel.writeList(partCuts) ParcelUtil.writeBoolean(parcel, isEditable) ParcelUtil.writeBoolean(parcel, readsAsDocument) } @@ -51,11 +56,15 @@ class LoadedDocument( val partUris = ArrayList() parcel.readList(partUris, classLoader) + val partCuts = ArrayList() + parcel.readList(partCuts, classLoader) + return LoadedDocument( request, file, partTitles, partUris, + partCuts, ParcelUtil.readBoolean(parcel), ParcelUtil.readBoolean(parcel), ) diff --git a/app/src/main/java/app/opendocument/droid/background/SheetCut.kt b/app/src/main/java/app/opendocument/droid/background/SheetCut.kt new file mode 100644 index 000000000000..30044b3d35fd --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/SheetCut.kt @@ -0,0 +1,51 @@ +package app.opendocument.droid.background + +import android.os.Parcel +import android.os.Parcelable + +/** + * What a sheet's markup leaves out: the extent its cells span against the extent that was written. + * + * odrcore's own answer (`HtmlView.sheetCut()`), and only present for a sheet [SpreadsheetBudget] + * cut - a sheet written whole has none. + */ +class SheetCut( + val contentRows: Int, + val contentColumns: Int, + val renderedRows: Int, + val renderedColumns: Int, +) : Parcelable { + + val rowsWereCut: Boolean + get() = renderedRows < contentRows + + val columnsWereCut: Boolean + get() = renderedColumns < contentColumns + + override fun describeContents(): Int = 0 + + override fun writeToParcel(parcel: Parcel, flags: Int) { + parcel.writeInt(contentRows) + parcel.writeInt(contentColumns) + parcel.writeInt(renderedRows) + parcel.writeInt(renderedColumns) + } + + companion object { + // @JvmField because the framework looks CREATOR up as a static field + @JvmField + val CREATOR: Parcelable.Creator = + object : Parcelable.Creator { + override fun createFromParcel(parcel: Parcel): SheetCut = + // in the order writeToParcel wrote them + SheetCut( + parcel.readInt(), + parcel.readInt(), + parcel.readInt(), + parcel.readInt(), + ) + + override fun newArray(size: Int): Array = arrayOfNulls(size) + } + } +} diff --git a/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt b/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt new file mode 100644 index 000000000000..1ec2b26d29a2 --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt @@ -0,0 +1,57 @@ +package app.opendocument.droid.background + +import android.app.ActivityManager +import android.content.Context +import androidx.core.content.getSystemService + +/** + * How much of a sheet is translated to html. + * + * The budget is the *WebView's*, not the core's. Every cell translated becomes a `` the browser + * engine then holds: measured on two devices, a rendered cell costs 10-20 KB in the WebView's + * renderer process against some 226 bytes of html, so a sheet the core writes in a second can be + * one no phone can lay out. A budget too high does not show more of the document - the page fails + * to load and the user is told the file could not be opened. + * + * Hence the ladder below, and hence its steps being small next to what the core would allow. + */ +object SpreadsheetBudget { + + /** + * The furthest a sheet is followed, before [cells] narrows it further. Bounds the two + * directions on their own, so one very long or very wide sheet cannot spend the whole budget in + * a direction nothing can scroll to. + */ + const val ROWS = 100000 + const val COLUMNS = 500 + + /** What the device this is running on can afford to show. */ + fun cells(context: Context): Long { + val activityManager = context.getSystemService() + val memoryInfo = ActivityManager.MemoryInfo() + activityManager?.getMemoryInfo(memoryInfo) + + return cellsFor(memoryInfo.totalMem, activityManager?.isLowRamDevice == true) + } + + /** + * The device's memory decides, not its api level or its screen: what is being budgeted is the + * renderer process, which is killed by the same low memory killer as everything else. + * + * [totalMemoryBytes] of zero is what a device that would not answer looks like, and takes the + * smallest step. + */ + fun cellsFor(totalMemoryBytes: Long, isLowRamDevice: Boolean): Long { + if (isLowRamDevice || totalMemoryBytes < 3L * GIGABYTE) { + return 50000 + } + + if (totalMemoryBytes < 6L * GIGABYTE) { + return 100000 + } + + return 150000 + } + + private const val GIGABYTE = 1024L * 1024L * 1024L +} diff --git a/app/src/main/java/app/opendocument/droid/ui/SnackbarHelper.kt b/app/src/main/java/app/opendocument/droid/ui/SnackbarHelper.kt index 4fca9f7ff608..d45bda58cbcf 100644 --- a/app/src/main/java/app/opendocument/droid/ui/SnackbarHelper.kt +++ b/app/src/main/java/app/opendocument/droid/ui/SnackbarHelper.kt @@ -65,6 +65,24 @@ object SnackbarHelper { ) } + /** Same, where the message carries numbers and is built rather than looked up. */ + fun show( + activity: Activity, + message: String, + callback: Runnable?, + isIndefinite: Boolean, + isError: Boolean, + ) { + show( + activity, + activity.getString(android.R.string.ok), + message, + callback, + isIndefinite, + isError, + ) + } + private fun show( activity: Activity, buttonText: String, diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index 9d1d8e3354fa..8676df92a499 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -34,6 +34,7 @@ import app.opendocument.droid.background.LoadedDocument import app.opendocument.droid.background.NightModeSetting import app.opendocument.droid.background.PaginationSetting import app.opendocument.droid.background.ReviewInvitation +import app.opendocument.droid.background.SheetCut import app.opendocument.droid.nonfree.AnalyticsConstants import app.opendocument.droid.nonfree.AnalyticsManager import app.opendocument.droid.nonfree.CrashManager @@ -44,6 +45,7 @@ import app.opendocument.droid.ui.widget.PageView import app.opendocument.droid.ui.widget.ProgressDialogFragment import com.google.android.material.tabs.TabLayout import java.io.FileNotFoundException +import java.text.NumberFormat class DocumentFragment : Fragment(), DocumentLoader.Listener { @@ -83,6 +85,12 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { */ private var positionToRestore: ReadingPosition? = null + /** + * Whether the bar on show is the one [reportSheetCut] raised, so that moving to a whole sheet + * takes it down and leaves anything else alone. + */ + private var sheetCutReported = false + /** A tab and how far down it, which survives the document being translated again. */ private data class ReadingPosition(val tab: Int, val scrollFraction: Float) @@ -705,6 +713,9 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { tabLayout.getTabAt(restored?.tab?.coerceAtMost(pages - 1) ?: 0)?.select() } else if (pages == 1) { loadData(document.partUris[0].toString()) + + // the tab listener says it for a document that has tabs, and this one has none + reportSheetCut(document, 0) } prepareActions(document) @@ -982,6 +993,67 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { ) } + /** + * Says what a sheet leaves out, for the sheet being shown. + * + * A sheet past what the device can lay out is cut by `SpreadsheetBudget`, and a spreadsheet + * that simply stops is what a user reports as a document that will not load. The bar names the + * numbers instead; there is nothing to offer beyond them, since the budget is what the WebView + * can hold and not a preference. + */ + private fun reportSheetCut(document: LoadedDocument, part: Int) { + val cut = document.partCuts.getOrNull(part) + + if (cut == null) { + // the reader moved to a sheet that is whole, so the bar about the one before it is no + // longer about anything on screen. only ours: any other bar is about the document + if (sheetCutReported) { + sheetCutReported = false + + SnackbarHelper.dismiss(requireActivity()) + } + + return + } + + SnackbarHelper.show( + requireActivity(), + describeSheetCut(cut), + null, + isIndefinite = false, + isError = false, + ) + + sheetCutReported = true + } + + private fun describeSheetCut(cut: SheetCut): String { + val numbers = NumberFormat.getIntegerInstance() + + return when { + cut.rowsWereCut && cut.columnsWereCut -> + getString( + R.string.toast_hint_sheet_cut_rows_and_columns, + numbers.format(cut.renderedRows), + numbers.format(cut.contentRows), + numbers.format(cut.renderedColumns), + numbers.format(cut.contentColumns), + ) + cut.columnsWereCut -> + getString( + R.string.toast_hint_sheet_cut_columns, + numbers.format(cut.renderedColumns), + numbers.format(cut.contentColumns), + ) + else -> + getString( + R.string.toast_hint_sheet_cut_rows, + numbers.format(cut.renderedRows), + numbers.format(cut.contentRows), + ) + } + } + fun openWith(activity: Activity) { doReopen(activity, requireLastRequest(), state.lastFile, share = false) } @@ -1140,6 +1212,8 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { state.lastSelectedTab = tab.position loadData(lastDocument.partUris[tab.position].toString()) + + reportSheetCut(lastDocument, tab.position) } override fun onTabUnselected(tab: TabLayout.Tab) {} diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index bc1c7ffd8740..18006939e632 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -8,6 +8,11 @@ The file may be damaged, or something went wrong on our side. If you think the file is fine, tell us at support@opendocument.app Couldn\'t find file. Maybe it doesn\'t exist any longer? Unsupported file format. Try opening it in another app. + + This sheet is too big to show in full. Showing the first %1$s of %2$s rows. + This sheet is too big to show in full. Showing the first %1$s of %2$s columns. + This sheet is too big to show in full. Showing the first %1$s of %2$s rows and %3$s of %4$s columns. Not happy with how the file is displayed? Open it in another app instead. No place to save the file to chosen. File could not be saved. Please contact support@opendocument.app diff --git a/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt b/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt new file mode 100644 index 000000000000..87cd42c0b4c8 --- /dev/null +++ b/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt @@ -0,0 +1,51 @@ +package app.opendocument.droid.background + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The device's memory is passed into [SpreadsheetBudget.cellsFor], so every step is reachable + * without the phones it stands for. + */ +class SpreadsheetBudgetTest { + + @Test + fun aSmallDeviceGetsTheSmallestBudget() { + assertEquals(50000L, cellsFor(gigabytes = 2)) + assertEquals(50000L, cellsFor(gigabytes = 8, isLowRamDevice = true)) + } + + @Test + fun anOrdinaryPhoneGetsTheMiddleOne() { + assertEquals(100000L, cellsFor(gigabytes = 4)) + } + + @Test + fun aLargeDeviceGetsTheLargest() { + assertEquals(150000L, cellsFor(gigabytes = 8)) + assertEquals(150000L, cellsFor(gigabytes = 16)) + } + + /** A device that would not say how much memory it has is treated as one that has little. */ + @Test + fun anUnansweredDeviceGetsTheSmallestBudget() { + assertEquals(50000L, cellsFor(gigabytes = 0)) + } + + /** The budget only ever grows with the memory: a step in the wrong place would show less. */ + @Test + fun theLadderNeverFalls() { + var previous = 0L + + for (gigabytes in 0..24) { + val cells = cellsFor(gigabytes) + + assert(cells >= previous) { "$gigabytes GB gets $cells, less than the device below it" } + + previous = cells + } + } + + private fun cellsFor(gigabytes: Int, isLowRamDevice: Boolean = false): Long = + SpreadsheetBudget.cellsFor(gigabytes * 1024L * 1024L * 1024L, isLowRamDevice) +} From a326ad0abc5158d2d14a32f9b181e8bb57146375 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 5 Sep 2026 22:16:07 +0200 Subject: [PATCH 2/2] Cut the comments back, and write the changelog entry The measurements and the failure modes belong in the commit and the issue, not in the source. What is left says what the reader of the code needs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0132LKna2nBJTFJKKq3J6fCe --- CHANGELOG.md | 8 +++++++ .../app/opendocument/droid/test/CoreTest.kt | 7 ++---- .../droid/background/CoreLoader.kt | 6 ++--- .../droid/background/LoadedDocument.kt | 3 +-- .../opendocument/droid/background/SheetCut.kt | 7 ++---- .../droid/background/SpreadsheetBudget.kt | 23 +++++-------------- .../droid/ui/activity/DocumentFragment.kt | 16 ++++--------- app/src/main/res/values/strings.xml | 3 +-- .../droid/background/SpreadsheetBudgetTest.kt | 5 +--- 9 files changed, 28 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c90cc126cc70..75be9c9fbb0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,14 @@ version code per locale under `fastlane/metadata/android//changelogs/`, written by `scripts/store-copy.py` before the release and uploaded by it. Play takes 500 characters, so not everything here reaches the store. +## Unreleased + +- A spreadsheet too big to show in full says so, and names how many of its rows + and columns are on screen. It used to stop without a word. +- How much of a sheet is shown follows the device's memory now, rather than one + number for every phone. A big sheet used to take the app past what the phone + could hold and fail to open at all. + ## 4.19.1 - The app no longer closes when the screen is rotated while an advertisement is diff --git a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt index a92f1352d5df..ee7e1b2a6ae4 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt @@ -288,11 +288,8 @@ class CoreTest { } /** - * A sheet past what the device can lay out says how much of it was written - the numbers the - * bar over the document names. - * - * The sheet is generated rather than shipped: `SpreadsheetBudget` answers per device, so a - * fixture big enough to be cut on one would be too small on the next. + * A cut sheet says how much of it was written. Generated rather than shipped, since + * `SpreadsheetBudget` answers per device. */ @Test fun aSheetPastTheBudgetSaysWhatItLeftOut() { diff --git a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt index 102ad880b18a..ca2d74ae83f5 100644 --- a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt @@ -187,9 +187,7 @@ class CoreLoader(private val context: Context) { // document. PageView.setDarkeningAllowed picks between them htmlConfig.colorScheme = HtmlColorScheme.SYSTEM - // stated rather than inherited, and this device's answer rather than one number for - // every phone - see SpreadsheetBudget. What is cut is reported rather than dropped - // silently: HostedView carries the core's own account of it + // stated rather than inherited, and this device's answer - see SpreadsheetBudget htmlConfig.spreadsheetLimit = TableDimensions(SpreadsheetBudget.ROWS, SpreadsheetBudget.COLUMNS) htmlConfig.spreadsheetCellLimit = SpreadsheetBudget.cells(context) @@ -343,7 +341,7 @@ class CoreLoader(private val context: Context) { /** * A translated view of a document, ready to be opened in the WebView. [sheetCut] is set only - * where the budget cut the sheet this view renders. + * where the budget cut this view's sheet. */ data class HostedView(val name: String, val url: String, val sheetCut: SheetCut?) diff --git a/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt b/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt index b733c5caf11c..aae89ca133d6 100644 --- a/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt +++ b/app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt @@ -9,8 +9,7 @@ import android.os.Parcelable * be, and one uri per part (spreadsheets have one per sheet, everything else a single one with a * null title). * - * [partCuts] runs alongside those: the entry for a sheet the budget cut says how much of it the - * markup carries, and every other part's is null. + * [partCuts] runs alongside them, null for every part but a sheet that was cut. * * [isEditable] and [readsAsDocument] are the core's own answers about this document, never a guess * from its mime type - see `CoreLoader.isDocumentEditable` and `CoreLoader.readsAsDocument`. diff --git a/app/src/main/java/app/opendocument/droid/background/SheetCut.kt b/app/src/main/java/app/opendocument/droid/background/SheetCut.kt index 30044b3d35fd..6f837538934e 100644 --- a/app/src/main/java/app/opendocument/droid/background/SheetCut.kt +++ b/app/src/main/java/app/opendocument/droid/background/SheetCut.kt @@ -4,10 +4,8 @@ import android.os.Parcel import android.os.Parcelable /** - * What a sheet's markup leaves out: the extent its cells span against the extent that was written. - * - * odrcore's own answer (`HtmlView.sheetCut()`), and only present for a sheet [SpreadsheetBudget] - * cut - a sheet written whole has none. + * How much of a sheet the markup carries against how much was written - odrcore's own + * `HtmlView.sheetCut()`, and only present where [SpreadsheetBudget] cut the sheet. */ class SheetCut( val contentRows: Int, @@ -37,7 +35,6 @@ class SheetCut( val CREATOR: Parcelable.Creator = object : Parcelable.Creator { override fun createFromParcel(parcel: Parcel): SheetCut = - // in the order writeToParcel wrote them SheetCut( parcel.readInt(), parcel.readInt(), diff --git a/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt b/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt index 1ec2b26d29a2..03856d8c4ecc 100644 --- a/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt +++ b/app/src/main/java/app/opendocument/droid/background/SpreadsheetBudget.kt @@ -7,21 +7,13 @@ import androidx.core.content.getSystemService /** * How much of a sheet is translated to html. * - * The budget is the *WebView's*, not the core's. Every cell translated becomes a `` the browser - * engine then holds: measured on two devices, a rendered cell costs 10-20 KB in the WebView's - * renderer process against some 226 bytes of html, so a sheet the core writes in a second can be - * one no phone can lay out. A budget too high does not show more of the document - the page fails - * to load and the user is told the file could not be opened. - * - * Hence the ladder below, and hence its steps being small next to what the core would allow. + * The budget is the *WebView's*, not the core's: a rendered cell costs 10-20 KB in the renderer + * process against some 226 bytes of html, so a budget too high shows none of the document rather + * than more of it - the page fails to load and the file is reported as one that cannot be opened. */ object SpreadsheetBudget { - /** - * The furthest a sheet is followed, before [cells] narrows it further. Bounds the two - * directions on their own, so one very long or very wide sheet cannot spend the whole budget in - * a direction nothing can scroll to. - */ + /** Each direction on its own, before [cells] narrows the two together. */ const val ROWS = 100000 const val COLUMNS = 500 @@ -35,11 +27,8 @@ object SpreadsheetBudget { } /** - * The device's memory decides, not its api level or its screen: what is being budgeted is the - * renderer process, which is killed by the same low memory killer as everything else. - * - * [totalMemoryBytes] of zero is what a device that would not answer looks like, and takes the - * smallest step. + * Memory decides, since what is budgeted is the renderer process. [totalMemoryBytes] of zero is + * a device that would not answer, and takes the smallest step. */ fun cellsFor(totalMemoryBytes: Long, isLowRamDevice: Boolean): Long { if (isLowRamDevice || totalMemoryBytes < 3L * GIGABYTE) { diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index 8676df92a499..78a118486489 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -86,8 +86,8 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { private var positionToRestore: ReadingPosition? = null /** - * Whether the bar on show is the one [reportSheetCut] raised, so that moving to a whole sheet - * takes it down and leaves anything else alone. + * Whether the bar on show is [reportSheetCut]'s, so moving to a whole sheet takes that one down + * and leaves any other alone. */ private var sheetCutReported = false @@ -714,7 +714,7 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { } else if (pages == 1) { loadData(document.partUris[0].toString()) - // the tab listener says it for a document that has tabs, and this one has none + // with tabs it is the tab listener that says this reportSheetCut(document, 0) } @@ -994,19 +994,13 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { } /** - * Says what a sheet leaves out, for the sheet being shown. - * - * A sheet past what the device can lay out is cut by `SpreadsheetBudget`, and a spreadsheet - * that simply stops is what a user reports as a document that will not load. The bar names the - * numbers instead; there is nothing to offer beyond them, since the budget is what the WebView - * can hold and not a preference. + * Names what the sheet on screen leaves out. Nothing is offered beyond the numbers: the budget + * is what the WebView can hold, not a preference. */ private fun reportSheetCut(document: LoadedDocument, part: Int) { val cut = document.partCuts.getOrNull(part) if (cut == null) { - // the reader moved to a sheet that is whole, so the bar about the one before it is no - // longer about anything on screen. only ours: any other bar is about the document if (sheetCutReported) { sheetCutReported = false diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 18006939e632..13c3b0850c12 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -8,8 +8,7 @@ The file may be damaged, or something went wrong on our side. If you think the file is fine, tell us at support@opendocument.app Couldn\'t find file. Maybe it doesn\'t exist any longer? Unsupported file format. Try opening it in another app. - + This sheet is too big to show in full. Showing the first %1$s of %2$s rows. This sheet is too big to show in full. Showing the first %1$s of %2$s columns. This sheet is too big to show in full. Showing the first %1$s of %2$s rows and %3$s of %4$s columns. diff --git a/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt b/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt index 87cd42c0b4c8..625789e8f95c 100644 --- a/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt +++ b/app/src/test/java/app/opendocument/droid/background/SpreadsheetBudgetTest.kt @@ -3,10 +3,7 @@ package app.opendocument.droid.background import org.junit.Assert.assertEquals import org.junit.Test -/** - * The device's memory is passed into [SpreadsheetBudget.cellsFor], so every step is reachable - * without the phones it stands for. - */ +/** [SpreadsheetBudget.cellsFor] takes the memory, so every step is reachable without the phones. */ class SpreadsheetBudgetTest { @Test