Review Request 116890: Patch for "Bug 279861 - When packing multiple selected files - use containing folder name "

[Raphael Kubo da Costa]

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


Thank you very much for the patch, sorry for taking so long to review. I have added a few inline comments for minor stylistic issues you should watch out for, but I'd like to comment on the overall approach here.

Since the logic for getting a file name is getting more complex, it makes sense to move it all to a separate method, which also makes it easier to test.

It also makes more sense to separate the case where we have only 1 file from the case where we have more than 1. In other words, instead of

    QString base = QFileInfo(...);
    // Logic for 1 file.

    if (have more than 1 file) {
        // Further touch base yadda yadda.
    }

we can have something like

    if (have 1 file) {
        // Easy case, just use the existing code.
    } else {
        // Implement logic to get the directory name.
    }

Another issue I'd like to raise is that doing all that index-based string manipulation shouldn't really be necessary: at this point you have a list of absolute paths, so just use the KUrl/QFileInfo/etc API to get the directory part of the paths.

Finally, you also need to think of the corner cases:
- What if you are compressing files in /, such as /foo and /baz?
- What if you are compressing files in different directories? Remember you can just call "ark --add" on the terminal to make it do the same thing as the context menu, and you can then pass /foo/bar.txt and /baz/zoink.jpg to it.

We could think of different responses for each of those cases, but for a first patch we can safely just switch back to the existing behavior, and only use the directory name in case we are compressing multiple files/directories that have the same parent directory and the resulting archive name would be valid.



kerfuffle/addtoarchive.cpp
<https://git.reviewboard.kde.org/r/116890/#comment39171>

    Extra empty line.



kerfuffle/addtoarchive.cpp
<https://git.reviewboard.kde.org/r/116890/#comment39172>

    Trailing white space.



kerfuffle/addtoarchive.cpp
<https://git.reviewboard.kde.org/r/116890/#comment39173>

    Please start the comment with a capital letter and end it with a period.



kerfuffle/addtoarchive.cpp
<https://git.reviewboard.kde.org/r/116890/#comment39174>

    "> 1" makes the check easier to understand.



kerfuffle/addtoarchive.cpp
<https://git.reviewboard.kde.org/r/116890/#comment39175>

    The variable name should be folderName, not foldername.


- Raphael Kubo da Costa


On March 19, 2014, 1:47 a.m., Florian Schunk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116890/
> -----------------------------------------------------------
> 
> (Updated March 19, 2014, 1:47 a.m.)
> 
> 
> Review request for KDE Utils.
> 
> 
> Bugs: 279861
>     http://bugs.kde.org/show_bug.cgi?id=279861
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> When in a folder i select multiple files to pack [Context Menu -> Compress -> As ZIP Archive] Ark automatically names the archive by first selected file. WinRAR uses in such cases name of the containing folder.
> 
> 
> Diffs
> -----
> 
>   kerfuffle/addtoarchive.cpp d13e095 
> 
> Diff: https://git.reviewboard.kde.org/r/116890/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Florian Schunk
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20140421/9e2d5b49/attachment.html>