Opened 5 years ago

Closed 2 months ago

Last modified 2 months ago

#11857 closed enhancement (fixed)

Add support for %V to wxDateTime::Format()

Reported by: BuschnicK Owned by: VZ
Priority: low Milestone:
Component: wxMSW Version: stable-latest
Keywords: wxDateTime Format simple week-number tests-needed Cc: al@…
Blocked By: Blocking:
Patch: no

Description

wxDateTime time( 31, wxDateTime::Dec, 2009 );
time.GetWeekOfYear(); => 53, correct according to ISO
time.Format( wxT( "%W" ));
=> 52, wrong

wxDateTime time( 3, wxDateTime::Jan, 2010 );
time.GetWeekOfYear(); => 53, correct
time.Format( wxT( "%W" ));
=> 0, wrong

wxDateTime time( 4, wxDateTime::Jan, 2010 );
time.GetWeekOfYear(); => 1, correct
time.Format( wxT( "%W" ));
=> 1, correct

I realize Format is using strftime and GetWeekOfYear wxWidgets proprietary code but that doesn't help much if the results are different as shown above. It's not possible to mix the two...

I'm using wx2.9 on a German locale WindowsXP.

Attachments (2)

datetimefmt.patch download (1.0 KB) - added by blicharski 4 years ago.
Patch for src/common/datetimefmt.cpp file.
datetimefmt.2.patch download (1.1 KB) - added by blicharski 4 years ago.
Patch for src/common/datetimefmt.cpp file.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by vadz

  • Keywords Format simple added
  • Summary changed from wxDateTime week of year calculation is wrong to wxDateTime::Format() week of year calculation is wrong

This looks like a bug in the CRT of the compiler you use (which one is it?) that we'd need to work around by using GetWeekOfYear() instead of passing %W to strftime(). This shouldn't be difficult to do in wxDateTime itself but until this is done you may also do it in your own code.

comment:2 Changed 5 years ago by BuschnicK

That's my current workaround: preparse the strings before passing them to wxDateTime::Format.

I'm using MSVC2008.

comment:3 Changed 4 years ago by blicharski

There is a small patch fixing the problem.
It simply avoids calling strftime function, when %W flag is present as argument to the Format() method.

comment:4 Changed 4 years ago by alarsen

  • Cc al@… added

FWIW wxDateTime.Format() behaves correctly, since its documented behaviour is to call strftime().

POSIX/ISO-C99 [1] the %W specifier of strftime() does not give you the ISO week number but instead a week number [00..53] where the first monday in january is the first day of week 1.

On POSIX/ISO-C99 compliant systems you can use %V to get the ISO8601 week number.
Sadly, not even MSVC2010 supports %V, so we should probably implement that (and leave %W as is).

[1] http://www.opengroup.org/onlinepubs/009695399/functions/strftime.html

comment:5 follow-up: Changed 4 years ago by vadz

  • Keywords medium week-number added; simple removed
  • Status changed from new to confirmed
  • Summary changed from wxDateTime::Format() week of year calculation is wrong to Add support for %V to wxDateTime::Format() and GetCWeeekOfYear()
  • Version changed from 2.9.0 to 2.9-svn

Good point about the difference between %W and %V, I didn't know about the latter, thanks Anders.

Implementing %V isn't difficult (just another case in the switch) and I agree we should do this as it's more important for Format() to be compatible with strftime() than with GetWeekOfYear().

OTOH this would still leave us with two problems:

  1. %W is not handled correctly for the dates out of time_t range right now.
  2. There is no way to get what %W returns from wxDateTime directly.

So the ideal fix IMHO would be to either add C_Rules to WeekFlags or add a new GetCWeekOfYear() function which would return the week number according to %W rules. Then we'd use it to implement %W in our Format() implementation to fix (1) and it would become consistent with it, just as GetWeekOfYear() would be consistent with %V, fixing (2).

Now that we know what must be done it just remains to do it :-)

comment:6 in reply to: ↑ 5 Changed 4 years ago by alarsen

Replying to vadz:

  1. %W is not handled correctly for the dates out of time_t range right now.

IMHO that's a problem that can be safely ignored, see below.

  1. There is no way to get what %W returns from wxDateTime directly.

Does anybody really honestly need that?
I'd consider %W a legacy from the time before ISO8601, and AFAICT it doesn't match any widely-used week-numbering scheme anyway [1] (it's closest to the third case)

So the ideal fix IMHO would be to either add C_Rules to WeekFlags or add a new GetCWeekOfYear() function which would return the week number according to %W rules. Then we'd use it to implement %W in our Format() implementation to fix (1) and it would become consistent with it, just as GetWeekOfYear() would be consistent with %V, fixing (2).

I'd suggest we refrain from implementing C_Rules and GetCWeekOfYear() since it would only complicate the API and add confusion IMHO.
Likewise, I don't think we lose much by leaving %W as it is (and only concentrate on getting the %V case right).

Cheers
Anders

[1] http://en.wikipedia.org/wiki/Seven-day_week#Week_numbering

comment:7 Changed 4 years ago by vadz

  • Keywords simple added; medium removed
  • Summary changed from Add support for %V to wxDateTime::Format() and GetCWeeekOfYear() to Add support for %V to wxDateTime::Format()

Hmm, yes, thinking more about this I agree with you. The %W interpretation must be simply a bug in the original C library implementation which is now ensconced due to backwards compatibility. So just adding %V (and documenting this somewhere, e.g. with a link from GetWeekOfYear() description) would indeed be enough.

Changed 4 years ago by blicharski

Patch for src/common/datetimefmt.cpp file.

comment:8 Changed 4 years ago by blicharski

Thanks for comments. I attached a patch implementing %V option support.

It calls GetWeekOfYear() on Windows platform and other platforms not supporting strftime function.
It calls strftime on platforms supporting strftime with %V option (Linux, Unix, MacOSX).

I wonder whether calling GetWeekOfYear on all platforms wouldn't be a better solution. In this case when another platform supports strftime but doesn't support %V option, it is not necessary to change the code (but for some reason we call strftime function every time it is possible, so finally I've chosen the current way).

comment:9 Changed 4 years ago by vadz

Thanks for the patch but I don't understand why do we test only for isVOptionSupported here, surely we should also check whether %V is present before forcing the use of our own code?

The reason for calling standard strftime() function is that it might have some extensions which we know nothing about but that might be useful. Also, initially, we didn't implement all the specifiers. By now we probably do so we could conceivably stop calling strftime() at all but this would require more testing than I can provide.

Changed 4 years ago by blicharski

Patch for src/common/datetimefmt.cpp file.

comment:10 Changed 4 years ago by blicharski

Oh! I'm sorry I have no idea how it happened:P Improved patch attached.

comment:11 follow-up: Changed 4 years ago by vadz

  • Priority changed from normal to low
  • Type changed from defect to enhancement

I don't think we can assume that all strftime() implementations support %V unfortunately. We need either a configure test or a run-time test for this (whose result would presumably be cached).

And we also need unit tests for this...

comment:12 in reply to: ↑ 11 Changed 4 years ago by alarsen

Replying to vadz:

I don't think we can assume that all strftime() implementations support %V unfortunately.

We even know that MSVC2010 strftime() does not support %V (see alarsen above).

comment:13 Changed 4 years ago by vadz

  • Keywords tests-needed added

In fact this makes things even simpler: only configure needs logic to define HAVE_STRFTIME_PERCENT_V as no compilers under MSW seem to support this.

In any case we still need unit tests for this to avoid breaking this later.

comment:14 Changed 2 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 76958:

Add support for "%V" and "%G" to wxDateTime::Format().

This is useful for creating ISO 8601 week number based stamps.

Closes #11857.

comment:15 Changed 2 months ago by VZ

In 76988:

Add support for "%V" and "%G" to wxDateTime::Format().

This is useful for creating ISO 8601 week number based stamps.

Closes #11857.

comment:16 Changed 2 months ago by VZ

In 76989:

Add wxDateTime::GetWeekBasedYear().

It was just added as a private function to implement %V format specifier
support, just extract and document it as it could possibly be useful in its
own right.

See #11857.

Note: See TracTickets for help on using tickets.