Review Request: Rewrite of KLocale::readTime() and KLocale::formatTime()

[Michael Leupold]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1065/
-----------------------------------------------------------

(Updated 2009-07-22 22:57:39.754729)


Review request for kdelibs.


Changes
-------

Did some cleanup and consistency checking, in particular:
- Add a new TimeFormatOptions enum with seconds fields, deprecate ReadTimeFlags. Like this all options/flags are in one place and have a proper name (no more ReadTimeFlags/FormatTimeFlags).
- Rename new readTime to readLocaleTime() (arguments clashing when overloading), deprecate old readTime call.
- Rename new formatTime to formatLocaleTime() for consitency, deprecate old formatTime call.

Unsure about:
- argument order for readTime() as I don't know if someone rather uses "ok" without "options" or vice-versa
- default value for strict (false seems saner)

Next step:
- Making use of the new flags when formatting DateTimes would be nice as well. I'd prefer this patch to be committed first though.


Summary
-------

While working on a QValidator for time entry I found KLocale::readTime() and KLocale::formatTime() to be somewhat lacking in certain areas:
- stripping of AM/PM ("%p") doesn't work for all locales
- if I don't want AM/PM I need to format as a duration (which means it will be 24h) which is inconvenient in cases where the AM/PM is "external" (eg. inside a combobox to choose from). Not having to mod 12 the hours beforehand seems more convenient.
- checking is always strict. As the method reads user input there should be a way to make it operate in lax mode (ie. ignoreing missing or additional spaces in user input)
- readTime() has less features than formatTime (no am/pm stripping or durations).
- signatures for readTime() and formatTime() could be adapted to match each other somewhat.

I tried to achieve all of the above with a partial rewrite of those methods using the old code and some code I wrote while developing the validator. I also wrote an additional testcase for reading/formatting times.


Diffs (updated)
-----

  /trunk/KDE/kdelibs/kdecore/localization/klocale.h 1001057 
  /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1001057 
  /trunk/KDE/kdelibs/kdecore/tests/CMakeLists.txt 1001057 
  /trunk/KDE/kdelibs/kdecore/tests/klocaletest.cpp 1001057 
  /trunk/KDE/kdelibs/kdecore/tests/klocaletimeformattest.h PRE-CREATION 
  /trunk/KDE/kdelibs/kdecore/tests/klocaletimeformattest.cpp PRE-CREATION 

Diff: http://reviewboard.kde.org/r/1065/diff


Testing
-------

klocaletest succeeds, the new testcase succeeds as well.


Thanks,

Michael