diff --git a/.plans/issue-17-page-overflow.md b/.plans/issue-17-page-overflow.md new file mode 100644 index 0000000..b7f0937 --- /dev/null +++ b/.plans/issue-17-page-overflow.md @@ -0,0 +1,109 @@ +# Issue #17: Fix page overflow — bounds checking and content splitting + +## Summary + +The PDF renderer (`PdfBookRenderer.renderSongPage()`) currently renders all song sections on page 0 and leaves page 1 blank for 2-page songs. There is no bounds checking — the `y` coordinate can go below the bottom margin, causing content to render outside the visible page area. This plan adds proper `y`-position tracking against a minimum `yMin` boundary, splits content across pages at section boundaries when a song exceeds one page, and reserves space for bottom metadata/references on the last page. + +## AC Verification Checklist + +1. The renderer tracks `y` against the bottom margin during song page rendering +2. For 2-page songs, content splits across pages when it exceeds page 0's available space — remaining sections continue on page 1 +3. Content that would be rendered below the bottom margin (minus reserved footer space) is moved to the next page +4. If metadata is "bottom" position, sufficient space is reserved at the bottom of the last page +5. No text or images are rendered outside the printable page area +6. Existing tests continue to pass +7. New tests verify content splitting for songs exceeding one page + +## Implementation Steps + +### Step 1: Add a section height calculation helper in PdfBookRenderer + +Add a private method `calculateSectionHeight()` that computes how many PDF points a given `SongSection` will consume when rendered. This mirrors the measurement engine logic but uses the actual PDF `BaseFont` widths (not stubs). This is needed to decide whether a section fits on the current page. + +The method signature: +```kotlin +private fun calculateSectionHeight( + section: SongSection, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float +): Float +``` + +### Step 2: Add footer space reservation calculation + +Add a private method `calculateFooterReservation()` that computes how much vertical space must be reserved at the bottom of the **last** page of a song for: +- Bottom-position metadata (if `metadataPosition == "bottom"`) +- Notes +- Reference book footer + +```kotlin +private fun calculateFooterReservation( + song: Song, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float +): Float +``` + +### Step 3: Refactor renderSongPage() to split content across pages + +The key change: Instead of `val sections = if (pageIndex == 0) song.sections else emptyList()`, determine which sections belong on each page by: + +1. Calculate `yMin` = bottom margin in points (plus footer reservation for the last page) +2. For `pageIndex == 0`: Render sections in order. Before rendering each section, check if the section's height fits above `yMin`. If not, stop — remaining sections go to page 1. +3. For `pageIndex == 1`: Render the sections that didn't fit on page 0. The split point is stored via a `splitIndex` that is computed during page 0 rendering. + +**Approach:** Since `renderSongPage()` is called separately for page 0 and page 1, we need a way to know the split point on both calls. The cleanest approach is to compute the split index as a function: + +```kotlin +private fun computeSplitIndex( + song: Song, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float, + availableHeight: Float // total space on page 0 (contentTop - yMin) +): Int // index of first section that goes to page 1 +``` + +This method calculates the cumulative height of header + sections. When the cumulative height exceeds `availableHeight`, it returns the section index. If all sections fit, it returns `song.sections.size`. + +### Step 4: Update renderSongPage() to use bounds checking during rendering + +Even after determining the split, the actual rendering loop should still check `y >= yMin` as a safety net. If a section that was estimated to fit actually overflows (due to measurement inaccuracy), clamp rendering — do not render below `yMin`. + +### Step 5: Update footer rendering for multi-page songs + +Currently `isLastPage` is hardcoded to `pageIndex == 0`. Change it to correctly identify the last page: +- For 1-page songs: `pageIndex == 0` is the last page +- For 2-page songs: `pageIndex == 1` is the last page + +The song's `pageCount` isn't directly available in the renderer, but we can determine it: if `pageIndex == 1`, it's always the last page. If `pageIndex == 0`, it's the last page only if the song fits on one page (i.e., `computeSplitIndex == song.sections.size`). + +A simpler approach: pass the total page count as a parameter, or compute whether the song needs 2 pages inside `renderSongPage()`. + +**Decision:** Add a `totalPages: Int` parameter to `renderSongPage()`. The caller already knows this from the `PageContent.SongPage` list (consecutive song pages with pageIndex 0 and 1 for the same song). + +Actually, the simplest approach: The renderer sees `PageContent.SongPage(song, 0)` and `PageContent.SongPage(song, 1)` in the page list. We can pre-scan the pages list to know if a song has 2 pages. But even simpler: we can compute `computeSplitIndex` to know whether the song needs a second page. If `splitIndex < song.sections.size`, the song has 2 pages. + +### Step 6: Move notes and bottom-metadata to the last page + +Currently notes and bottom metadata only render on `pageIndex == 0`. Change this to render on the last page (which might be page 1 for 2-page songs). The logic: +- Compute `isLastPage` based on split index +- Render notes, bottom metadata, and reference footer only on the last page + +### Step 7: Write tests + +Add tests in `PdfBookRendererTest`: + +1. `render handles two-page song with content split across pages` — Create a song with many sections that exceed one page, render with pageIndex 0 and 1, verify PDF is valid. + +2. `render does not overflow below bottom margin` — Create a very long song, verify rendering completes without error. + +3. `render places metadata at bottom of last page for two-page songs` — Use `metadataPosition = "bottom"`, create a 2-page song, verify PDF is valid. + +4. `render handles notes on last page of two-page song` — Song with notes that spans 2 pages, verify rendering. + +### Step 8: Verify existing tests pass + +Run `gradle :renderer-pdf:test` and `gradle :app:test` to ensure no regressions. diff --git a/renderer-pdf/src/main/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRenderer.kt b/renderer-pdf/src/main/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRenderer.kt index 77c9b14..dc5d51a 100644 --- a/renderer-pdf/src/main/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRenderer.kt +++ b/renderer-pdf/src/main/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRenderer.kt @@ -63,7 +63,7 @@ class PdfBookRenderer : BookRenderer { renderSongPage( cb, chordLyricRenderer, fontMetrics, config, pageContent.song, pageContent.pageIndex, - contentTop, leftMargin, contentWidth + contentTop, leftMargin, contentWidth, marginBottom ) } is PageContent.FillerImage -> { @@ -91,6 +91,166 @@ class PdfBookRenderer : BookRenderer { document.close() } + /** + * Computes the index of the first section that should be rendered on page 1. + * All sections before this index render on page 0; sections from this index + * onward render on page 1. + * + * If all sections fit on page 0, returns song.sections.size (i.e., nothing on page 1). + */ + private fun computeSplitIndex( + song: Song, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float, + availableHeightOnPage0: Float + ): Int { + var consumed = 0f + + // Header: title + consumed += config.fonts.title.size * 1.5f + + // Top metadata + val renderMetaAtBottom = config.layout.metadataPosition == "bottom" + if (!renderMetaAtBottom) { + val metaParts = buildMetadataLines(song, config) + if (metaParts.isNotEmpty()) { + consumed += config.fonts.metadata.size * 1.8f * metaParts.size + } + } + + // Key/capo + if (song.key != null || song.capo != null) { + consumed += config.fonts.metadata.size * 1.8f + } + + consumed += 4f // gap before sections + + for ((index, section) in song.sections.withIndex()) { + val sectionHeight = calculateSectionHeight(section, fontMetrics, config, contentWidth) + if (consumed + sectionHeight > availableHeightOnPage0) { + // This section doesn't fit on page 0 + return index + } + consumed += sectionHeight + // Add verse spacing + consumed += config.layout.verseSpacing / 0.3528f + } + + return song.sections.size + } + + /** + * Calculates the height in PDF points that a section will consume when rendered. + */ + private fun calculateSectionHeight( + section: SongSection, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float + ): Float { + var height = 0f + val metaSize = config.fonts.metadata.size + + // Section label + if (section.label != null || section.type == SectionType.CHORUS) { + val labelText = section.label ?: when (section.type) { + SectionType.CHORUS -> "Refrain" + SectionType.REPEAT -> "Wiederholung" + else -> null + } + if (labelText != null) { + height += metaSize * 1.5f + } + } + + // Empty chorus (reference) + if (section.type == SectionType.CHORUS && section.lines.isEmpty()) { + height += metaSize * 1.8f + return height + } + + // Repeat start marker (contributes no extra height - drawn at current y) + // Lines + val chordSize = config.fonts.chords.size + val lyricSize = config.fonts.lyrics.size + val chordLineHeight = chordSize * 1.2f + val lyricLineHeight = lyricSize * 1.2f + val chordLyricGap = config.layout.chordLineSpacing / 0.3528f + + for (line in section.lines) { + if (line.imagePath != null) { + // Inline image: 40mm max height + gaps + val maxImageHeight = 40f / 0.3528f + height += maxImageHeight + 6f + } else { + val hasChords = line.segments.any { it.chord != null } + var lineHeight = lyricLineHeight + if (hasChords) { + lineHeight += chordLineHeight + chordLyricGap + } + height += lineHeight + 1f // 1pt gap between lines + } + } + + // Repeat end marker + if (section.type == SectionType.REPEAT) { + height += metaSize * 1.5f + } + + return height + } + + /** + * Calculates the space (in PDF points) that must be reserved at the bottom of + * the last page of a song for notes, bottom-position metadata, and reference footer. + */ + private fun calculateFooterReservation( + song: Song, + fontMetrics: PdfFontMetrics, + config: BookConfig, + contentWidth: Float + ): Float { + var reserved = 0f + val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) + val metaSize = config.fonts.metadata.size + + // Notes + if (song.notes.isNotEmpty()) { + reserved += 4f // gap before notes + val noteLineHeight = metaSize * 1.5f + for ((idx, note) in song.notes.withIndex()) { + val wrappedLines = wrapText(note, metaFont, metaSize, contentWidth) + reserved += noteLineHeight * wrappedLines.size + if (idx < song.notes.size - 1) { + reserved += noteLineHeight * 0.3f + } + } + } + + // Bottom metadata + if (config.layout.metadataPosition == "bottom") { + val metaParts = buildMetadataLines(song, config) + if (metaParts.isNotEmpty()) { + reserved += 4f + for (metaLine in metaParts) { + val wrappedLines = wrapText(metaLine, metaFont, metaSize, contentWidth) + reserved += metaSize * 1.5f * wrappedLines.size + } + } + } + + // Reference footer + if (config.referenceBooks.isNotEmpty() && song.references.isNotEmpty()) { + val lineHeight = metaSize * 1.4f + reserved += lineHeight * 2 // two rows (headers + numbers) + reserved += lineHeight * 1.5f // separator line gap + reserved += lineHeight * 0.5f // bottom gap + } + + return reserved + } + private fun renderSongPage( cb: PdfContentByte, chordLyricRenderer: ChordLyricRenderer, @@ -100,12 +260,28 @@ class PdfBookRenderer : BookRenderer { pageIndex: Int, // 0 for first page, 1 for second page of 2-page songs contentTop: Float, leftMargin: Float, - contentWidth: Float + contentWidth: Float, + bottomMargin: Float ) { var y = contentTop val renderMetaAtBottom = config.layout.metadataPosition == "bottom" + // Calculate the footer reservation for the last page + val footerReservation = calculateFooterReservation(song, fontMetrics, config, contentWidth) + + // Compute the split index to determine which sections go on which page. + // Page 0 gets sections 0.. 0) footerReservation else 0f) + val splitIndex = computeSplitIndex(song, fontMetrics, config, contentWidth, availableOnPage0) + val isTwoPageSong = splitIndex < song.sections.size + val isLastPage = if (isTwoPageSong) pageIndex == 1 else pageIndex == 0 + + // Bottom boundary for content on this page + val yMin = bottomMargin + (if (isLastPage) footerReservation else 0f) + if (pageIndex == 0) { // Render title val titleFont = fontMetrics.getBaseFont(config.fonts.title) @@ -125,6 +301,7 @@ class PdfBookRenderer : BookRenderer { val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size for (metaLine in metaParts) { + if (y - metaSize * 1.8f < yMin) break cb.beginText() cb.setFontAndSize(metaFont, metaSize) cb.setColorFill(Color.GRAY) @@ -143,24 +320,31 @@ class PdfBookRenderer : BookRenderer { if (infoParts.isNotEmpty()) { val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size - cb.beginText() - cb.setFontAndSize(metaFont, metaSize) - cb.setColorFill(Color.GRAY) - cb.setTextMatrix(leftMargin, y - metaSize) - cb.showText(infoParts.joinToString(" | ")) - cb.endText() - y -= metaSize * 1.8f + if (y - metaSize * 1.8f >= yMin) { + cb.beginText() + cb.setFontAndSize(metaFont, metaSize) + cb.setColorFill(Color.GRAY) + cb.setTextMatrix(leftMargin, y - metaSize) + cb.showText(infoParts.joinToString(" | ")) + cb.endText() + y -= metaSize * 1.8f + } } y -= 4f // gap before sections } // Determine which sections to render on this page - // For simplicity in this implementation, render all sections on pageIndex 0 - // A more sophisticated implementation would split sections across pages - val sections = if (pageIndex == 0) song.sections else emptyList() + val sections = if (pageIndex == 0) { + song.sections.subList(0, splitIndex) + } else { + song.sections.subList(splitIndex, song.sections.size) + } for (section in sections) { + // Safety check: stop rendering if we've gone below the boundary + if (y < yMin) break + // Section label if (section.label != null || section.type == SectionType.CHORUS) { val labelText = section.label ?: when (section.type) { @@ -171,6 +355,7 @@ class PdfBookRenderer : BookRenderer { if (labelText != null) { val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size + if (y - metaSize * 1.5f < yMin) break cb.beginText() cb.setFontAndSize(metaFont, metaSize) cb.setColorFill(Color.DARK_GRAY) @@ -185,6 +370,7 @@ class PdfBookRenderer : BookRenderer { if (section.type == SectionType.CHORUS && section.lines.isEmpty()) { val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size + if (y - metaSize * 1.8f < yMin) break cb.beginText() cb.setFontAndSize(metaFont, metaSize) cb.setColorFill(Color.DARK_GRAY) @@ -209,6 +395,7 @@ class PdfBookRenderer : BookRenderer { // Render lines for (line in section.lines) { + if (y < yMin) break val imgPath = line.imagePath if (imgPath != null) { // Render inline image @@ -223,21 +410,23 @@ class PdfBookRenderer : BookRenderer { if (section.type == SectionType.REPEAT) { val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size - cb.beginText() - cb.setFontAndSize(metaFont, metaSize) - cb.setColorFill(Color.DARK_GRAY) - cb.setTextMatrix(leftMargin, y - metaSize) - cb.showText(":\u2502") - cb.endText() - y -= metaSize * 1.5f + if (y - metaSize * 1.5f >= yMin) { + cb.beginText() + cb.setFontAndSize(metaFont, metaSize) + cb.setColorFill(Color.DARK_GRAY) + cb.setTextMatrix(leftMargin, y - metaSize) + cb.showText(":\u2502") + cb.endText() + y -= metaSize * 1.5f + } } // Verse spacing y -= config.layout.verseSpacing / 0.3528f } - // Render notes at the bottom (with word-wrap for multi-paragraph notes) - if (pageIndex == 0 && song.notes.isNotEmpty()) { + // Render notes on the last page + if (isLastPage && song.notes.isNotEmpty()) { y -= 4f val metaFont = fontMetrics.getBaseFont(config.fonts.metadata) val metaSize = config.fonts.metadata.size @@ -261,8 +450,8 @@ class PdfBookRenderer : BookRenderer { } } - // Render metadata at bottom of song page (if configured) - if (renderMetaAtBottom && pageIndex == 0) { + // Render metadata at bottom of song page (if configured) - on the last page only + if (renderMetaAtBottom && isLastPage) { val metaParts = buildMetadataLines(song, config) if (metaParts.isNotEmpty()) { y -= 4f @@ -284,15 +473,12 @@ class PdfBookRenderer : BookRenderer { } // Render reference book footer on the last page of the song - if (config.referenceBooks.isNotEmpty() && song.references.isNotEmpty()) { - val isLastPage = (pageIndex == 0) // For now, all content renders on page 0 - if (isLastPage) { - renderReferenceFooter( - cb, fontMetrics, config, song, - leftMargin, contentWidth, - config.layout.margins.bottom / 0.3528f - ) - } + if (config.referenceBooks.isNotEmpty() && song.references.isNotEmpty() && isLastPage) { + renderReferenceFooter( + cb, fontMetrics, config, song, + leftMargin, contentWidth, + bottomMargin + ) } } diff --git a/renderer-pdf/src/test/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRendererTest.kt b/renderer-pdf/src/test/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRendererTest.kt index 08709c8..626f6da 100644 --- a/renderer-pdf/src/test/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRendererTest.kt +++ b/renderer-pdf/src/test/kotlin/de/pfadfinder/songbook/renderer/pdf/PdfBookRendererTest.kt @@ -417,4 +417,213 @@ class PdfBookRendererTest { baos.size() shouldBeGreaterThan 0 } + + // --- Content splitting tests --- + + private fun createLongSong(title: String = "Long Song"): Song { + // Create a song with many sections that will exceed one A5 page + val sections = (1..20).map { i -> + SongSection( + type = SectionType.VERSE, + label = "Verse $i", + lines = (1..4).map { + SongLine( + listOf( + LineSegment(chord = "Am", text = "Some text with chords "), + LineSegment(chord = "G", text = "and more text here") + ) + ) + } + ) + } + return Song(title = title, sections = sections) + } + + @Test + fun `render splits content across pages for two-page song`() { + val song = createLongSong() + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, BookConfig(), baos) + + baos.size() shouldBeGreaterThan 0 + val bytes = baos.toByteArray() + val header = String(bytes.sliceArray(0..4)) + header shouldBe "%PDF-" + } + + @Test + fun `render does not overflow below bottom margin for very long song`() { + // Create an extremely long song + val sections = (1..40).map { i -> + SongSection( + type = SectionType.VERSE, + label = "Verse $i", + lines = (1..6).map { + SongLine( + listOf( + LineSegment(chord = "C", text = "A long line of text that should be rendered properly "), + LineSegment(chord = "G", text = "with chords above each segment") + ) + ) + } + ) + } + val song = Song(title = "Very Long Song", sections = sections) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, BookConfig(), baos) + + baos.size() shouldBeGreaterThan 0 + } + + @Test + fun `render places metadata at bottom of last page for two-page song`() { + val config = BookConfig( + layout = LayoutConfig(metadataPosition = "bottom") + ) + val song = createLongSong().copy( + composer = "Bach", + lyricist = "Goethe" + ) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, config, baos) + + baos.size() shouldBeGreaterThan 0 + } + + @Test + fun `render places notes on last page of two-page song`() { + val song = createLongSong().copy( + notes = listOf("This is a note that should appear on the last page") + ) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, BookConfig(), baos) + + baos.size() shouldBeGreaterThan 0 + } + + @Test + fun `render places reference footer on last page of two-page song`() { + val config = BookConfig( + referenceBooks = listOf( + ReferenceBook(id = "mo", name = "Mundorgel", abbreviation = "MO"), + ReferenceBook(id = "pl", name = "Pfadfinderlied", abbreviation = "PL") + ) + ) + val song = createLongSong().copy( + references = mapOf("mo" to 42, "pl" to 17) + ) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, config, baos) + + baos.size() shouldBeGreaterThan 0 + } + + @Test + fun `render handles short song that fits on one page without splitting`() { + // A simple short song should still work correctly after split logic is added + val song = Song( + title = "Short Song", + sections = listOf( + SongSection( + type = SectionType.VERSE, + lines = listOf( + SongLine(listOf(LineSegment(chord = "Am", text = "One line"))) + ) + ) + ) + ) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf(PageContent.SongPage(song, 0)), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, BookConfig(), baos) + + baos.size() shouldBeGreaterThan 0 + } + + @Test + fun `render two-page song with bottom metadata and references`() { + val config = BookConfig( + layout = LayoutConfig( + metadataPosition = "bottom", + metadataLabels = "german" + ), + referenceBooks = listOf( + ReferenceBook(id = "mo", name = "Mundorgel", abbreviation = "MO") + ) + ) + val song = createLongSong().copy( + composer = "Bach", + lyricist = "Goethe", + notes = listOf("Play softly", "Repeat last verse"), + references = mapOf("mo" to 55) + ) + + val layout = LayoutResult( + tocPages = 0, + pages = listOf( + PageContent.SongPage(song, 0), + PageContent.SongPage(song, 1) + ), + tocEntries = emptyList() + ) + + val baos = ByteArrayOutputStream() + renderer.render(layout, config, baos) + + baos.size() shouldBeGreaterThan 0 + } }