----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
