Differential D8536 Diff 23772 src/widgets/kpropertiesdialog.cpp

Changeset View

Standalone View

View Options

src/widgets/kpropertiesdialog.cpp

Context not available.
53 53​
54#include <QCheckBox> 54​#include <QCheckBox>
55#include <QClipboard> 55​#include <QClipboard>
56​#include <QtConcurrent/QtConcurrent>
56#include <QDebug> 57​#include <QDebug>
57#include <QDBusConnection> 58​#include <QDBusConnection>
58#include <QDBusInterface> 59​#include <QDBusInterface>
Context not available.
67#include <QLabel> 68​#include <QLabel>
68#include <QLayout> 69​#include <QLayout>
69#include <QLocale> 70​#include <QLocale>
71​#include <QMetaEnum>
70#include <QMimeDatabase> 72​#include <QMimeDatabase>
71#include <QMutableStringListIterator> 73​#include <QMutableStringListIterator>
72#include <QPainter> 74​#include <QPainter>
Context not available.
2647 Ui::ChecksumsWidget m_ui; 2649​ Ui::ChecksumsWidget m_ui;
2648 2650​
2649 QFileSystemWatcher fileWatcher; 2651​ QFileSystemWatcher fileWatcher;
2650 QString m_md5; 2652​ QMap<QCryptographicHash::Algorithm, QString> cache;
2651 QString m_sha1; 2653​ mutable QMutex mutex;
2652 QString m_sha256; ​
2653}; 2654​};
2654 2655​
2655KChecksumsPlugin::KChecksumsPlugin(KPropertiesDialog *dialog) 2656​KChecksumsPlugin::KChecksumsPlugin(KPropertiesDialog *dialog)
2656 : KPropertiesDialogPlugin(dialog), d(new KChecksumsPluginPrivate) 2657​ : KPropertiesDialogPlugin(dialog), d(new KChecksumsPluginPrivate)
anthonyfieroniUnsubmitted
Done

Make it static const and initialize it here.

anthonyfieroni: Make it static const and initialize it here.
2657{ 2658​{
2658 d->m_ui.setupUi(&d->m_widget); 2659​ d->m_ui.setupUi(&d->m_widget);
2659 properties->addPage(&d->m_widget, i18nc("@title:tab", "C&hecksums")); 2660​ properties->addPage(&d->m_widget, i18nc("@title:tab", "Checksums"));
2660 ​
2661 d->m_ui.md5CopyButton->hide(); ​
2662 d->m_ui.sha1CopyButton->hide(); ​
2663 d->m_ui.sha256CopyButton->hide(); ​
2664 2661​
2665 connect(d->m_ui.lineEdit, &QLineEdit::textChanged, this, [=](const QString &text) { 2662​ connect(d->m_ui.lineEdit, &QLineEdit::textChanged, this, [=](const QString &text) {
2666 slotVerifyChecksum(text.toLower()); 2663​ slotVerifyChecksum(text.toLower());
2667 }); 2664​ });
2668 2665​
2669 connect(d->m_ui.md5Button, &QPushButton::clicked, this, &KChecksumsPlugin::slotShowMd5); ​
2670 connect(d->m_ui.sha1Button, &QPushButton::clicked, this, &KChecksumsPlugin::slotShowSha1); ​
2671 connect(d->m_ui.sha256Button, &QPushButton::clicked, this, &KChecksumsPlugin::slotShowSha256); ​
2672 ​
2673 d->fileWatcher.addPath(properties->item().localPath()); 2666​ d->fileWatcher.addPath(properties->item().localPath());
2674 connect(&d->fileWatcher, &QFileSystemWatcher::fileChanged, this, &KChecksumsPlugin::slotInvalidateCache); 2667​ connect(&d->fileWatcher, &QFileSystemWatcher::fileChanged, this, &KChecksumsPlugin::slotInvalidateCache);
2675 2668​
2676 auto clipboard = QApplication::clipboard(); 2669​ auto clipboard = QApplication::clipboard();
2677 connect(d->m_ui.md5CopyButton, &QPushButton::clicked, this, [=]() { ​
2678 clipboard->setText(d->m_md5); ​
2679 }); ​
2680 2670​
2681 connect(d->m_ui.sha1CopyButton, &QPushButton::clicked, this, [=]() { 2671​ for(const auto alg : {
2682 clipboard->setText(d->m_sha1); 2672​ QCryptographicHash::Md4,
2683 }); 2673​ QCryptographicHash::Md5,
2684 2674​ QCryptographicHash::Sha1,
2685 connect(d->m_ui.sha256CopyButton, &QPushButton::clicked, this, [=]() { 2675​ QCryptographicHash::Sha224,
2686 clipboard->setText(d->m_sha256); 2676​ QCryptographicHash::Sha256,
2687 }); 2677​ QCryptographicHash::Sha384,
2678​ QCryptographicHash::Sha512,
2679​ QCryptographicHash::Sha3_224,
2680​ QCryptographicHash::Sha3_256,
2681​ QCryptographicHash::Sha3_384,
2682​ QCryptographicHash::Sha3_512
anthonyfieroniUnsubmitted
Done

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

anthonyfieroni: Use braces {} even on one line block in all places.
2683​ }) {
2684​ auto buttons = getButtons(alg);
2685​ auto calculateButton = buttons.first;
2686​ auto copyButton = buttons.second;
2687​ connect(calculateButton, &QPushButton::clicked, this, [=]() {
2688​ showChecksum(alg, calculateButton, copyButton);
2689​ });
2690​ copyButton->hide();
2691​ connect(copyButton, &QPushButton::clicked, this, [=]() {
2692​ clipboard->setText(d->cache.value(alg, ""));
2693​ });
2694​ }
2688 2695​
2689 connect(d->m_ui.pasteButton, &QPushButton::clicked, this, [=]() { 2696​ connect(d->m_ui.pasteButton, &QPushButton::clicked, this, [=]() {
2690 d->m_ui.lineEdit->setText(clipboard->text()); 2697​ d->m_ui.lineEdit->setText(clipboard->text());
Context not available.
2704 return false; 2711​ return false;
2705 } 2712​ }
2706 2713​
2707 const KFileItem item = items.first(); 2714​ const auto& item = items.first();
2708 return item.isFile() && item.isLocalFile() && item.isReadable() && !item.isDesktopFile() && !item.isLink(); 2715​ return item.isFile() && item.isLocalFile() && item.isReadable() && !item.isDesktopFile() && !item.isLink();
2709} 2716​}
2710 2717​
2711void KChecksumsPlugin::slotInvalidateCache() 2718​void KChecksumsPlugin::slotInvalidateCache()
2712{ 2719​{
2713 d->m_md5 = QString(); 2720​ d->cache.clear();
2714 d->m_sha1 = QString(); ​
2715 d->m_sha256 = QString(); ​
2716} 2721​}
2717 2722​
2718void KChecksumsPlugin::slotShowMd5() 2723​void KChecksumsPlugin::verifyChecksums(const QString input, QVector<QCryptographicHash::Algorithm> algorithms)
2719{ 2724​{
2720 auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget); 2725​ if (algorithms.isEmpty()) {
2721 label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard); 2726​ return;
2727​ }
2722 2728​
2723 d->m_ui.calculateWidget->layout()->replaceWidget(d->m_ui.md5Button, label); 2729​ auto futureWatcher = new QFutureWatcher<bool>(this);
2724 d->m_ui.md5Button->hide(); ​
2725 2730​
2726 showChecksum(QCryptographicHash::Md5, label, d->m_ui.md5CopyButton); 2731​ connect(futureWatcher, &QFutureWatcher<bool>::finished, this, [=]() {
2727} 2732​ const bool match = futureWatcher->result();
2733​ futureWatcher->deleteLater();
2728 2734​
2729void KChecksumsPlugin::slotShowSha1() 2735​ showChecksums();
2730{ ​
2731 auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget); ​
2732 label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard); ​
2733 2736​
2734 d->m_ui.calculateWidget->layout()->replaceWidget(d->m_ui.sha1Button, label); 2737​ if (match) {
2735 d->m_ui.sha1Button->hide(); 2738​ setMatchState();
2739​ } else {
2740​ setMismatchState();
2741​ }
2742​ });
2736 2743​
2737 showChecksum(QCryptographicHash::Sha1, label, d->m_ui.sha1CopyButton); 2744​ std::function<QString(QCryptographicHash::Algorithm)> mapper = [this](QCryptographicHash::Algorithm alg) {
2738} 2745​ const QString cache = cachedChecksum(alg);
2746​ if (!cache.isEmpty()) {
2747​ return cache;
2748​ }
2739 2749​
2740void KChecksumsPlugin::slotShowSha256() 2750​ const QString checksum = computeChecksum(alg, properties->item().localPath());
2741{ 2751​ cacheChecksum(checksum, alg);
2742 auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget); 2752​
2743 label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard); 2753​ return checksum;
2754​ };
2744 2755​
2745 d->m_ui.calculateWidget->layout()->replaceWidget(d->m_ui.sha256Button, label); 2756​ std::function<void(bool&, const QString&)> reducer = [input](bool& match, const QString& checksum) {
2746 d->m_ui.sha256Button->hide(); 2757​ match |= checksum == input;
anthonyfieroniUnsubmitted
Done

1 space between ) { in all places

anthonyfieroni: 1 space between ) { in all places
2758​ };
2747 2759​
2748 showChecksum(QCryptographicHash::Sha256, label, d->m_ui.sha256CopyButton); 2760​ QFuture<bool> future = QtConcurrent::mappedReduced<bool>(algorithms, mapper, reducer);
2761​
2762​ futureWatcher->setFuture(future);
2749} 2763​}
2750 2764​
2751void KChecksumsPlugin::slotVerifyChecksum(const QString &input) 2765​void KChecksumsPlugin::slotVerifyChecksum(const QString &input)
2752{ 2766​{
2753 auto algorithm = detectAlgorithm(input); 2767​ auto algorithms = detectAlgorithms(input);
2754 2768​
2755 // Input is not a supported hash algorithm. 2769​ // Input is not a supported hash algorithm.
2756 if (algorithm == QCryptographicHash::Md4) { 2770​ if (algorithms.isEmpty()) {
2757 if (input.isEmpty()) { 2771​ if (input.isEmpty()) {
2758 setDefaultState(); 2772​ setDefaultState();
2759 } else { 2773​ } else {
Context not available.
2762 return; 2776​ return;
2763 } 2777​ }
2764 2778​
2765 const QString checksum = cachedChecksum(algorithm); ​
2766 ​
2767 // Checksum alread in cache. ​
2768 if (!checksum.isEmpty()) { ​
2769 const bool isMatch = (checksum == input); ​
2770 if (isMatch) { ​
2771 setMatchState(); ​
2772 } else { ​
2773 setMismatchState(); ​
2774 } ​
2775 ​
2776 return; ​
2777 } ​
2778 ​
2779 // Calculate checksum in another thread. ​
2780 auto futureWatcher = new QFutureWatcher<QString>(this); ​
2781 connect(futureWatcher, &QFutureWatcher<QString>::finished, this, [=]() { ​
2782 ​
2783 const QString checksum = futureWatcher->result(); ​
2784 futureWatcher->deleteLater(); ​
2785 ​
2786 cacheChecksum(checksum, algorithm); ​
2787 ​
2788 switch (algorithm) { ​
2789 case QCryptographicHash::Md5: ​
2790 slotShowMd5(); ​
2791 break; ​
2792 case QCryptographicHash::Sha1: ​
2793 slotShowSha1(); ​
2794 break; ​
2795 case QCryptographicHash::Sha256: ​
2796 slotShowSha256(); ​
2797 break; ​
2798 default: ​
2799 break; ​
2800 } ​
2801 ​
2802 const bool isMatch = (checksum == input); ​
2803 if (isMatch) { ​
2804 setMatchState(); ​
2805 } else { ​
2806 setMismatchState(); ​
2807 } ​
2808 }); ​
2809 ​
2810 // Notify the user about the background computation. 2779​ // Notify the user about the background computation.
2811 setVerifyState(); 2780​ setVerifyState();
2812 2781​
2813 auto future = QtConcurrent::run(&KChecksumsPlugin::computeChecksum, algorithm, properties->item().localPath()); 2782​ verifyChecksums(input, algorithms);
anthonyfieroniUnsubmitted
Done

Are you sure about calling this function in all watchers?

anthonyfieroni: Are you sure about calling this function in all watchers?
2814 futureWatcher->setFuture(future); ​
2815} ​
2816 ​
2817bool KChecksumsPlugin::isMd5(const QString &input) ​
2818{ ​
2819 QRegularExpression regex(QStringLiteral("^[a-f0-9]{32}$")); ​
2820 return regex.match(input).hasMatch(); ​
2821} ​
2822 ​
2823bool KChecksumsPlugin::isSha1(const QString &input) ​
2824{ ​
2825 QRegularExpression regex(QStringLiteral("^[a-f0-9]{40}$")); ​
2826 return regex.match(input).hasMatch(); ​
2827} ​
2828 ​
2829bool KChecksumsPlugin::isSha256(const QString &input) ​
2830{ ​
2831 QRegularExpression regex(QStringLiteral("^[a-f0-9]{64}$")); ​
2832 return regex.match(input).hasMatch(); ​
2833} 2783​}
2834 2784​
2835QString KChecksumsPlugin::computeChecksum(QCryptographicHash::Algorithm algorithm, const QString &path) 2785​QString KChecksumsPlugin::computeChecksum(QCryptographicHash::Algorithm algorithm, const QString &path)
2836{ 2786​{
2837 QFile file(path); 2787​ QFile file(path);
2838 if (!file.open(QIODevice::ReadOnly)) { 2788​ if (!file.open(QIODevice::ReadOnly)) {
2839 return QString(); 2789​ return {};
2840 } 2790​ }
2841 2791​
2842 QCryptographicHash hash(algorithm); 2792​ QCryptographicHash hash(algorithm);
Context not available.
2845 return QString::fromLatin1(hash.result().toHex()); 2795​ return QString::fromLatin1(hash.result().toHex());
2846} 2796​}
2847 2797​
2848QCryptographicHash::Algorithm KChecksumsPlugin::detectAlgorithm(const QString &input) 2798​QVector<QCryptographicHash::Algorithm> KChecksumsPlugin::detectAlgorithms(const QString &input)
2849{ 2799​{
2850 if (isMd5(input)) { 2800​ static const QVector<QPair<QCryptographicHash::Algorithm, QRegularExpression>> regexes{
2851 return QCryptographicHash::Md5; 2801​ {QCryptographicHash::Md4, QRegularExpression("^[a-f0-9]{32}$")},
2852 } 2802​ {QCryptographicHash::Md5, QRegularExpression("^[a-f0-9]{32}$")},
2803​ {QCryptographicHash::Sha1, QRegularExpression("^[a-f0-9]{40}$")},
2804​ {QCryptographicHash::Sha224, QRegularExpression("^[a-f0-9]{56}$")},
anthonyfieroniUnsubmitted
Not Done

You don't understand me, now you have a race condition. cachedChecksum reads from cache while cacheChecksum writes. It's a race condition and you need mutex here. When you add here you will not needed anymore in cacheChecksum.

anthonyfieroni: You don't understand me, now you have a race condition. cachedChecksum reads from cache while…
petermajchrakAuthorUnsubmitted
Not Done

But adding a mutex for guarding the body of the mapper function will serialize the hash computations. The computations themselves are independent and parallelizable, only the synchronization points (cachedChecksum and cacheChecksum) are not.

petermajchrak: But adding a mutex for guarding the body of the mapper function will serialize the hash…
anthonyfieroniUnsubmitted
Not Done

I understand but you don't have a choice, now this *works* on luck you can face a crash when one thread read caches while one writes it.

anthonyfieroni: I understand but you don't have a choice, now this *works* on luck you can face a crash when…
petermajchrakAuthorUnsubmitted
Not Done

I can add two critical sections, one for reading the cache and one for writing while the hash computation is not in either of them

petermajchrak: I can add two critical sections, one for reading the cache and one for writing while the hash…
anthonyfieroniUnsubmitted
Not Done

Basically you need computation only one time when you don't have cache so you can check if cache does not exist then make a future watcher. In this case you can use blockedMappedReduced to wait all threads and mapper function looks like

std::function<QString(QCryptographicHash::Algorithm)> mapper = [this, path](QCryptographicHash::Algorithm alg) {
    const QString checksum = computeChecksum(alg, path);
     QMutexLocker locker(&d->mutex);
     cacheChecksum(checksum, alg);
}
anthonyfieroni: Basically you need computation only one time when you don't have cache so you can check if…
2805​ {QCryptographicHash::Sha256, QRegularExpression("^[a-f0-9]{64}$")},
anthonyfieroniUnsubmitted
Done

Better get localPath before concurrent function and you should use mutex as well, simultaneously read/write is data race i.e.

const auto itemPath = properties->item().localPath();
std::function<QString(QCryptographicHash::Algorithm)> mapper = [itemPath, this](QCryptographicHash::Algorithm alg) {
    QMutexLocker locker(&d->mutex);
anthonyfieroni: Better get localPath before concurrent function and you should use mutex as well…
2806​ {QCryptographicHash::Sha384, QRegularExpression("^[a-f0-9]{96}$")},
2807​ {QCryptographicHash::Sha512, QRegularExpression("^[a-f0-9]{128}$")},
2808​ {QCryptographicHash::Sha3_224, QRegularExpression("^[a-f0-9]{56}$")},
2809​ {QCryptographicHash::Sha3_256, QRegularExpression("^[a-f0-9]{64}$")},
2810​ {QCryptographicHash::Sha3_384, QRegularExpression("^[a-f0-9]{96}$")},
2811​ {QCryptographicHash::Sha3_512, QRegularExpression("^[a-f0-9]{128}$")}
2812​ };
2853 2813​
2854 if (isSha1(input)) { 2814​ QVector<QCryptographicHash::Algorithm> algorithms;
2855 return QCryptographicHash::Sha1; ​
2856 } ​
2857 2815​
2858 if (isSha256(input)) { 2816​ for (const auto &p : regexes) {
2859 return QCryptographicHash::Sha256; 2817​ if (p.second.match(input).hasMatch()) {
2818​ algorithms.push_back(p.first);
2819​ }
2860 } 2820​ }
2861 2821​
2862 // Md4 used as negative error code. 2822​ return algorithms;
2863 return QCryptographicHash::Md4; ​
2864} 2823​}
2865 2824​
2866void KChecksumsPlugin::setDefaultState() 2825​void KChecksumsPlugin::setDefaultState()
petermajchrakAuthorUnsubmitted
Not Done

If the mutex is locked only around the cacheChecksum function I might as well put in back inside it

petermajchrak: If the mutex is locked only around the cacheChecksum function I might as well put in back…
Context not available.
2928 d->m_ui.feedbackLabel->show(); 2887​ d->m_ui.feedbackLabel->show();
2929} 2888​}
2930 2889​
2931void KChecksumsPlugin::showChecksum(QCryptographicHash::Algorithm algorithm, QLabel *label, QPushButton *copyButton) 2890​void KChecksumsPlugin::showChecksums()
2932{ 2891​{
2892​ for (const auto alg : d->cache.keys()) {
2893​ auto buttons = getButtons(alg);
2894​ showChecksum(alg, buttons.first, buttons.second);
2895​ }
2896​}
2897​
2898​void KChecksumsPlugin::showChecksum(QCryptographicHash::Algorithm algorithm, QPushButton *calculateButton, QPushButton *copyButton)
2899​{
2900​ if (calculateButton == nullptr || copyButton == nullptr) {
2901​ return;
2902​ }
2903​
2904​ auto label = new QLabel(i18nc("@action:button", "Calculating..."), &d->m_widget);
2905​ label->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard);
2906​
anthonyfieroniUnsubmitted
Done

It's a private function, i can't image why it can be call with null pointers :)

anthonyfieroni: It's a private function, i can't image why it can be call with null pointers :)
petermajchrakAuthorUnsubmitted
Done

I am looking up the UI components based on name and I was getting some nullptrs until I found the right way to get them correctly from the layout. Not sure if they are needed but in case something in the layout changes (like a name of a button) it might pass in a nullptr

petermajchrak: I am looking up the UI components based on name and I was getting some nullptrs until I found…
2907​ calculateButton->hide();
2908​ calculateButton->parentWidget()->layout()->replaceWidget(calculateButton, label);
2909​
2910​ // If it is placed in the "Advanced" KCollapsibleGroupBox then expand it
2911​ auto groupBox = dynamic_cast<KCollapsibleGroupBox*>(label->parentWidget());
2912​ if (groupBox) {
2913​ groupBox->expand();
2914​ }
2915​
2933 const QString checksum = cachedChecksum(algorithm); 2916​ const QString checksum = cachedChecksum(algorithm);
2934 2917​
2935 // Checksum in cache, nothing else to do. 2918​ // Checksum in cache, nothing else to do.
Context not available.
2954 futureWatcher->setFuture(future); 2937​ futureWatcher->setFuture(future);
2955} 2938​}
2956 2939​
2957QString KChecksumsPlugin::cachedChecksum(QCryptographicHash::Algorithm algorithm) const 2940​std::pair<QPushButton*, QPushButton*> KChecksumsPlugin::getButtons(QCryptographicHash::Algorithm algorithm)
2958{ 2941​{
2959 switch (algorithm) { 2942​ QString name{QMetaEnum::fromType<QCryptographicHash::Algorithm>().valueToKey(algorithm)};
2960 case QCryptographicHash::Md5: 2943​ // TODO: Remove after QCryptographicHash::Algorithm is renamed from RealSha3 to Sha3
2961 return d->m_md5; 2944​ name.replace("Real", "");
2962 case QCryptographicHash::Sha1: 2945​ auto calculateBtn = d->m_widget.findChild<QPushButton*>("calculate" + name);
2963 return d->m_sha1; 2946​ auto copyBtn = d->m_widget.findChild<QPushButton*>("copy" + name);
2964 case QCryptographicHash::Sha256: 2947​ return std::make_pair(calculateBtn, copyBtn);
2965 return d->m_sha256; 2948​}
2966 default: ​
2967 break; ​
2968 } ​
2969 2949​
2970 return QString(); 2950​QString KChecksumsPlugin::cachedChecksum(QCryptographicHash::Algorithm algorithm) const
2951​{
2952​ return d->cache.value(algorithm, QString{});
2971} 2953​}
2972 2954​
2973void KChecksumsPlugin::cacheChecksum(const QString &checksum, QCryptographicHash::Algorithm algorithm) 2955​void KChecksumsPlugin::cacheChecksum(const QString &checksum, QCryptographicHash::Algorithm algorithm)
2974{ 2956​{
2975 switch (algorithm) { 2957​ QMutexLocker locker(&d->mutex);
2976 case QCryptographicHash::Md5: 2958​ d->cache.insert(algorithm, checksum);
2977 d->m_md5 = checksum; ​
2978 break; ​
2979 case QCryptographicHash::Sha1: ​
2980 d->m_sha1 = checksum; ​
2981 break; ​
2982 case QCryptographicHash::Sha256: ​
2983 d->m_sha256 = checksum; ​
2984 break; ​
2985 default: ​
2986 return; ​
2987 } ​
2988} 2959​}
2989 2960​
2990class KUrlPropsPlugin::KUrlPropsPluginPrivate 2961​class KUrlPropsPlugin::KUrlPropsPluginPrivate
Context not available.
anthonyfieroniUnsubmitted
Done

Mutex is not needed here.

anthonyfieroni: Mutex is not needed here.
petermajchrakAuthorUnsubmitted
Done

This function is called from the QtConcurrent mapper function and therefore can be called concurrently by more threads. QMap::insert is not thread safe for concurrent write operations as far as I know.

petermajchrak: This function is called from the QtConcurrent mapper function and therefore can be called…