I extended the KPropertiesDialog with checksums that were requested by users (and wanted by me :D) as specified in bug 377430. I added most of the available hashing algorithms to the dialog.
BUG: 377430
Functionality changes:
Before:
After:
Details
Details
- Reviewers
elvisangelaccio colomar - Group Reviewers
VDG
After:
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Thanks, now I can build it. Please add screenshots so that the VDG team can have a look.
I don't think we can show so much algorithms by default. The current layout was designed to be as simple as possible.
Maybe we can add an "advanced" section (hidden by default), using KCollapsibleGroupBox.
Comment Actions
Please add the following on its own line in the Summary section: "BUG: 377430"
That way it will automatically close the bug once this is in.
Comment Actions
I am new to KDE development and I want to use the KCollapsibleGroupBox but I don't know how. Are there any plugins for Qt Designer for KWidgetsAddons?
Comment Actions
The concept of an "advanced" hashing algorithm doesn't really make sense IMHO. This whole thing is an "advanced" feature.
How about a combobox containing a list of all available hashing algorithms, with a single Calculate button?
Comment Actions
The most common algorithms (sha1, sha256) should be easily reachable, hiding them among a huge list of algorithms would be annoying.
Using a combobox could be a good idea, but I would still put it inside an hidden collapsible box.
Also, I'd ignore MD4 (even MD5 is questionable these days).
Comment Actions
It is kinda questionable as far as what are the most common hash algorithms. I most often use SHA512 for example. I think having only the combobox would be ok for me, but I'm not a UX person. So just let me know if you figure out anything else.
Comment Actions
My vote is for a single combobox with a reasonably common default choice like sha256, but that that remember the last-used choice if you change it. My sense is that most people who actually use this feature (= very very small group of people) already have a favorite hashing algorithm. The first time they use the combobox to choose that one, it will remain the default forever until changed.
Comment Actions
Which repository is it...?
For me it looks fine on the latest screenshots, combobox is less convenient.
Comment Actions
When uploading diffs, and creating the review for the first time one of the fields Phabricator will ask you about is the repository.
I've now set this for the review.
Comment Actions
That is not what this dialog was designed for. The primary use case is to make easy for the average user to check the integrity of downloaded files, and that's why we only added MD5, SHA1 and SHA256.
Comment Actions
I will also need help figuring out what config to use for storing the users last used hash algorithm from the combobox (if we go that route).
Comment Actions
As I mentioned, using only a combobox doesn't work, because the main functionality (= check integrity of downloaded files) is now harder to use.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2657 | Make it static const and initialize it here. | |
| 2681–2682 | Use braces {} even on one line block in all places. | |
| 2757 | 1 space between ) { in all places | |
| 2777–2794 | 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)); ? | |
| 2782 | Are you sure about calling this function in all watchers? | |
Comment Actions
In other hand moslty user check one of following algorithms i.e. when i check a downloaded file i validate sha256, so i selected it in combobox (if needed) then paste expected. In most of time *one* prefered algorithm is used.
Comment Actions
I am still not clear on what to do now. Should I revert it to be a lot of separate buttons (hidden in a collapsible group box) (as @elvisangelaccio said) or should I keep it in a dropdown (ok according to me and @anthonyfieroni)?... I'm going to fix some other stuff in the meantime. I also made this verifyChecksumRecursive function to verify the checksums serially (one after another)(because up to 2 algorithms can be candidates for certain checksum inputs) to not consume too much resources. I'm thinking of doing that in parallel instead. Do you think it's a good idea? Are there any guidelines for these situations?
Comment Actions
Unfortunately many websites only show you the SHA1 or, even worse, only the MD5 of the file. As I said, it doesn't matter that your preferred algo is e.g. SHA256, if the website doesn't actually provide it.
@petermajchrak I vote to revert to the collapsible groupbox layout. We should also ask feedback from the people involved in the original design (@colomar in particular).
Comment Actions
I reverted back to the group box layout and parallelized hash calculations (removed the annoying recursive function).
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2904–2906 | It's a private function, i can't image why it can be call with null pointers :) | |
Comment Actions
! In D8536#172105, @elvisangelaccio wrote:
Unfortunately many websites only show you the SHA1 or, even worse, only the MD5 of the file. As I said, it doesn't matter that your preferred algo is e.g. SHA256, if the website doesn't actually provide it.
@petermajchrak I vote to revert to the collapsible groupbox layout. We should also ask feedback from the people involved in the original design (@colomar in particular).
You are right, but after i saw the screenshots still prefer dropdown, it looks clear and simple to me.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2904–2906 | 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 | |
Comment Actions
@anthonyfieroni, @elvisangelaccio, @colomar any progress? just want to get a status... this diff seems to be taking quite a long time to get reviewed after making changes...
Comment Actions
With that many available algorithms, the combobox solution makes more sense to me as well.
For checking a provided checksum, users don't have to use the combobox, anyway. They just paste in the checksum and get a result automatically.
Comment Actions
I can revert it back to the combobox based layout. Don't we have some UX people that could take a look at this?
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2795–2805 | 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); | |
| 3015 | Mutex is not needed here. | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 3015 | 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. | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2798–2804 | 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. | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2798–2804 | 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. | |
Comment Actions
Fix mutex placement and add show cached checksum values when combobox value is changed
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2798–2804 | 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. | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2798–2804 | 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 | |
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2798–2804 | 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);
} | |
Comment Actions
Won't using the blocking versions block the UI as well?... that's why I chose to use the non blocking versions and just update the UI once the work is finished by the other threads.
| src/widgets/kpropertiesdialog.cpp | ||
|---|---|---|
| 2825 | If the mutex is locked only around the cacheChecksum function I might as well put in back inside it | |
Comment Actions
Is there a way to separate this class to its own file? Or is it some convention in this project to have these huge files?
Comment Actions
I don't want to bother you but even last attempt can have race condition. If slotVerifyChecksums is called while future is not finished read/write cache can occur.
https://paste.kde.org/pdqu2bywp/sccbzb
I'm not using mutex at all. You can always use a signal/slot between threads, if receiver remains the same, cause slot will be called in its thread, synchronously.
Comment Actions
All right, let's go with the combobox.
Before I look into the non-ui code, please update the patch by following the Frameworks coding style: https://community.kde.org/Policies/Kdelibs_Coding_Style
| src/widgets/checksumswidget.ui | ||
|---|---|---|
| 58 | Please don't use a grid layout, keep the current form layout. Using a grid layout results in huge push buttons which is really ugly. | |
| 67 | This label can become huge with SHA3 checksums and it will resize the dialog. Now it's probably the right time to switch to KSqueezedTextLabel. | |
Comment Actions
@ngraham I have in the meantime switched away from KDE, so it would be difficult for me to continue the development...