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

Localisation Based NumberFormatException on getWidth #332

Closed
AndreasTAK opened this issue Oct 23, 2024 · 6 comments · Fixed by #333
Closed

Localisation Based NumberFormatException on getWidth #332

AndreasTAK opened this issue Oct 23, 2024 · 6 comments · Fixed by #333
Milestone

Comments

@AndreasTAK
Copy link
Contributor

Depending on Locale OdfTableColumn.getWidth() produces a NumberFormatException.
Steps to reproduce

  1. Set computer locale to "de" (german)
  2. run the following code
     var spreadsheetDocument = OdfSpreadsheetDocument.newSpreadsheetDocument();
     var table = OdfTable.newTable(spreadsheetDocument);
     var columnWidth = table.getColumnByIndex(0).getWidth();

will lead to:

java.lang.NumberFormatException: For input string: "001,2000"
	at java.base/jdk.internal.math.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:2054)
	at java.base/jdk.internal.math.FloatingDecimal.parseDouble(FloatingDecimal.java:110)
	at java.base/java.lang.Double.parseDouble(Double.java:792)
	at java.base/java.lang.Double.valueOf(Double.java:755)
	at org.odftoolkit.odfdom.type.Length.parseDouble(Length.java:207)
	at org.odftoolkit.odfdom.type.Length.parseLong(Length.java:179)
	at org.odftoolkit.odfdom.doc.table.OdfTableColumn.getWidth(OdfTableColumn.java:144)
	at org.odftoolkit.odfdom.doc.table.OdfTableColumn.setWidth(OdfTableColumn.java:173)
@svanteschubert
Copy link
Contributor

svanteschubert commented Oct 24, 2024

@AndreasTAK, is there any chance of getting a patch from you, Andreas? It would be most welcome! 🤗
We could start by adding the following lines of code to an existing test file! 👍

@AndreasTAK
Copy link
Contributor Author

@svanteschubert so I looked into it and the issue originally stems from the createTable() method. To define the column-width style attribute java.text.DecimalFormat is used without a DecimalFormatSymbols parameter which then defaults to the users OS locale. I am not sure if the potentially resulting string using , as a decimal separator conforms with OpenDocument specs. LibreOffice handles it fine, Microsoft Excel does not. My proposed fix would be to define the DecimalFormatSymbols with the English Locale. You use DecimalFormat without a defined DecimalFormatSymbols in OdfTableCell.java as well however I did not look into whether this could lead to the same issue yet. I have implemented the fix in #333

@svanteschubert
Copy link
Contributor

@AndreasTAK Many thanks, Andreas for the patch.
Checked out the sources locally on Linux with 'gh pr checkout 333' (see docs) and double checked for further occurrences of the same problem via 'find . -name "*.java" | xargs grep DecimalFormat' and found two classes where DecimalFormat is being used as well:

  1. ./odfdom/src/main/java/org/odftoolkit/odfdom/doc/table/OdfTableCell.java
  2. ./odfdom/src/test/java/org/odftoolkit/odfdom/doc/table/TableCellTest.java

Honestly, as I am uncertain how to edit your patch, may I ask if you add a test (as you described above) in TableCellTest.java and apply a similar test & patch to OdfTableCell.java? ;-)

I believe the internal XML version is all the same in ODF and just the GUI of Calc shows different localized separators, therefore your patch seems quite correct to me - I have asked Michael Stahl as well, but he might be on vacation (the only thing that might delay this here a bit) :-)

Thanks in advance, Andreas!
Greetings from Berlin 👋

AndreasTAK added a commit to AndreasTAK/odftoolkit that referenced this issue Oct 24, 2024
@AndreasTAK
Copy link
Contributor Author

@svanteschubert I have added a test to demostrate my fix works with german locale.
Also I checked into the other two instances where DecimalFormat is used.
Both these instances only affect DisplayText which is never parsed as an actual number so no Exception occurs, in this case using the default System locale is most likely fine and I think doesn't need changing.
As is noted by the documentation of the setDisplayText method in OdfTableCell and I just also confirmed with my own testing both for microsoft excel as for LibreOffice most odf viewers calculate the display value for table cells themselves and disregard what is in the display text property.
In case a change is desired anyway since this is in regards to the display text I am not sure if the system default, the documents locale or simply a unified english locale would be the best choice.

Greetings from Vienna, Austria👋

mistmist pushed a commit that referenced this issue Oct 25, 2024
@mistmist mistmist added this to the 0.13.0 milestone Oct 25, 2024
@svanteschubert
Copy link
Contributor

@AndreasTAK @mistmist
Thank you both, I have just uploaded another SNAPSHOT release for all ODF Toolkit deliverables, like for the validator at
https://oss.sonatype.org/content/repositories/snapshots/org/odftoolkit/odfvalidator/0.13.0-SNAPSHOT/

@AndreasTAK
Copy link
Contributor Author

@svanteschubert Fantastic. Thanks for the quick review and SNAPSHOT release. I'll keep my eyes open for any other issues I might find

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

Successfully merging a pull request may close this issue.

3 participants