jferard / fastods

A very fast and lightweight (no dependency) library for creating ODS (Open Document Spreadsheet, mainly for Calc) files in Java. It's a Martin Schulz's SimpleODS fork
GNU General Public License v3.0
35 stars 6 forks source link

Default-_-float-data #217

Closed ueen closed 3 years ago

ueen commented 3 years ago

Sorry to bother once more, theres an issue with setFloatValue or more specifically with Default-_-float-data it displays a number like this 1,00 but the actual value is 1 (same for Excel and Libreoffice)

it only is displayed correctly after removing all table:style-name="Default-_-float-data" tags maybe a setNumber method could be added without this tag in the final XML?

ueen commented 3 years ago

I fixed it, take a look https://github.com/jferard/fastods/pull/218

ueen commented 3 years ago

this simply doesnt add the float-data data style, which displays it as an actual number in Excel and Libreoffice, i think this makes sense regarding what to expect when you use setFloat (display 1,00) and setNumber (just display the number 1)

jferard commented 3 years ago

I looked at your PR. I agree with the principle. If you write:

cell.setFloatValue(10);

There is no reason why you get: 10.00. But I think this can be fixed in the method:

public void setFloatValue(final int value);

Rather than with a new method (plus, setNumberValue is pretty vague and does not tell you what number it is: integer, float, complex...).

ueen commented 3 years ago

I guess public void setFloatValue(final int value); could be changed, but, depending on how familiar you are with ods specs, it gets confusing fast, especially if you have methods with the same name (setFloatValue) but only some add a data style while others are not. So i think setNumber is the most intuitive, it is what i tried the first time and was supriesed, that there were no such method. If you really dont wont that (but i really would encourage you to change your mind) it might be possible to have a enum of default datasyles that could be passed as a second parameter to setFloatValue - so the default be no datastyle....but i think thats really even more confusing and changing the behaviour of existing setFloatValue will break all implementations if updated... So i think setNumberValue is the most sensible thing, it could also be renamed to setIntegerValue which is also confusing because the data type is float (which is a number) but not actually integer. Best would be to have this in some kind of documentation/example so one can read what each method does exactly.

jferard commented 3 years ago

I will think about it. Here are some remarks:

Best would be to have this in some kind of documentation/example so one can read what each method does exactly.

Some javadocs are outdated and could be improved.

Changing the behaviour of existing setFloatValue will break all implementations if updated

This is only half true: the behavior is not dramatically changed. If someone uses the setFloatValue(final int value) method, the int is expected (that's why there is an issue here). And this is the 0.7.3 version, hence some changes are expected.

The main objection to your suggestion is philosophical: FastODS was designed to be farther from XML than, let's say, Apache's ODF tooIkit. But not too far. That's why I'm reluctant to add a setXXXValue where XXX is not an office:value-type. As written, I'll think about it.

ueen commented 3 years ago

I absolutely understand that and as i said it makes total sense if youre familiar with ods specs, i just wanted to offer my perspective, as a unfamiliar dev just wanting to create some ods for wider compatibility.

On the philosophical side i really think methods should do what you expect from them, and that can be different things for different people and maybe the fault is on the spec for naming all numbers float but i'm not sure one has to stick to that if it complicates things that could be made more understandable and intuitive by renaming methods, that are on a higher abstraction level than the xml anyway.

I guess changing setFloatValue(final int value) is also fine but then i would suggest a carefull documentation and the option to pass the datastyle (maybe from a set of default ones) as a second parameter, which also should be documented well, but this goes really into enhancement and im not sure how much work you want to put in to this...

Well think about it and merge or modify if you made up your mind :)

jferard commented 3 years ago

Hi, I'm back on this issue. I made some tests with LO and there is no implicit data style added to float cells (contrary to what happens with percentage, date, currency cells). Hence I will remove the automatic data style and this will fix, incidentally, the issue with integers.

I'll update the doc too, to signal that every number is a float in the OpenDocument spec.

ueen commented 3 years ago

Great, that should solve the issue, its still kind of confusing but at least its consistent, so thanks :)

ueen commented 3 years ago

Hey, could you make a release so i can import through jitpack?

jferard commented 3 years ago

Ok, I'll try to find some time this week-end. Please use discussions for this kind of request: https://github.com/jferard/fastods/discussions/new.