D17245: Add string formatting function to property info

[Stefan Brüns]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181217/00930e62/attachment-0001.html>