Update Excel column width calculation to consistently respect emitter properties #2256

[cmontgomery-innovent]

Remove redundant call to autoSizeColumn
Set a lower bounds to calcWidth
Remove the conditional for setColumnWidth

[speckyspooky]

Why you have removed the state.currentSheet.autoSizeColumn(col, true); - you mean it redundant but why?
Because state.currentSheet.setColumnWidth will not called at every cases.

[cmontgomery-innovent]

I say redundant because we already calculate column width for every column using SheetUtil.getColumnWidth, so it's being calculated twice for every column. We already have the column width, why do we need to call autoSizeColumn when we can always call setColumnWidth?

I don't see the benefit of using autoSizeColumn, and the issues with it I've outlined in #2256

[speckyspooky]

The autoSizeColumn was used for the case calcWidth is equal or lower 1 and "excel/xlsx" was used to set the auto sizing.
Now with the change it will be never set. So you cannot say what the effect will be for all.
I would prefer to keep it in there and create an else-tree of if (calcWidth > 1.0) {

And you have changed the behaviour of column width which is set at column level of a table and the calculated with.
The calculated column width will be fetch from styling properties of the cell content.
The column width is coming from the table object but it will never used only like compare with the default value.

It seems to me that we can get side effects there.
Please be aware if you change it you have to consider to the other excel column handlings which work currently.

[cmontgomery-innovent]

The column width is coming from the table object but it will never used only like compare with the default value.

The block we're working in only gets executed if(forceAutoColWidths || columnWidth == defaultColumnWidth). So we can assume that if this block is executing, the column does not have a width defined in the report, or a flag was set to explicitly ignore the defined column widths. There should be no change in behavior if that flag is not set and the column's width is defined in the report.

I missed the if (calcWidth > 1.0) {, though I think it appropriate to remove altogether in order to always call setColumnWidth so as to avoid the issues I've noted about autoSizeColumn. Below is a few test cases with my proposed changes, and if (calcWidth > 1.0) { removed.

I'll use a blank string for the data to simulate a case where calcWidth is < 1.
Using a mix of columns with defined widths (40px, 80px, 160px), and undefined widths (Unset) in the report design.
new_report.rptdesign.txt

Default emitter properties
Current:

D and F are sized based on their header as a result of autoSizeColumn. Note how column F did not actually get sized appropriately for the length of header.

With proposed change:
Defined width columns are unchanged, unset columns size to a minimum value if calcWidth < defaultColumnWidth

I argue that it makes more sense with the proposed change, which sets width to minimum value instead of using autoSizeColumn.

forceAutoColWidths = true
Current:

With proposed change:

excelAutoColWidthsIncludeTableHeader = true
Current:

With proposed change:

forceAutoColWidths = true AND excelAutoColWidthsIncludeTableHeader = true
Current:

With proposed change:

Please let me know your thoughts. Am I missing possible corner cases? Is there other issues with the before/after that need to be accounted for? The notable side effect is with those empty columns that have short headers are slightly wider than needed, is that a concern?

[speckyspooky]

Thanks for the test documentation and the test report.
A small changes of the Standard behavior is give, but for me it looks bettet like before.
The Only Thing what you could test is the width handling with mm because px wasn't the default at history.
I added px handling later.
If the mm looks good too then I would say we can go ahead with your improvement.

[cmontgomery-innovent]

Columns with fixed width in mm match original behavior

[speckyspooky]

Change approved