D8536: Add more hashing algorithms to KPropertiesDialog

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:2657
> +class KChecksumAlgorithmListModel: public QAbstractListModel {
> +    QVector<QCryptographicHash::Algorithm> algorithms;
> +

Make it static const and initialize it here.

> kpropertiesdialog.cpp:2681-2682
> +    QVariant data(const QModelIndex &index, int role) const override {
> +        if (!index.isValid())
> +            return QVariant();
> +

Use braces {} even on one line block in all places.

> kpropertiesdialog.cpp:2757
>  {
> -    auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget);
> -    label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard);
> -
> -    d->m_ui.calculateWidget->layout()->replaceWidget(d->m_ui.sha1Button, label);
> -    d->m_ui.sha1Button->hide();
> -
> -    showChecksum(QCryptographicHash::Sha1, label, d->m_ui.sha1CopyButton);
> -}
> -
> -void KChecksumsPlugin::slotShowSha256()
> -{
> -    auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget);
> -    label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard);
> -
> -    d->m_ui.calculateWidget->layout()->replaceWidget(d->m_ui.sha256Button, label);
> -    d->m_ui.sha256Button->hide();
> -
> -    showChecksum(QCryptographicHash::Sha256, label, d->m_ui.sha256CopyButton);
> -}
> -
> -void KChecksumsPlugin::slotVerifyChecksum(const QString &input)
> -{
> -    auto algorithm = detectAlgorithm(input);
> -
> -    // Input is not a supported hash algorithm.
> -    if (algorithm == QCryptographicHash::Md4) {
> -        if (input.isEmpty()) {
> -            setDefaultState();
> -        } else {
> -            setInvalidChecksumState();
> -        }
> +    if (algorithms.isEmpty()){
>          return;

1 space between ) { in all places

> kpropertiesdialog.cpp:2777-2794
>      auto futureWatcher = new QFutureWatcher<QString>(this);
>      connect(futureWatcher, &QFutureWatcher<QString>::finished, this, [=]() {
> -
>          const QString checksum = futureWatcher->result();
>          futureWatcher->deleteLater();
>  
> -        cacheChecksum(checksum, algorithm);
> -
> -        switch (algorithm) {
> -        case QCryptographicHash::Md5:
> -            slotShowMd5();
> -            break;
> -        case QCryptographicHash::Sha1:
> -            slotShowSha1();
> -            break;
> -        case QCryptographicHash::Sha256:
> -            slotShowSha256();
> -            break;
> -        default:
> -            break;
> -        }
> +        showChecksum(algorithm);
>  

Why not create all watchers in a loop not in recursion. When you use non-const QVector it can't garanteed that it'll not make a deep copy.

  verifyChecksumRecursive(input, std::move(algorithms));

?

> kpropertiesdialog.cpp:2782
>  
> -        cacheChecksum(checksum, algorithm);
> -
> -        switch (algorithm) {
> -        case QCryptographicHash::Md5:
> -            slotShowMd5();
> -            break;
> -        case QCryptographicHash::Sha1:
> -            slotShowSha1();
> -            break;
> -        case QCryptographicHash::Sha256:
> -            slotShowSha256();
> -            break;
> -        default:
> -            break;
> -        }
> +        showChecksum(algorithm);
>  

Are you sure about calling this function in all watchers?

REPOSITORY
  R241 KIO

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

To: petermajchrak, elvisangelaccio
Cc: anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171108/c842bb8e/attachment-0001.html>