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

[David Jarvie]

> On 2009-07-21 12:23:53, David Jarvie wrote:
> > Some minor comments to help make some things more understandable.
> 
> Michael Leupold wrote:
>     Thanks for your comments. Also feel free to bug me on IRC if you have further comments or questions.
>     
>     One thing I should probably change is using KLocale::ReadTimeFlags instead of plain bool for the includeSecs arguments. Do you agree?

The ReadTimeFlags values exactly correspond to the bool parameter's true and false, so it would definitely be the preferred parameter type.


- David


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


On 2009-07-21 13:27:09, Michael Leupold wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1065/
> -----------------------------------------------------------
> 
> (Updated 2009-07-21 13:27:09)
> 
> 
> Review request for kdelibs.
> 
> 
> 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
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.h 1000446 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1000446 
>   /trunk/KDE/kdelibs/kdecore/tests/CMakeLists.txt 1000446 
>   /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
> 
>