fix: preserve joined headers #178
Conversation
|
""" WalkthroughThe changes refactor header and footer cell text extraction in the Vaadin Grid exporter. A generic method is replaced with dedicated methods for headers and footers, introducing improved logic for handling merged cells, caching, and error handling. The test demo is updated to use explicit column variables and a new joined header row. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java (2)
176-176: Minor formatting inconsistency.Missing space around the equality operator.
- return footer==null?"":footer; + return footer == null ? "" : footer;
155-177: Footer extraction method is well-structured with minor formatting issue.The footer text extraction logic is sound and follows a similar pattern to the header extraction. However, there's a minor formatting issue on line 176.
Apply this diff to improve code formatting:
- return footer==null?"":footer; + return footer == null ? "" : footer;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java(3 hunks)src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterMultipleHeaderRowsDemo.java(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterMultipleHeaderRowsDemo.java
🔇 Additional comments (27)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java (15)
27-27: Import addition looks good.The import for
HeaderRow.HeaderCellis necessary for the new header cell handling logic.
107-107: Method call updated to use specialized header rendering.The call to
renderHeaderCellTextContentreplaces the previous generic approach, providing better control over header text extraction.
118-118: Method call updated to use specialized footer rendering.The call to
renderFooterCellTextContentprovides dedicated logic for footer text extraction.
124-133: Helper method provides safe text extraction.The
obtainCellFunctionmethod safely extracts text from header cells, first tryinggetText()and falling back togetTextRecursively()from the component if available.
135-153: Excellent logic for handling merged header cells.The key improvement is in lines 141-142 where the method checks if the current column is the first column OR if the header cell for the previous column differs from the current header cell. This prevents duplicate text extraction from merged cells, which directly addresses the PR objective to "preserve joined headers."
The error handling with try-catch is also appropriate for robustness.
155-177: Footer rendering method provides dedicated logic.The method handles footer text extraction with proper fallback from footer text to footer component text, maintaining consistency with the header approach.
27-27: Import addition looks good.The new import for
HeaderRow.HeaderCellis necessary for the enhanced header cell handling logic.
107-107: Method call updates align with the refactoring.The calls to the new specialized methods
renderHeaderCellTextContentandrenderFooterCellTextContentproperly replace the previous generic approach.Also applies to: 118-118
124-133: Helper method provides clean text extraction logic.The
obtainCellFunctionmethod properly handles both text-based and component-based header cells with appropriate fallback logic.
135-153: Excellent implementation of joined header preservation logic.The core fix for preserving joined headers is well-implemented. The logic at lines 141-142 correctly prevents duplicate text extraction for merged header cells by checking if the current header cell differs from the previous column's header cell. This ensures that joined headers are only processed once and their text is preserved correctly.
The error handling with descriptive exception messages is also a good practice.
27-27: LGTM: Import addition supports new functionality.The new import for
HeaderRow.HeaderCellis necessary for the enhanced header cell handling logic.
107-107: LGTM: Refactoring to specialized methods improves maintainability.The replacement of generic method calls with dedicated
renderHeaderCellTextContentandrenderFooterCellTextContentmethods provides better separation of concerns and allows for specialized handling of merged cells.Also applies to: 118-118
124-133: LGTM: Helper method with good error handling.The
obtainCellFunctionmethod provides a clean abstraction for extracting text from header cells with appropriate fallback logic (text → component text recursively).
135-153: Excellent logic for handling merged headers.The key improvement is the merged cell detection logic:
if (columnIndex == 0 || headerRow.getCell(grid.getColumns().get(columnIndex - 1)) != headerCell) {This correctly prevents duplicate text extraction from merged/joined header cells by only processing when the current cell differs from the previous column's cell. This directly addresses the PR objective of preserving joined headers.
155-177: LGTM: Consistent footer handling with proper error management.The footer text extraction method maintains consistency with the header approach and includes appropriate error handling. The logic correctly handles both text and component-based footers.
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterMultipleHeaderRowsDemo.java (12)
60-65: Good refactoring to use explicit column variables.Assigning columns to named variables (
firstNameColumn,lastNameColumn,bigColumn) improves code readability and enables the subsequent joined header functionality.
82-82: Simplified method call is cleaner.Removing the explicit
thisqualifier improves code conciseness without affecting functionality.
84-86: Perfect demonstration of joined headers functionality.The addition of a joined header row that merges columns under grouped labels ("Full name" and "Big column and budget") provides an excellent test case for the new header cell handling logic in
BaseStreamResourceWriter.java.
91-91: Correct index adjustment for the new header structure.Changing from
get(0)toget(1)correctly accounts for the prepended joined header row. SinceprependHeaderRow()adds the joined header at index 0, the original headers are now at index 1.
60-65: Good refactoring to named column variables.Converting anonymous column additions to named variables improves code readability and enables referencing these columns later for creating joined headers.
82-82: Minor code cleanup looks good.Removing the explicit
thisqualifier simplifies the code without changing functionality.
84-86: Excellent test scenario for joined headers.The addition of a joined header row that merges columns provides a perfect test case for validating the joined header preservation functionality implemented in the main code. This directly tests the scenario that issue #176 was addressing.
91-91: Correct adjustment for the new header row structure.Changing from
get(0)toget(1)is necessary because the new joined header row was prepended, making the original header row shift to index 1. This ensures the test continues to reference the correct header row for adding extra headers.
60-65: Good refactoring: Explicit column variables improve testability.Storing column references in named variables (
firstNameColumn,lastNameColumn,bigColumn) makes the code more readable and enables the subsequent joined header operations.
82-82: Minor improvement: Simplified method call.Removing the explicit
thisqualifier is a minor style improvement that maintains readability.
84-86: Perfect test scenario for joined headers.The addition of a joined header row that merges columns provides an excellent test case for validating the fix in
BaseStreamResourceWriter.java. This directly supports the PR objective of preserving joined headers.
91-91: Correct index adjustment for prepended header row.The change from
get(0)toget(1)is necessary because the new joined header row was prepended, shifting the original header row to index 1. This ensures the loop correctly references the column-specific headers rather than the joined headers.
|
Close #176
d8c12e2 - refactor: remove unused import
3a7adfe - refactor: duplicate method
907759a - refactor: remove flag argument
5c42daf - refactor: remove dead code
106e3c9 - refactor: remove unused parameter
4de4652 - refactor: remove dead code
a23167f - refactor: rename variables
b007e95 - refactor: move lambda to private method
Summary by CodeRabbit
Refactor
Tests