Skip to content

refactor: delegate format-specific logic to per-format exporters#204

Merged
paodb merged 1 commit into
masterfrom
refactor-171
Jul 22, 2026
Merged

refactor: delegate format-specific logic to per-format exporters#204
paodb merged 1 commit into
masterfrom
refactor-171

Conversation

@javier-godoy

@javier-godoy javier-godoy commented Jul 22, 2026

Copy link
Copy Markdown
Member

Close #171

Internal refactor (public API is unchanged).

  1. Introduced a package-private per-format exporter hierarchy (nested classes so they reach GridExporter's private members without any visibility widening):
  • FormatExporter — base: holds enabled, tooltipText, tooltipConfigurator; declares createIcon(), getFileExtension(), getContentType(), createWriter(template), getDownloadHandler(template), getStreamResource(template).
  • ConcurrentFormatExporter — adds the concurrency-wrapped download handler / stream resource and the .forComponent(button) footer wiring, shared by Excel/DOCX/PDF.
  • ExcelFormatExporter, DocxFormatExporter, PdfFormatExporter, CsvFormatExporter — one per format.
  1. GridExporter now delegates to these instances:
  • is/setXxxExportEnabled → xxx.enabled
  • setXxxExportTooltipText / setXxxExportTooltipConfigurator → xxx.tooltipText / xxx.tooltipConfigurator
  • getXxxDownloadHandler(...) → xxx.getDownloadHandler(...)
  • deprecated getXxxStreamResource(...) → xxx.getStreamResource(...)
  1. Footer initialized from the enabled exporters — the four hardcoded if blocks in createFor's attach listener became a single loop over getFormatExporters() ([excel, docx, pdf, csv]), calling the new addExportButton(...) for each enabled one.

Behaviour I deliberately preserved

  • CSV stays outside the concurrency control (plain StreamResourceWriterAdapter / StreamResource, no .forComponent), unlike the other three.
  • PDF's auto-attached button still reuses the DOCX template (the existing quirk at old line 194) — encapsulated in PdfFormatExporter.getConfiguredTemplate() with a comment.
  • Button order (Excel → DOCX → PDF → CSV), the no-arg download-handler getters passing null, and all filenames/content-types are identical to before.

The excelCustomTemplate / docxCustomTemplate values from createFor(grid, excel, docx) are now stored as fields (they were previously captured only in the listener closure) so the format exporters can supply them for the footer buttons.

Summary by CodeRabbit

  • Refactor

    • Centralized export behavior across Excel, DOCX, PDF, and CSV formats.
    • Improved consistency for format-specific settings, tooltips, file types, and download handling.
  • Bug Fixes

    • Export buttons now appear only for enabled formats.
    • Existing enable/disable controls and tooltip settings continue to apply correctly across all supported formats.

@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

GridExporter now centralizes Excel, DOCX, PDF, and CSV behavior in per-format exporters. Footer buttons, tooltip configuration, stream resources, download handlers, and enablement accessors delegate through these exporters.

Changes

Format exporter refactor

Layer / File(s) Summary
Exporter implementations
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
Adds shared, concurrent, and CSV-specific exporter implementations for format metadata, writers, handlers, and stream resources.
State and footer wiring
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
Stores ordered format exporters, propagates templates and tooltip settings, and creates buttons for enabled formats.
Public API delegation
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
Delegates stream-resource getters, download-handler getters, and enablement accessors to the corresponding exporter.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The refactor delegates format-specific logic and initializes the footer from enabled exporters as requested in #171.
Out of Scope Changes check ✅ Passed No unrelated changes are evident; the edits stay within the exporter delegation and footer-initialization refactor.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main refactor: moving format-specific logic into per-format exporters.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor-171

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)

1144-1190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused button parameter flagged by static analysis is intentional (template method), but worth suppressing explicitly.

createFooterDownloadHandler(Component button)'s default implementation in FormatExporter doesn't use button — it's only consumed by the ConcurrentFormatExporter override via forComponent(button). This is a legitimate template-method pattern (CSV correctly doesn't need it), so the Sonar warning is a false positive rather than a real defect. Consider a @SuppressWarnings annotation or a brief comment to make the intent explicit and silence the recurring finding.

Suggested annotation
+    `@SuppressWarnings`("java:S1172") // button is used by ConcurrentFormatExporter overrides
     DownloadHandler createFooterDownloadHandler(Component button) {
       return getDownloadHandler(getConfiguredTemplate());
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java`
around lines 1144 - 1190, Add an explicit suppression or concise explanatory
comment to FormatExporter.createFooterDownloadHandler(Component button)
documenting that the unused parameter is intentional for the template-method
contract and is consumed by ConcurrentFormatExporter overrides. Keep the default
implementation behavior unchanged.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java`:
- Around line 1144-1190: Add an explicit suppression or concise explanatory
comment to FormatExporter.createFooterDownloadHandler(Component button)
documenting that the unused parameter is intentional for the template-method
contract and is consumed by ConcurrentFormatExporter overrides. Keep the default
implementation behavior unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 20892c9e-75cd-46b5-8e76-382257e198e5

📥 Commits

Reviewing files that changed from the base of the PR and between 7cca9a6 and 2f1be27.

📒 Files selected for processing (1)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java

@javier-godoy
javier-godoy requested review from paodb and scardanzan July 22, 2026 14:55
@javier-godoy
javier-godoy marked this pull request as ready for review July 22, 2026 14:55
@javier-godoy javier-godoy moved this from To Do to In Progress in Flowing Code Addons Jul 22, 2026
@paodb
paodb merged commit 3acf8b9 into master Jul 22, 2026
4 checks passed
@paodb
paodb deleted the refactor-171 branch July 22, 2026 15:27
@github-project-automation github-project-automation Bot moved this from In Progress to Pending release in Flowing Code Addons Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending release

Development

Successfully merging this pull request may close these issues.

Refactor Exporters

2 participants