Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Auto sizing not working properly due to rounding error when calculating defaultCharWidth #541

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

NiklasMelznerExxcellent
Copy link

The XSSFSheet#autoSizeColumn method doesn't work properly for some specific cell values and font metrics.

The following code snippet is an example for this behavior:

String sheetName = "Sheet 1";
String cellText = "Auto sizing incorrect! :(";

try (XSSFWorkbook workbook = new XSSFWorkbook()) {

    final XSSFSheet sheet = workbook.createSheet(sheetName);

    final XSSFCell cell = sheet.createRow(0).createCell(0);

    cell.setCellValue(cellText);

    final XSSFFont font = workbook.createFont();
    font.setFontName("Arial");
    font.setFontHeight(15);
    font.setBold(true);

    final XSSFCellStyle style = workbook.createCellStyle();
    style.setFont(font);
    cell.setCellStyle(style);

    sheet.autoSizeColumn(0, false);

    try (FileOutputStream fOS = new FileOutputStream("./test-excel.xlsx")) {
        workbook.write(fOS);
    }
}

Generated excel file:
image

The bug is caused by a rounding error in SheetUtil#getDefaultCharWidth:

TextLayout layout = new TextLayout(str.getIterator(), fontRenderContext);
return Math.round(layout.getAdvance());

Rounding is not necessary at all, removing the operation and converting the return type of getDefaultCharWidth() to double solves the issue.

The column width calculation that causes the error (and uses a value returned by SheetUtil#getDefaultCharWidth) occurs in SheetUtil#getCellWidth:

return Math.max(minWidth, ((frameWidth / colspan) / defaultCharWidth) + style.getIndention());

@pjfanning
Copy link
Contributor

I added some changes in 8819952

I didn't want to break backward compatibility so I've added new methods and deprecated the ones that they replace.

It's hard to automate tests because the calculated widths depend on the font setup that testers have installed.

If you can spot a mistake or could try out my change that would be great. I will double check the code change later and it is possible it might break a few tests.

@NiklasMelznerExxcellent
Copy link
Author

I can't see any additional failing tests (except the ones that fail due to my locale but aren't related to the issue).
My example seems to work too:
image
Do you know when the next version will be released yet?

@pjfanning
Copy link
Contributor

It will be a few weeks before POI 5.2.5 is released. We are concentrating on releasing XMLBeans 5.2.0 first and once that is released, we should be in a position to start a POI release vote.

@NiklasMelznerExxcellent
Copy link
Author

Alright, thank you for addressing the issue so quickly!

@pjfanning
Copy link
Contributor

@NiklasMelznerExxcellent Some users are complaining that this change is still creating cells that are not wide enough. Would you have any interest in having another look?

I'm wondering whether it might be worth just arbitrarily increasing the width of each cell by some hardcoded width. Maybe we could make this configurable but default to some non-zero value.

@niklasmelzner
Copy link

niklasmelzner commented Dec 10, 2023

I guess you're referring to this bug?
I'll have a look at it next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants