bruns added inline comments.
INLINE COMMENTS
> formatstrings.cpp:27
> +
> +const KFormat FormatStrings::form;
> +
I don't think using a static default constructed KFormat is a good idea ...
> formatstrings.cpp:50
> + QTime time = dt.time();
> + if (!time.hour() && !time.minute() && !time.second()){
> + return FormatStrings::form.formatRelativeDate(dt.date(),
> QLocale::LongFormat);
Why is midnight an invalid time?
> propertyinfo.cpp:567
> +
> + if (d->formatAsString == nullptr) {
> + if (d->valueType == QVariant::StringList) {
I think this would be better written as `d->formatAsString =
&FormatStrings::toStringFunction;` unconditionally on the top (like
`d->shouldBeIndexed = true`), and `d->formatAsString =
&FormatStrings::joinStringListFunction;` in the few matching case statements.
> propertyinfo.h:93
> + */
> + QString formatAsDisplayString(const QVariant& value) const;
> +
I think you should hand in a KFormat here if you want to avoid constructing a
new one for each value.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D17245
To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich,
spoorun, bruns, abrahams
