Review Request: karchive plugin extract all fix

[Raphael Kubo da Costa]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102960/#review9203
-----------------------------------------------------------


Besides the other comments, my commit c6bc845 to master should help you catch the missing QLatin1{Char,String} calls in the patch.


plugins/karchiveplugin/karchiveplugin.h
<http://git.reviewboard.kde.org/r/102960/#comment7599>

    Coding style: please follow the Qt/kdelibs style and use
    
      enum {
          SomeEntry,
          AnotherEntry
      };



plugins/karchiveplugin/karchiveplugin.h
<http://git.reviewboard.kde.org/r/102960/#comment7600>

    Coding style: Foo *bar, not Foo* bar.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7601>

    myList.isEmpty()



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7610>

    autoSkip?



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7608>

    Keeping track of whether the a given directory has already been created and only calling mkpath if it does not exist would be a nice improvement.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7607>

    It'd be good to return an error message here.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7609>

    I'm in favor of checking for the opposite to avoid having a large indented block below:
    
      if (autoSkipSelected) {
          continue;
      }



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7605>

    No need for the comment.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7602>

    This could be removed.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7603>

    Ditto.



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7604>

    Is this possible?



plugins/karchiveplugin/karchiveplugin.cpp
<http://git.reviewboard.kde.org/r/102960/#comment7606>

    Nitpick: "fileName" would be a better name.


- Raphael Kubo da Costa


On Nov. 24, 2011, 7:32 p.m., Theofilos Intzoglou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102960/
> -----------------------------------------------------------
> 
> (Updated Nov. 24, 2011, 7:32 p.m.)
> 
> 
> Review request for KDE Utils and Raphael Kubo da Costa.
> 
> 
> Description
> -------
> 
> * If no item was selected from the archive, nothing was extracted! Now it extracts everything.
> * Check if an entry is a directory and append '/' if necessary
> 
> 
> Diffs
> -----
> 
>   plugins/karchiveplugin/karchiveplugin.h 1db6e07 
>   plugins/karchiveplugin/karchiveplugin.cpp 6543874 
> 
> Diff: http://git.reviewboard.kde.org/r/102960/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Theofilos Intzoglou
> 
>

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