fix: correct SheetWriteHandler.afterSheetDispose to avoid repeated execution

[bengbengbalabalabeng]

Purpose of the pull request

Related: #799

What's changed?

Adjust the trigger timing of afterSheetDispose: move the callback from ExcelBuilderImpl#addContent() / ExcelBuilderImpl#fill() to ExcelBuilderImpl#finish() WriteContextImpl#finish() to avoid repeated invocations in multi-write / multi-fill scenarios.

Coverage

• write, table write, fill
• single/multi sheet handler

Checklist

• I have read the Contributor Guide.
• I have written the necessary doc or comment.
• I have added the necessary unit tests and all cases have passed.

[skytin1004]

Hi @bengbengbalabalabeng, I think it would be good to add a multi-sheet test case too. What do you think?

My understanding is that the original issue is caused by afterSheetDispose being called after every writer.write(...). So when two tables are written to the same sheet, the callback runs twice for that sheet.

This PR moves the callback to finish(), which seems to fix the same-sheet case. But I think it may miss the multi-sheet case.

For example:

1. writer.write(data, sheet1)
2. writer.write(data, sheet2)
3. writer.finish()

After writing sheet2, the current sheet is sheet2. So when finish() calls afterSheetDispose, only sheet2 gets the callback.

I tried this locally, and the handler for sheet1 had afterSheetDispose count 0.

Could you also check this case please? I think afterSheetDispose should run once for each written sheet, not only for the last current sheet.

[bengbengbalabalabeng]

Hi @bengbengbalabalabeng, I think it would be good to add a multi-sheet test case too. What do you think?

My understanding is that the original issue is caused by afterSheetDispose being called after every writer.write(...). So when two tables are written to the same sheet, the callback runs twice for that sheet.

This PR moves the callback to finish(), which seems to fix the same-sheet case. But I think it may miss the multi-sheet case.

For example:

1. writer.write(data, sheet1)
2. writer.write(data, sheet2)
3. writer.finish()

After writing sheet2, the current sheet is sheet2. So when finish() calls afterSheetDispose, only sheet2 gets the callback.

I tried this locally, and the handler for sheet1 had afterSheetDispose count 0.

Could you also check this case please? I think afterSheetDispose should run once for each written sheet, not only for the last current sheet.

Thank for the suggestion. I will follow up as soon as possible :)

[Copilot]

Pull request overview

This PR addresses issue #799 by changing when SheetWriteHandler.afterSheetDispose is triggered so it runs once per sheet after writing/filling is complete, instead of being invoked after each write(...) / fill(...) call (which caused repeated execution in multi-write/multi-table scenarios).

Changes:

• Remove per-operation afterSheetDispose invocation from ExcelBuilderImpl#addContent() and ExcelBuilderImpl#fill().
• Trigger afterSheetDispose from WriteContextImpl#finish(...), iterating over initialized sheets to invoke the callback once per sheet.
• Add/extend integration tests to cover multi-write, multi-table, multi-sheet, and fill scenarios, plus a new CountingWriteHandler test utility.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/ExcelBuilderImpl.java	Stops invoking afterSheetDispose after each add/fill operation.
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java	Invokes afterSheetDispose during finish(...) for initialized sheets.
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/WriteHandlerUtils.java	Refactors afterSheetDispose to accept a SheetWriteHandlerContext directly.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/WriteHandlerUtilsTest.java	Updates unit tests to match the new afterSheetDispose signature.
fesod-sheet/src/test/java/org/apache/fesod/sheet/handler/WriteHandlerTest.java	Adds coverage for multi-write/multi-table/multi-sheet write and fill behaviors.
fesod-sheet/src/test/java/org/apache/fesod/sheet/handler/CountingWriteHandler.java	Adds a counting handler used to assert hook invocation counts in new tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bengbengbalabalabeng]

When there is only one sheet, I don't think we need to manually switch the holder, just use the default.

[delei]

LGTM.

FYI, The issue has been confirmed as resolved, and I have successfully tested both the CSV and multiple WriteTable write scenarios in my local environment.