D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. @ngraham I have in the meantime switched away from KDE, so it would be difficult for me to continue the development... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: kde-framework

D8536: Add more hashing algorithms to KPropertiesDialog

ngraham added a comment. Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks. @petermajchrak, are you going to finish this, or should someone else take over? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio,

D8536: Add more hashing algorithms to KPropertiesDialog

broulik added inline comments. INLINE COMMENTS > checksumswidget.ui:62 > > - MD5: > + Value > Perhaps "Result:"? > checksumswidget.ui:76 > + > + Algorithm > + Shouldn't that have a colon at the end? REPOSITORY R241 KIO REVISION DE

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio requested changes to this revision. elvisangelaccio added a comment. This revision now requires changes to proceed. 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://communit

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added a comment. 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 signa

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, a

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:2825 > +QMutexLocker locker(&d->mutex); > +cacheChecksum(checksum, alg); > +} If the mutex is locked only around the cacheChecksum function I might as well put in back inside it REPOSIT

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To:

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 24402. petermajchrak added a comment. Screen out the cached algorithms beforehand REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=24401&id=24402 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FI

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added a comment. Even more future watcher will not needed anymore, if you use blocked mapper. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham,

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > petermajchrak wrote in 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 Basically you need computation only one time when

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > anthonyfieroni wrote in 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 he

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > petermajchrak wrote in 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 syn

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 24401. petermajchrak added a comment. Fix mutex placement and add show cached checksum values when combobox value is changed REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=24390&id=24401 REVISION DETAIL ht

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > anthonyfieroni wrote in 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 he

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:2798-2804 > +const QString cache = cachedChecksum(alg); > +if (!cache.isEmpty()) { > +return cache; > +} > > -void KChecksumsPlugin::slotShowSha256() > -{ > -auto label = new QL

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kpropertiesdialog.cpp:3015 > Mutex is not needed here. 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 conc

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 24390. petermajchrak added a comment. Capture item path by value REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=24389&id=24390 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FILES src/widgets

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:2795-2805 > +std::function mapper = > [this](QCryptographicHash::Algorithm alg) { > +const QString cache = cachedChecksum(alg); > +if (!cache.isEmpty()) { > +return cache; > +} >

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 24389. petermajchrak added a comment. Format algorithm values in combobox. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=24381&id=24389 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FILES s

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

ngraham accepted this revision as: VDG. ngraham added a comment. looks good to me visually. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio,

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 24381. petermajchrak added a comment. Reverted back to the combobox based layout REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=23772&id=24381 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FIL

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. Ou, ok. Sorry, I didn't know that. Perfect, so combobox it is then. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisange

D8536: Add more hashing algorithms to KPropertiesDialog

colomar added a comment. In https://phabricator.kde.org/D8536#181775, @petermajchrak wrote: > I can revert it back to the combobox based layout. Don't we have some UX people that could take a look at this? Yes, me :) Which is what I did, and commented with my recommendation. REPO

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. I can revert it back to the combobox based layout. Don't we have some UX people that could take a look at this? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfier

D8536: Add more hashing algorithms to KPropertiesDialog

colomar added a comment. 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. REPOSITORY R241 KIO REVI

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. @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... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: peterma

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 23772. petermajchrak added a comment. Small code tweaks, nothing that changes functionality REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=22957&id=23772 REVISION DETAIL https://phabricator.kde.org/D8536 A

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kpropertiesdialog.cpp:2904-2906 > It's a private function, i can't image why it can be call with null pointers > :) I am looking up the UI components based on name and I was getting some nullptrs until I found the r

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added a comment. > ! In https://phabricator.kde.org/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 d

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:2904-2906 > +if (calculateButton == nullptr || copyButton == nullptr) { > +return; > +} It's a private function, i can't image why it can be call with null pointers :) REPOSITORY R241 KIO REVISION D

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak marked 4 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 22957. petermajchrak added a comment. I reverted back to the group box layout and parallelized hash calculations (removed the annoying recursive function). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=21535

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio added reviewers: VDG, colomar. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio, #vdg, colomar Cc: colomar, anthonyfieroni, bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio added a subscriber: colomar. elvisangelaccio added a comment. In https://phabricator.kde.org/D8536#165899, @anthonyfieroni wrote: > In https://phabricator.kde.org/D8536#165865, @elvisangelaccio wrote: > > > As I mentioned, using //only// a combobox doesn't work, because

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added a comment. In https://phabricator.kde.org/D8536#165865, @elvisangelaccio wrote: > As I mentioned, using //only// a combobox doesn't work, because the main functionality (= check integrity of downloaded files) is now harder to use. In other hand moslty user che

D8536: Add more hashing algorithms to KPropertiesDialog

anthonyfieroni added inline comments. INLINE COMMENTS > kpropertiesdialog.cpp:2657 > +class KChecksumAlgorithmListModel: public QAbstractListModel { > +QVector algorithms; > + Make it static const and initialize it here. > kpropertiesdialog.cpp:2681-2682 > +QVariant data(const QModelInd

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio requested changes to this revision. elvisangelaccio added a comment. This revision now requires changes to proceed. As I mentioned, using //only// a combobox doesn't work, because the main functionality (= check integrity of downloaded files) is now harder to use. REPOSITORY

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: bcooksley, alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 21535. petermajchrak added a comment. Convert to only combobox based layout REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=21495&id=21535 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FILES

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: bcook

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. A possible combobox layout F5454950: Screenshot_20171028_225633.png REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: bcooksley, alexeymin, ngraham, elvisan

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio added a comment. In https://phabricator.kde.org/D8536#161276, @ngraham wrote: > 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 us

D8536: Add more hashing algorithms to KPropertiesDialog

bcooksley added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, el

D8536: Add more hashing algorithms to KPropertiesDialog

bcooksley set the repository for this revision to R241 KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. KIO repository REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

alexeymin added a comment. Which repository it is...? For me it looks fine on the latest screenshots, combobox is less convenient. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: alexeymin, ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

ngraham added a comment. 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

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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. REVISION DETAIL

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio added a comment. In https://phabricator.kde.org/D8536#161255, @ngraham wrote: > 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

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. I'm going to give that a try. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

ngraham added a comment. 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? REVISION DETAIL https://phabri

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 21495. petermajchrak added a comment. Place advanced algorithms into KCollapsibleGroupBox CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=21480&id=21495 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FILES src/widge

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. 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? REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: ngraham, elvisangel

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: ngraham, elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

ngraham added a comment. 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. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: ngraham, elvisangelaccio, #fram

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added a comment. I agree that the layout is not optimal. I'm going to try to optimize it now. REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak, elvisangelaccio Cc: elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio requested changes to this revision. elvisangelaccio added a comment. This revision now requires changes to proceed. Thanks, now I can build it. Please add screenshots so that the https://phabricator.kde.org/tag/vdg/ team can have a look. I don't think we can show so much a

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak updated this revision to Diff 21480. petermajchrak added a comment. Remove C++17 features CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8536?vs=21478&id=21480 REVISION DETAIL https://phabricator.kde.org/D8536 AFFECTED FILES src/widgets/checksumswidget.ui src/wi

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak added inline comments. INLINE COMMENTS > elvisangelaccio wrote in CMakeLists.txt:6 > We can't use this in frameworks (and the patch doesn't build for me), see > https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11 Ok. I was kinda hoping this

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak Cc: elvisangelaccio, #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

elvisangelaccio added inline comments. INLINE COMMENTS > CMakeLists.txt:6 > > +set(CMAKE_CXX_STANDARD 17) > + We can't use this in frameworks (and the patch doesn't build for me), see https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11 REPOSITORY R2

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8536 To: petermajchrak Cc: #frameworks

D8536: Add more hashing algorithms to KPropertiesDialog

petermajchrak created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY I added most of the hashing algorithms to the dialog. I still need to figure out a better way to lay out the GUI but the code should b