Review Request 128283: Add checksums tab to the properties dialog

Fri Jul 1 07:34:35 UTC 2016

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128283/#review96983
-----------------------------------------------------------


src/widgets/checksumswidget.ui (line 26)
<https://git.reviewboard.kde.org/r/128283/#comment65525>

    there two spaces before "are"


src/widgets/checksumswidget.ui (line 48)
<https://git.reviewboard.kde.org/r/128283/#comment65526>

    two spaces after MD5


src/widgets/checksumswidget.ui (line 55)
<https://git.reviewboard.kde.org/r/128283/#comment65527>

    same


src/widgets/kpropertiesdialog.cpp (line 2653)
<https://git.reviewboard.kde.org/r/128283/#comment65528>

    Why the indirection via signal/slots? You could rename slotDefaultState() to setDefaultState() and just call it directly where you currently emit defaultState. Signals are meant for the "outside" (users of the class), but here they are internal.
    Same for the other signals below.
    
    I think direct method calls will improve readability, the control flow will be easier to determine.


src/widgets/kpropertiesdialog.cpp (line 2661)
<https://git.reviewboard.kde.org/r/128283/#comment65529>

    This would be done in setDefaultState(), for instance.
    
    If one day an if() has to be added or something, it will be easier ;)


src/widgets/kpropertiesdialog.cpp (line 2772)
<https://git.reviewboard.kde.org/r/128283/#comment65531>

    This should be an if(), not a tri-state operator returning void.


src/widgets/kpropertiesdialog.cpp (line 2777)
<https://git.reviewboard.kde.org/r/128283/#comment65530>

    the 3rd argument should be "this"


src/widgets/kpropertiesdialog.cpp (line 2799)
<https://git.reviewboard.kde.org/r/128283/#comment65532>

    if ()


src/widgets/kpropertiesdialog.cpp (line 2844)
<https://git.reviewboard.kde.org/r/128283/#comment65533>

    what if the file changes while this dialog is up? (e.g. during a download).
    
    Maybe a change of size or mtime should invalidate the cache?


src/widgets/kpropertiesdialog.cpp (line 2851)
<https://git.reviewboard.kde.org/r/128283/#comment65534>

    add this as 3rg arg (to avoid crashes if the dialog is closed before this finishes)


src/widgets/kpropertiesdialog.cpp (line 2863)
<https://git.reviewboard.kde.org/r/128283/#comment65535>

    This method is called from the secondary thread, so the private class needs a mutex to protect d->m_md5, d->m_sha1 and d->m_sha256
    
    Alternatively, check the cache outside of the method called by QtConcurrent. After all it's wasteful to create a QtConcurrent task just to realize we have no computation to do.


src/widgets/kpropertiesdialog.cpp (line 2896)
<https://git.reviewboard.kde.org/r/128283/#comment65537>

    I would make this a file-static method, to make sure you're not accessing any member variables from the class from a secondary thread (undefined behaviour, without a mutex).
    
    If you remove the cache check and pass the filename as argument, then you don't need "this" anymore, it can be static and race-free.


src/widgets/kpropertiesdialog.cpp (line 2899)
<https://git.reviewboard.kde.org/r/128283/#comment65536>

    double lookup, use a local variable


- David Faure


On June 30, 2016, 8:57 a.m., Elvis Angelaccio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128283/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 8:57 a.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> This patch adds a Checksums tab in the properties dialog, where the user can retrieve and verify the most popular checksum algorithms (md5, sha1 and sha256). 
> 
> To simplify the implementation, the checksums are computed as soon as the user opens the dialog. This can take a while if the file is huge (in particular with sha256), but the computation happens in another thread and in practice this should not be a performance problem.
> 
> The tab is available only for readable local files (no simlinks) and only when there is a single selection.
> 
> Please note that some of the labels in the screenshots are clipped due to a bug in breeze: https://bugs.kde.org/show_bug.cgi?id=364426
> 
> 
> Diffs
> -----
> 
>   src/widgets/CMakeLists.txt f906577 
>   src/widgets/checksumswidget.ui PRE-CREATION 
>   src/widgets/kpropertiesdialog.cpp d0a2faa 
>   src/widgets/kpropertiesdialog_p.h c01554e 
> 
> Diff: https://git.reviewboard.kde.org/r/128283/diff/
> 
> 
> Testing
> -------
> 
> * Check whether the computed values match the values from md5sum, sha1sum, sha256sum.
> * Check whether the line edits get a green background if the computed and expected values match.
> * Check whether the line edits get a red background if the computed and expected values differ.
> 
> 
> File Attachments
> ----------------
> 
> MD5 ready to be shared
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/26/b882fad9-85b0-4250-9743-3549339e6718__Spectacle.l10844.png
> Default dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/27/ad06e136-7ce3-4876-a594-98fbc64f5538__Spectacle.M13222.png
> Mismatch
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/27/99838e45-d9c5-4a14-b26a-9440f0249c4b__Spectacle.V13222.png
> Match
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/27/519fa28f-7c7d-4bb4-bd24-622d18d7f2e2__Spectacle.v13222.png
> Invalid checksum
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/27/a243c830-2dc0-4cbd-95e9-8f1684bc86a4__Spectacle.J13336.png
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160701/49bee21d/attachment-0001.html>