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

[Michael Leupold]

> On 2009-07-21 12:23:53, David Jarvie wrote:
> > Some minor comments to help make some things more understandable.

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?


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp, line 2043
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8715#file8715line2043>
> >
> >     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.

It's a bug number someone left in as some sort of quasi-explanation. I think it can be removed because this bug mostly occurred when %S was removed while constructing the output (and not beforehand like it is now).


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.h, lines 727-728
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8714#file8714line727>
> >
> >     Add some hint as to how 12/24h format can be specified.

I tried. Hope the new explanation and example clear things up.


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.h, lines 729-730
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8714#file8714line729>
> >
> >     Does this mean to format the hours/minutes/seconds ignoring am/pm, and set a maximum value of < 24 hours? Please make it clearer.

Yes it does. Changed the description, hope that's clearer as well now :)


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.h, line 935
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8714#file8714line935>
> >
> >     noAmPm -> no am/pm

Leftover from refactoring. Removed the parameter description altogether and replaced with what belongs there.


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp, line 2032
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8715#file8715line2032>
> >
> >     Suggest renaming parameter 'strip' to 'strip2char' to hint that it must be a 2-character string.

Agreed. Also added a function description.


> On 2009-07-21 12:23:53, David Jarvie wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp, line 2058
> > <http://reviewboard.kde.org/r/1065/diff/2/?file=8715#file8715line2058>
> >
> >     Simplification:
> >     inout.remove(remPos, endPos - remPos);

Thanks.


- Michael


-----------------------------------------------------------
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
> 
>