[Differential] [Commented On] D2577: KoResourcePaths: Fix handling of wildcards in directories and files

[staniek (Jarosław Staniek)]

staniek added a comment.


  Nice, some early notes.

INLINE COMMENTS

> KoResourcePaths.cpp:43
> +{
> +    static const QLoggingCategory category("calligra.lib.widgets.koresourcepaths");
> +    return category;

Can we drop the "ko" legacy here?

Or even: do we really need a category per class/namespace or per lib?

> KoResourcePaths.cpp:47
> +
> +#define debugKoResourcePaths qCDebug(KORESOURCEPATHS_LOG)<<QString("%1:").arg(__func__)
> +#define warnKoResourcePaths qCWarning(KORESOURCEPATHS_LOG)<<QString("%1:").arg(__func__)

Isn't the modern like this?

// in a header
Q_DECLARE_LOGGING_CATEGORY(driverUsb)

// in one source file
Q_LOGGING_CATEGORY(driverUsb, "driver.usb")

> KoResourcePaths.cpp:59
> +    {
> +        QStandardPaths::StandardLocation l = QStandardPaths::GenericDataLocation;
> +        if (m_standardlocations.contains(type)) {

let's avoid variables 'l' too much similar to 1 :)

> KoResourcePaths.h:122
>       *
> +     * Limitation: Wildcards is not supported.
> +     * 

is -> are ?

> KoResourcePaths.h:143
> +     *        may consist of an optional directory and a '*' wildcard. E.g. <tt>"images\*.jpg"</tt>.
> +     *        Also directories may contain wildcard. E.g. <tt>"images\*\stop.jpg"</tt>.
>       *        Use QString() if you do not want a filter.

What does the \s mean? Perhaps you meant /s ?

> KoResourcePaths.h:181
>       *         saved, or QString() if the resource type is unknown.
> +     *         The returned path always have a trailing '/'.
> +     *         If type does not exist, QString() is returned.

has

> KoResourcePaths.h:215
> +
> +#ifndef NDEBUG
> +    // for unit tests

Perhaps it's not the most accurate approach. Better is to #ifdef COMPILING_TESTS.

See also all other uses of #ifndef NDEBUG in this patch.

REPOSITORY
  rCALLIGRA Calligra

REVISION DETAIL
  https://phabricator.kde.org/D2577

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: danders, #calligra:_3.0, Calligra-Devel-list
Cc: staniek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20160829/3b528af5/attachment.htm>