Review Request 110672: Allow multiple selection in archiveview

[Raphael Kubo da Costa]

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



part/CMakeLists.txt
<https://git.reviewboard.kde.org/r/110672/#comment45983>

    Why isn't it possible to use `arkpart`?



part/CMakeLists.txt
<https://git.reviewboard.kde.org/r/110672/#comment45968>

    This is a minor nitpick: keeping an extra empty line before the "install files" comment looks better.



part/archivemodel.h
<https://git.reviewboard.kde.org/r/110672/#comment45969>

    Same thing about keeping an extra line. And the coding style is slightly off (can't blame you though, there's some inconsistency above).
    
    Per kdelibs's coding style, the "&" should be next to `files`. By the way, you can use a `QVariantList` here.



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45970>

    The style here is wrong too. Pay attention to the position of the "&" and remove the spaces around "QVariant" (if you don't use a `QVariantList`, that is).



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45971>

    Asserting one thing and then checking for the opposite (and using two different methods that have the same purpose) looks confusing.
    
    I think you can just do something like this:
    
    ```c++
    if (files.isEmpty()) {
        qDebug() << "Got an empty file list.";
        return QLatin1String("/");
    }
    ```



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45982>

    Aren't you basically doing this?
    
    1. Get length of the shortest string in `files`.
    2. Start reducing all strings from 1..shortestLength to '' until you find a substring that's common to all of them.
    
    If I understood what you're trying to do correctly I think your code can be trimmed down.



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45979>

    `rootLength-1` should be `rootLength - 1`.



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45980>

    Style: Missing {}s.



part/archivemodel.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45981>

    `root.truncate()` should be in a separate line.



part/part.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45973>

    You're kind of repurposing `internalRoot` here; it makes more sense to use a new variable instead.



part/part.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45972>

    IIRC `kDebug()` already separates each argument with a space, so "selection root is" also works.



part/test/archivemodeltest.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45978>

    Why not just include QtDate?



part/test/archivemodeltest.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45974>

    Extra empty line.



part/test/archivemodeltest.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45976>

    I'd like to see some test cases with files with different prefixes (e.g. "foo/bar/baz.txt" and "some/other/path.ext"), more single-item lists and tests for "/". You're mostly testing one possible scenario here.



part/test/archivemodeltest.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45977>

    This causes the test to just crash here.
    
    > QFATAL : archiveModelTest::testGetSelectionRoot(test1) ASSERT failure in QCoreApplication: "there should be only one application object", file /s/kde/src/qt4/src/corelib/kernel/qcoreapplication.cpp, line 753



part/test/archivemodeltest.cpp
<https://git.reviewboard.kde.org/r/110672/#comment45975>

    Extra empty line.


As a general comment: you still need to adjust the coding style. Please see the "kdelibs coding style" page in TechBase; there are comments without a period at the end, blocks that should be split with empty lines, empty lines that shouldn't exist and "&"s placed incorrectly.

I've left some comments in the code. I believe we're in the right direction here, thanks for working on this.

- Raphael Kubo da Costa


On Aug. 23, 2014, 11:10 a.m., Alim Gokkaya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/110672/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2014, 11:10 a.m.)
> 
> 
> Review request for KDE Utils, Albert Astals Cid and Raphael Kubo da Costa.
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> This patch enables extracting multiple files using drag&drop by removing the code that singles the selection.
> 
> 
> Diffs
> -----
> 
>   part/CMakeLists.txt 9e38443 
>   part/archivemodel.h 7f8c527 
>   part/archivemodel.cpp 4326268 
>   part/archiveview.cpp 6b9918d 
>   part/part.cpp b4ebcd2 
>   part/test/archivemodeltest.cpp PRE-CREATION 
>   part/test/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/110672/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alim Gokkaya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20140904/fe43854f/attachment-0001.html>