tfussell / xlnt

:bar_chart: Cross-platform user-friendly xlsx library for C++11+
Other
1.47k stars 409 forks source link

cell::is_date() now correctly works for dates #602

Closed doomlaur closed 2 years ago

doomlaur commented 2 years ago

cell::is_date() only worked correctly for times (saved in Excel as numbers), but not for dates (saved in Excel as strings - usually shared_strings). This pull request only adds a few more allowed types for dates, as all the other checks such as is_date_format() already exist and work very well.

doomlaur commented 2 years ago

It seems that my change breaks one unit test. I'll explain my motivation behind this, why it breaks that unit test, and what can (or can't) be done against it.

So my problem was that writing in Excel something like 1.12.2021 15:10 and setting it to Date caused the date to be saved as a shared_string instead of a number (which was already supported by XLNT). Anyway, this means that cell::is_date() saw that the type is not a number and immediately said it's not a date - which is what I fixed in my previous commit.

However, cell::is_date() checks whether the format is a date format, but NOT whether it could be used to parse a string. That's the information we'd need. However, this would be more complicated: first, number_formatter::format_text() would need to consider that the string might be a date/time. Second, we could use something like std::get_time to parse the string (or std::chrono::parse in the far future, with C++20). For that we'd need a format string (which we have already, but we must convert it to a format that can be used by std::get_time, e.g. for weekdays dddd -> a, or mmmm -> b). And then we'd need the correct locale to imbue the stream used for std::get_time. Although we do have the locale from Excel, I believe there is no cross-platform way to convert these to an std::locale, and I honestly don't even see an OS-specific proper way to do it.

In other words, as far as I see, what I want to do is unfortunately not possible. Please let me know if you have any ideas, otherwise I would close this pull request.

doomlaur commented 2 years ago

Oh, now I see. It seems Excel saved it as string because it never recognized that it's a date, in the first place - even when using a format string and a proper input for it. In that case, it's pretty much impossible to parse it properly, as explained above. Let me know if you have any ideas - until then, I'll close this pull request, as it makes no sense.

doomlaur commented 2 years ago

Ok, now I understand. The format string set in Excel for dates and times is used for displaying dates and times, but NOT for parsing them! So you must always make sure that you write the date in the same format as the Windows locale, or use the DATE function in Excel - otherwise the date will be parsed as text and you'll never know what's wrong.

If the date is parsed correctly, it is right-aligned in the cell in Excel, then saved by Excel as number, and also read by XLNT as a number. If it can't be parsed, it is left-aligned in the cell in Excel, then saved by Excel as a string, and also read by XLNT as a string. And this is obviously NOT a bug in XLNT!

It would be nice if Excel also tried to use the format string to also parse the date, and not just to display it. But that's life, I guess :)

doomlaur commented 2 years ago

Hmm, just an idea: wouldn't it work to just use the system locale - std::locale("") - or the C locale, when using std::get_time, and ignore the locale provided by Excel altogether? We would then have the last missing piece and could finally parse the string. What wouldn't work in this case are locale-specific things like month names or weekday names - but it's nothing XLNT could fix at this point, as explained above. At least it would work in a lot of other cases.

But then it's a good question whether XLNT should support something that Excel itself doesn't support. However, we already have a string to parse and a format string (which needs to be "translated" to be usable with std::get_time), so we have all pieces to actually do it.