[Differential] [Requested Changes To] D2987: Detect compression method

[elvisangelaccio (Elvis Angelaccio)]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> archive_kerfuffle.h:225
>      void onNewEntry(const Archive::Entry *entry);
> +    void onFoundCompressionMethod(const QStringList methods);
>  

Missing reference in the argument

> archiveinterface.h:170
>      void testSuccess();
> +    void foundCompressionMethod(const QStringList);
>  

Can we call this `compressionMethodFound`? Same with the slots, e.g. onCompressionMethodFound

> cliplugin.cpp:270-274
> +                    // If other methods are found, we dont report "Store" method.
> +                    if (m_compressionMethods.size() > 1 &&
> +                        m_compressionMethods.contains(QStringLiteral("Store"))) {
> +                        m_compressionMethods.removeOne(QStringLiteral("Store"));
> +                    }

This check is duplicated in two plugins. Can we move it in the Archive's slot that sets the new property?

> cliplugin.cpp:220
> +            if (line.contains(QLatin1String("RAR 4"))) {
> +                emit foundCompressionMethod(QStringList({QStringLiteral("RAR4")}));
> +            } else if (line.contains(QLatin1String("RAR 5"))) {

This creates a temporary `std::initializer` list that is then converted to QStringList. A more efficient way is using `QStringList {QStringLiteral("foo")}`

> cliplugin.cpp:222
> +            } else if (line.contains(QLatin1String("RAR 5"))) {
> +                emit foundCompressionMethod(QStringList({QStringLiteral("RAR4")}));
> +            }

same as above.

> cliplugin.cpp:245
> +    if (formatName == QLatin1String("RAR")) {
> +        emit foundCompressionMethod(QStringList({QStringLiteral("RAR4")}));
> +    } else if (formatName == QLatin1String("RAR 5")) {

`QStringList {QStringLiteral("foo")}`

> cliplugin.cpp:247
> +    } else if (formatName == QLatin1String("RAR 5")) {
> +        emit foundCompressionMethod(QStringList({QStringLiteral("RAR5")}));
> +    }

`QStringList {QStringLiteral("foo")}`

> libarchiveplugin.cpp:63
> +    if (!compMethod.isEmpty()) {
> +        emit foundCompressionMethod(QStringList({compMethod}));
> +    }

`QStringList {compMethod}`

REPOSITORY
  rARK Ark

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

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

To: rthomsen, elvisangelaccio
Cc: kde-utils-devel, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20161009/cd8f9581/attachment-0001.html>