pjfanning opened a new issue, #1409:
URL: https://github.com/apache/poi/issues/1409
## Problem
`FormatTrackingHSSFListener.getFormatString(int formatIndex)` ignores FORMAT
records that the file defines for built-in format indexes:
```java
if (formatIndex >= HSSFDataFormat.getNumberOfBuiltinBuiltinFormats()) {
FormatRecord tfr = _customFormatRecords.get(formatIndex);
...
} else {
format = HSSFDataFormat.getBuiltinFormat((short) formatIndex);
}
```
The listener already collects every `FormatRecord` into
`_customFormatRecords`, including those with index codes below 50, but for
those indexes it never looks them up and returns POI's hard-coded built-in
string instead.
Excel does write FORMAT records for some built-in indexes, and their content
depends on the locale. A `.xls` saved by a Chinese, German, etc. Excel defines
indexes 5–8, 42 and 44 (the currency/accounting formats) with that locale's
currency symbol. Through the event API these files come back with POI's `$`
strings.
The usermodel API behaves differently. `HSSFDataFormat.getFormat(short)`
returns the file's string when a built-in index has been overridden ("It's been
overridden, use that value"). So the same cell gets a different format string
depending on whether it is read with `HSSFWorkbook` or with the event API.
Downstream impact: Apache Fesod (incubating) is adding its own
`FormatTrackingHSSFListener` subclass to get the file's format string back for
indexes 1–22 and 37–49 (apache/fesod#1195).
## Proposed fix
When a `FormatRecord` exists for the index, use it whatever the index is.
Fall back to `HSSFDataFormat.getBuiltinFormat` only when the file defines
nothing for it.
## Behaviour-change risk for existing users
This would change output for existing callers:
- Anyone reading `.xls` files saved by a non-English Excel (with
`DataFormatter.formatRawCellContents(value, index, formatString)` or by
inspecting the format string) would see results change for currency/accounting
cells. For example `$1,234.50` would become `￥1,234.50` or `1.234,50 €`. That
is arguably more correct, but it is visible, and some users may have tests or
downstream parsing that depend on the current `$` output.
- Files that override other built-in indexes (dates 14/22 are sometimes
redefined) would also change.
- Indexes 23–36 are reserved placeholders in POI's built-in table. A file's
FORMAT record there would now be returned rather than POI's `reserved-0x..`
string. That is probably desirable, but it is still a change.
## Suggestions for making it opt-in
Any of these would keep current behaviour by default:
1. **Flag on the listener** (smallest change): a
`setUseFileFormatsForBuiltinIndexes(boolean)` setter or a constructor overload.
It defaults to `false` in 5.x and could switch to `true` in the next major
release.
2. **New method**: add something like `getFileFormatString(int formatIndex)`
that returns the file's FORMAT record string, or `null` when there isn't one.
Callers can then choose. `getFormatString` stays unchanged.
3. **Protected accessor**: expose `_customFormatRecords`, or a `protected
FormatRecord getFormatRecord(int)`, so subclasses can implement the lookup
without re-tracking FORMAT records in `processRecordInternally`. Fesod
currently has to do that re-tracking.
Option 1 plus a release note seems best: it fixes the inconsistency with
`HSSFDataFormat` for anyone who opts in, and gives a migration path towards
making it the default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
