20260121 issue fixing#292
Merged
Merged
Conversation
Implemented #197 setFixedHeight(true) will set a fixed height for a row, and will shrink the (text) content to fit the height. See unit test FixedHeightRowTest
…fter drawing it, including page and Y implement #213
… inner height fix #211 and added unit test CellSetTextSetHeightTest.java
Unit tests in RowspanInnerTableTest.java
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple issues (referenced as "20260121 issue fixing") by implementing several new features and fixes for the Boxable PDF library:
Changes:
- Adds support for fixed-height rows with automatic text shrinking
- Implements configurable inner table rendering with border and padding controls
- Adds
drawAndGetPosition()method for chaining consecutive tables across pages - Includes HTML sanitization for inner tables to prevent malformed input
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| RowspanInnerTableTest.java | New test demonstrating inner table functionality with customizable borders and alignment |
| FixedHeightRowTest.java | New test verifying fixed-height row behavior with auto-shrinking text |
| ConsecutiveTablesMultiPageOverlapTest.java | New test confirming consecutive tables render correctly after multi-page tables |
| CellSetTextSetHeightTest.java | New test ensuring setText() and setHeight() work together correctly |
| TableCell.java | Adds inner table configuration options, HTML sanitization, and border control methods |
| Table.java | Adds drawAndGetPosition() method, fixed-height support, and comprehensive javadoc |
| Row.java | Implements fixed-height functionality and adds javadoc |
| Cell.java | Adds fitFontSizeToHeight() for auto-shrinking text and warning logging for negative heights |
| pom.xml | Adds slf4j-simple test dependency for logging |
| build.gradle | Adds slf4j-simple test dependency for logging |
| README.md | Documents new fixed-height row feature with example |
| .github/copilot-instructions.md | New file with instructions for issue resolution workflow |
Comment on lines
+145
to
+161
| /** | ||
| * <p> | ||
| * Creates a table that starts on a new page provided by the {@link PageProvider}. | ||
| * </p> | ||
| * | ||
| * @param yStartNewPage Y position where new pages will start | ||
| * @param pageTopMargin top margin for new pages | ||
| * @param pageBottomMargin bottom margin for new pages | ||
| * @param width {@link Table} width in points | ||
| * @param margin left/right margin in points | ||
| * @param document {@link PDDocument} for drawing | ||
| * @param drawLines {@code true} to draw borders | ||
| * @param drawContent {@code true} to draw text and images | ||
| * @param pageProvider page provider for new pages | ||
| * @throws IOException if fonts cannot be loaded | ||
| */ | ||
| public Table(float yStartNewPage, float pageTopMargin, float pageBottomMargin, float width, float margin, |
There was a problem hiding this comment.
Inconsistent indentation. The method annotation and declaration are indented more than other methods in the class.
Suggested change
| /** | |
| * <p> | |
| * Creates a table that starts on a new page provided by the {@link PageProvider}. | |
| * </p> | |
| * | |
| * @param yStartNewPage Y position where new pages will start | |
| * @param pageTopMargin top margin for new pages | |
| * @param pageBottomMargin bottom margin for new pages | |
| * @param width {@link Table} width in points | |
| * @param margin left/right margin in points | |
| * @param document {@link PDDocument} for drawing | |
| * @param drawLines {@code true} to draw borders | |
| * @param drawContent {@code true} to draw text and images | |
| * @param pageProvider page provider for new pages | |
| * @throws IOException if fonts cannot be loaded | |
| */ | |
| public Table(float yStartNewPage, float pageTopMargin, float pageBottomMargin, float width, float margin, | |
| /** | |
| * <p> | |
| * Creates a table that starts on a new page provided by the {@link PageProvider}. | |
| * </p> | |
| * | |
| * @param yStartNewPage Y position where new pages will start | |
| * @param pageTopMargin top margin for new pages | |
| * @param pageBottomMargin bottom margin for new pages | |
| * @param width {@link Table} width in points | |
| * @param margin left/right margin in points | |
| * @param document {@link PDDocument} for drawing | |
| * @param drawLines {@code true} to draw borders | |
| * @param drawContent {@code true} to draw text and images | |
| * @param pageProvider page provider for new pages | |
| * @throws IOException if fonts cannot be loaded | |
| */ | |
| public Table(float yStartNewPage, float pageTopMargin, float pageBottomMargin, float width, float margin, |
Comment on lines
+102
to
+140
| @Test | ||
| public void negativeInnerHeightLogsWarning() { | ||
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | ||
|
|
||
| PrintStream originalErr = System.err; | ||
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | ||
| System.setErr(new PrintStream(errStream)); | ||
|
|
||
| try { | ||
| PDDocument doc = new PDDocument(); | ||
| PDPage page = new PDPage(PDRectangle.A4); | ||
| doc.addPage(page); | ||
|
|
||
| float margin = 50f; | ||
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | ||
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | ||
| float yStart = yStartNewPage; | ||
| float bottomMargin = 70f; | ||
|
|
||
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | ||
| true); | ||
|
|
||
| Row<PDPage> row = table.createRow(10f); | ||
| row.setFixedHeight(true); | ||
| Cell<PDPage> cell = row.createCell(100, "Text"); | ||
|
|
||
| float innerHeight = cell.getInnerHeight(); | ||
| assertTrue(innerHeight < 0); | ||
|
|
||
| doc.close(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } finally { | ||
| System.setErr(originalErr); | ||
| } | ||
|
|
||
| String logs = errStream.toString(); | ||
| assertTrue(logs.contains("Cell inner height is negative")); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation. The method body is indented more than it should be. The indentation should be consistent with other methods.
Suggested change
| @Test | |
| public void negativeInnerHeightLogsWarning() { | |
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | |
| PrintStream originalErr = System.err; | |
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | |
| System.setErr(new PrintStream(errStream)); | |
| try { | |
| PDDocument doc = new PDDocument(); | |
| PDPage page = new PDPage(PDRectangle.A4); | |
| doc.addPage(page); | |
| float margin = 50f; | |
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | |
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | |
| float yStart = yStartNewPage; | |
| float bottomMargin = 70f; | |
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | |
| true); | |
| Row<PDPage> row = table.createRow(10f); | |
| row.setFixedHeight(true); | |
| Cell<PDPage> cell = row.createCell(100, "Text"); | |
| float innerHeight = cell.getInnerHeight(); | |
| assertTrue(innerHeight < 0); | |
| doc.close(); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } finally { | |
| System.setErr(originalErr); | |
| } | |
| String logs = errStream.toString(); | |
| assertTrue(logs.contains("Cell inner height is negative")); | |
| } | |
| @Test | |
| public void negativeInnerHeightLogsWarning() { | |
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | |
| PrintStream originalErr = System.err; | |
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | |
| System.setErr(new PrintStream(errStream)); | |
| try { | |
| PDDocument doc = new PDDocument(); | |
| PDPage page = new PDPage(PDRectangle.A4); | |
| doc.addPage(page); | |
| float margin = 50f; | |
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | |
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | |
| float yStart = yStartNewPage; | |
| float bottomMargin = 70f; | |
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | |
| true); | |
| Row<PDPage> row = table.createRow(10f); | |
| row.setFixedHeight(true); | |
| Cell<PDPage> cell = row.createCell(100, "Text"); | |
| float innerHeight = cell.getInnerHeight(); | |
| assertTrue(innerHeight < 0); | |
| doc.close(); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } finally { | |
| System.setErr(originalErr); | |
| } | |
| String logs = errStream.toString(); | |
| assertTrue(logs.contains("Cell inner height is negative")); | |
| } |
Comment on lines
+212
to
+237
| int colIndex = 0; | ||
| int colPosition = 0; | ||
| for (Element col : tableHasHeaderColumns ? tableHeaderCols : tableCols) { | ||
| String cellHtml = Parser.unescapeEntities(col.html(), true); | ||
| int colSpan = 1; | ||
| if (col.attr("colspan") != null && !col.attr("colspan").isEmpty()) { | ||
| try { | ||
| colSpan = Integer.parseInt(col.attr("colspan")); | ||
| } catch (NumberFormatException e) { | ||
| colSpan = 1; | ||
| } | ||
| } | ||
| int startCol = colPosition; | ||
| int endCol = Math.min(columnsSize - 1, colPosition + colSpan - 1); | ||
| if (col.attr("colspan") != null && !col.attr("colspan").isEmpty()) { | ||
| Cell<T> cell = (Cell<T>) row.createCell( | ||
| tableWidth / columnsSize * Integer.parseInt(col.attr("colspan")) / row.getWidth() * 100, | ||
| cellHtml); | ||
| applyInnerTableBorderOptions(cell, rowIndex, startCol, endCol, rows.size(), columnsSize); | ||
| } else { | ||
| Cell<T> cell = (Cell<T>) row.createCell(tableWidth / columnsSize / row.getWidth() * 100, | ||
| cellHtml); | ||
| applyInnerTableBorderOptions(cell, rowIndex, startCol, endCol, rows.size(), columnsSize); | ||
| } | ||
| colIndex++; | ||
| colPosition += colSpan; |
There was a problem hiding this comment.
Unused variable colIndex. The variable is incremented at line 236 but never used in the loop. If it's not needed, it should be removed.
Comment on lines
+670
to
+672
| if (document.select("table").isEmpty()) { | ||
| String textOnly = Jsoup.clean(input, Safelist.none()); | ||
| return "<table><tr><td>" + textOnly + "</td></tr></table>"; |
There was a problem hiding this comment.
The sanitization may convert non-table HTML into a table unexpectedly. When the input contains no table tags, the text is wrapped in a table structure (line 672). This could be surprising behavior for API consumers who might expect plain text to remain as plain text or to receive an error. Consider documenting this behavior more clearly in the method's javadoc or the constructor.
Comment on lines
+190
to
252
| /** | ||
| * <p> | ||
| * Draws a title above the table using default color (black) and no wrapping. | ||
| * </p> | ||
| * | ||
| * @param title title text, or {@code null} to reserve height only | ||
| * @param font font used for the title | ||
| * @param fontSize font size in points | ||
| * @param tableWidth available width for the title | ||
| * @param height reserved title height | ||
| * @param alignment horizontal alignment string | ||
| * @param freeSpaceForPageBreak minimum free space required before drawing | ||
| * @param drawHeaderMargin whether to draw header margin below title | ||
| * @throws IOException if drawing fails | ||
| */ | ||
| public void drawTitle(String title, PDFont font, int fontSize, float tableWidth, float height, String alignment, | ||
| float freeSpaceForPageBreak, boolean drawHeaderMargin) throws IOException { | ||
| drawTitle(title, font, fontSize, tableWidth, height, alignment, freeSpaceForPageBreak, null, drawHeaderMargin, | ||
| Color.BLACK, null); | ||
| } | ||
|
|
||
| /** | ||
| * <p> | ||
| * Draws a title above the table using a wrapping function and default color (black). | ||
| * </p> | ||
| * | ||
| * @param title title text, or {@code null} to reserve height only | ||
| * @param font font used for the title | ||
| * @param fontSize font size in points | ||
| * @param tableWidth available width for the title | ||
| * @param height reserved title height | ||
| * @param alignment horizontal alignment string | ||
| * @param freeSpaceForPageBreak minimum free space required before drawing | ||
| * @param wrappingFunction wrapping function for line breaks | ||
| * @param drawHeaderMargin whether to draw header margin below title | ||
| * @throws IOException if drawing fails | ||
| */ | ||
| public void drawTitle(String title, PDFont font, int fontSize, float tableWidth, float height, String alignment, | ||
| float freeSpaceForPageBreak, WrappingFunction wrappingFunction, boolean drawHeaderMargin) | ||
| throws IOException { | ||
| drawTitle(title, font, fontSize, tableWidth, height, alignment, freeSpaceForPageBreak, wrappingFunction, | ||
| drawHeaderMargin, Color.BLACK, null); | ||
| } | ||
|
|
||
| /** | ||
| * <p> | ||
| * Draws a title above the table with explicit color and text type styling. | ||
| * </p> | ||
| * | ||
| * @param title title text, or {@code null} to reserve height only | ||
| * @param font font used for the title | ||
| * @param fontSize font size in points | ||
| * @param tableWidth available width for the title | ||
| * @param height reserved title height | ||
| * @param alignment horizontal alignment string | ||
| * @param freeSpaceForPageBreak minimum free space required before drawing | ||
| * @param wrappingFunction wrapping function for line breaks | ||
| * @param drawHeaderMargin whether to draw header margin below title | ||
| * @param color title text color | ||
| * @param textType optional text decoration | ||
| * @throws IOException if drawing fails | ||
| */ | ||
| public void drawTitle(String title, PDFont font, int fontSize, float tableWidth, float height, String alignment, |
There was a problem hiding this comment.
Inconsistent indentation. The method annotations and declarations are indented more than other methods in the class.
| assertTrue(file.exists()); | ||
| assertTrue(file.length() > 0); | ||
| assertTrue(text.contains("Expected behavior: the inner table uses default spacing")); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation. The closing brace is indented more than it should be. It should align with the method's opening.
| assertTrue(file.exists()); | ||
| assertTrue(file.length() > 0); | ||
|
|
||
| assertTrue(text.contains("If the test is successfull, you should see this text here")); |
There was a problem hiding this comment.
Spelling error in the test assertion. "successfull" should be "successful" (one 'l').
Comment on lines
+102
to
+140
| @Test | ||
| public void negativeInnerHeightLogsWarning() { | ||
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | ||
|
|
||
| PrintStream originalErr = System.err; | ||
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | ||
| System.setErr(new PrintStream(errStream)); | ||
|
|
||
| try { | ||
| PDDocument doc = new PDDocument(); | ||
| PDPage page = new PDPage(PDRectangle.A4); | ||
| doc.addPage(page); | ||
|
|
||
| float margin = 50f; | ||
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | ||
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | ||
| float yStart = yStartNewPage; | ||
| float bottomMargin = 70f; | ||
|
|
||
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | ||
| true); | ||
|
|
||
| Row<PDPage> row = table.createRow(10f); | ||
| row.setFixedHeight(true); | ||
| Cell<PDPage> cell = row.createCell(100, "Text"); | ||
|
|
||
| float innerHeight = cell.getInnerHeight(); | ||
| assertTrue(innerHeight < 0); | ||
|
|
||
| doc.close(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } finally { | ||
| System.setErr(originalErr); | ||
| } | ||
|
|
||
| String logs = errStream.toString(); | ||
| assertTrue(logs.contains("Cell inner height is negative")); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation. The method annotation and declaration are indented more than other methods in the file. They should align with line 18.
Suggested change
| @Test | |
| public void negativeInnerHeightLogsWarning() { | |
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | |
| PrintStream originalErr = System.err; | |
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | |
| System.setErr(new PrintStream(errStream)); | |
| try { | |
| PDDocument doc = new PDDocument(); | |
| PDPage page = new PDPage(PDRectangle.A4); | |
| doc.addPage(page); | |
| float margin = 50f; | |
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | |
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | |
| float yStart = yStartNewPage; | |
| float bottomMargin = 70f; | |
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | |
| true); | |
| Row<PDPage> row = table.createRow(10f); | |
| row.setFixedHeight(true); | |
| Cell<PDPage> cell = row.createCell(100, "Text"); | |
| float innerHeight = cell.getInnerHeight(); | |
| assertTrue(innerHeight < 0); | |
| doc.close(); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } finally { | |
| System.setErr(originalErr); | |
| } | |
| String logs = errStream.toString(); | |
| assertTrue(logs.contains("Cell inner height is negative")); | |
| } | |
| @Test | |
| public void negativeInnerHeightLogsWarning() { | |
| System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "warn"); | |
| PrintStream originalErr = System.err; | |
| ByteArrayOutputStream errStream = new ByteArrayOutputStream(); | |
| System.setErr(new PrintStream(errStream)); | |
| try { | |
| PDDocument doc = new PDDocument(); | |
| PDPage page = new PDPage(PDRectangle.A4); | |
| doc.addPage(page); | |
| float margin = 50f; | |
| float yStartNewPage = page.getMediaBox().getHeight() - (2 * margin); | |
| float tableWidth = page.getMediaBox().getWidth() - (2 * margin); | |
| float yStart = yStartNewPage; | |
| float bottomMargin = 70f; | |
| BaseTable table = new BaseTable(yStart, yStartNewPage, bottomMargin, tableWidth, margin, doc, page, true, | |
| true); | |
| Row<PDPage> row = table.createRow(10f); | |
| row.setFixedHeight(true); | |
| Cell<PDPage> cell = row.createCell(100, "Text"); | |
| float innerHeight = cell.getInnerHeight(); | |
| assertTrue(innerHeight < 0); | |
| doc.close(); | |
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } finally { | |
| System.setErr(originalErr); | |
| } | |
| String logs = errStream.toString(); | |
| assertTrue(logs.contains("Cell inner height is negative")); | |
| } |
Comment on lines
+167
to
+168
| } finally { | ||
| yStart = originalYStart; |
There was a problem hiding this comment.
The yStart restoration in the finally block may cause issues if an exception occurs before originalYStart is assigned. Consider initializing originalYStart at the beginning of the method, outside the try block, to ensure it always has a valid value.
Comment on lines
+18
to
+20
| import org.jsoup.safety.Safelist; | ||
|
|
||
| import be.quodlibet.boxable.line.LineStyle; |
There was a problem hiding this comment.
Missing import organization. The import for LineStyle at line 20 is placed after imports from org.jsoup and before org.slf4j, breaking the conventional ordering (standard library, then third-party, then project-local). It should be placed with other be.quodlibet imports or in a more logical position.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.