Review Request 114187: KFormat - Add new KFormat class

[Alex Merry]

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


This looks like a sensible overall design to me.


tier1/kcoreaddons/src/lib/util/kformat.h
<http://git.reviewboard.kde.org/r/114187/#comment31969>

    I think the general trend now is not to include the module name.



tier1/kcoreaddons/src/lib/util/kformat.h
<http://git.reviewboard.kde.org/r/114187/#comment31965>

    You're only using this for the translations, right?  I think you can use Q_DECLARE_TR_FUNCTIONS(KFormat) instead (and, indeed, use it in KFormatPrivate directly) and avoid the overhead of allocating all the QObject stuff.



tier1/kcoreaddons/src/lib/util/kformat.h
<http://git.reviewboard.kde.org/r/114187/#comment31967>

    This suggests you want a copy constructor.



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31968>

    Module name again.



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31970>

    Call them "//:" comments, since that's how they look?



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31966>

    Q_ASSERT?  And does gcc really warn about this?  I thought it was cleverer than that...



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31972>

    I think you want to use %n and the third argument to tr() (see http://doc-snapshot.qt-project.org/qt5-stable/i18n-source-translation.html#handling-plurals)



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31973>

    Use %n instead of %1 and the .arg()



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31974>

    I think these want a translator comment about the argument



tier1/kcoreaddons/src/lib/util/kformatprivate.cpp
<http://git.reviewboard.kde.org/r/114187/#comment31975>

    Include the comment about if it doesn't fit the grammar again?


- Alex Merry


On Nov. 28, 2013, 8 p.m., John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114187/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2013, 8 p.m.)
> 
> 
> Review request for KDE Frameworks, Albert Astals Cid, David Faure, and Kevin Ottens.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> KFormat - Add new KFormat class
> 
> KLocale offers a number of extra formatting options not yet available
> in Qt.  The KFormat class adds these options to KCoreAddons:
> 
> * formatByteSize()
> * formatDuration()
> * formatDecimalDuration()
> * formatSpelloutDuration()
> * formatRelativeDate()
> * formatRelativeDateTime()
> 
> The KFormat class can be initialised with any QLocale to use in the
> date and number formatting, or the default locale can be easily
> accessed via KFormat():
> 
>   QString result = KFormat().formatDuration(1000);
> 
> ----------------------------------------
> 
> There's a few things that need looking at here.  The main one is the translation stuff because I had to convert from using ki18n to tr and it may have lost something in the process.  In particular it looks like we'll actually need an en_US translation done to get the plurals right?  If we can't make tr() work for these we'll have to move the class into  k18n.  The second is to look at the formatting options provided and decide if they are actually useful to have.  The third is to confirm that the design is OK, I did think about making these simple static methods with an extra parm for QLocale, but I think this approach offers more future flexibility, and writing KFormat().formatDuration() is just as convenient as KFormat::formatDuration().
> 
> 
> Diffs
> -----
> 
>   tier1/kcoreaddons/autotests/CMakeLists.txt c8043576181e7d06663195d017be930d0bdcbde9 
>   tier1/kcoreaddons/autotests/kformattest.h PRE-CREATION 
>   tier1/kcoreaddons/autotests/kformattest.cpp PRE-CREATION 
>   tier1/kcoreaddons/src/lib/CMakeLists.txt 638525f7b719bcd0bc1dfdf94debd51296521334 
>   tier1/kcoreaddons/src/lib/util/kformat.h PRE-CREATION 
>   tier1/kcoreaddons/src/lib/util/kformat.cpp PRE-CREATION 
>   tier1/kcoreaddons/src/lib/util/kformatprivate.cpp PRE-CREATION 
>   tier1/kcoreaddons/src/lib/util/kformatprivate_p.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/114187/diff/
> 
> 
> Testing
> -------
> 
> Autotests copied from KLocale tests and improved.
> 
> 
> Thanks,
> 
> John Layt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131128/614735cc/attachment-0001.html>