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

[David Jarvie]

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


Some minor comments to help make some things more understandable.


/trunk/KDE/kdelibs/kdecore/localization/klocale.h
<http://reviewboard.kde.org/r/1065/#comment1099>

    Add some hint as to how 12/24h format can be specified.



/trunk/KDE/kdelibs/kdecore/localization/klocale.h
<http://reviewboard.kde.org/r/1065/#comment1100>

    Does this mean to format the hours/minutes/seconds ignoring am/pm, and set a maximum value of < 24 hours? Please make it clearer.



/trunk/KDE/kdelibs/kdecore/localization/klocale.h
<http://reviewboard.kde.org/r/1065/#comment1101>

    noAmPm -> no am/pm



/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp
<http://reviewboard.kde.org/r/1065/#comment1102>

    Suggest renaming parameter 'strip' to 'strip2char' to hint that it must be a 2-character string.



/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp
<http://reviewboard.kde.org/r/1065/#comment1104>

    What is #164813? If it's a bug number, it should be removed since the bug presumably won't exist after this code is committed. If it is something else, it should be explained.



/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp
<http://reviewboard.kde.org/r/1065/#comment1103>

    Simplification:
    inout.remove(remPos, endPos - remPos);


- David


On 2009-07-20 22:03:02, Michael Leupold wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1065/
> -----------------------------------------------------------
> 
> (Updated 2009-07-20 22:03:02)
> 
> 
> 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 1000119 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1000119 
>   /trunk/KDE/kdelibs/kdecore/tests/CMakeLists.txt 1000119 
>   /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
> 
>