Re: Review Request: Find In Files — Allow depth definition instead of just recursion

[Milian Wolff]

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



plugins/grepview/grepdialog.h
<http://git.reviewboard.kde.org/r/105856/#comment13754>

    please remove trailing whitespaces in your patch (see configure editor -> appearance -> highlight trailing spaces)



plugins/grepview/grepdialog.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13757>

    remove trailing spaces



plugins/grepview/grepfindthread.h
<http://git.reviewboard.kde.org/r/105856/#comment13756>

    if you want to use this, then please also use it in the grepdialog.cpp case where you default the value to -1 == Full.
    
    Also, make this an Enum, and don't forget to adapt the usages of these values (i.e. from Depth::Full to just FULL (yeah, uppercase the values is also a good idea))



plugins/grepview/grepfindthread.h
<http://git.reviewboard.kde.org/r/105856/#comment13767>

    please break long lines



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13755>

    whitespaces (remove the whole line?)



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13766>

    this could and should be always used for anything but depths == Depth::None, i.e.:
    
    if ( depth == Depth::None ) {
      if ( !url...) {
        continue;
      }
    } else if ( !dir.isParentOf(url) ) {
      continue;
    } else if ( depth > 0) {
      ...
    }



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13765>

    please add a comment to this branch, explaining what you are doing here.
    
    took me some time to figure it out



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13759>

    initialize!
    
    match = false;



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13758>

    please rephrase this to if ( folderUrl.equals(dir, ...) ) {
      match = true;
      break;
    }



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13760>

    whitespaces!



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13761>

    on two lines:
    
    if(depth > 0)
      depth--;
    
    or even better use braces if () {\n...\n}
    
    and add an empty line before the dirFiles << ...



plugins/grepview/grepfindthread.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13764>

    please break this and the next over long line, i.e.:
    
    GrepFindFilesThread::...(...,
                             ...)
    : QThread(parent)
    , m_startDirs(...)
    , ...
    ...
    {
      ...
    }



plugins/grepview/grepjob.h
<http://git.reviewboard.kde.org/r/105856/#comment13762>

    whitespaces!



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/105856/#comment13763>

    please break this long line - e.g. after the "m_depthValue," argument


- Milian Wolff


On Aug. 15, 2012, 1:42 p.m., Adrián Chaves Fernández wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105856/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2012, 1:42 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> I got the Find In Files dialog to allow the user to specify a level of depth for the search instead of just whether it should use full depth (recursion) or none (no recursion).
> 
> This is meant to be my first step in the implementation of Find In Files for (current file and) Included Files (https://bugs.kde.org/show_bug.cgi?id=160143). Just in case I give up with the task before I finish the whole of it, I figured out it would be better to at least send you this bit now, since it is already an enhancement over the current behavior.
> 
> 
> Diffs
> -----
> 
>   plugins/grepview/grepdialog.h d4a1db6 
>   plugins/grepview/grepdialog.cpp d1f4bf7 
>   plugins/grepview/grepfindthread.h b24520c 
>   plugins/grepview/grepfindthread.cpp bc39d87 
>   plugins/grepview/grepjob.h c357b77 
>   plugins/grepview/grepjob.cpp 66df1a9 
>   plugins/grepview/grepwidget.ui 94cfb00 
>   plugins/grepview/tests/findreplacetest.cpp ac0687d 
> 
> Diff: http://git.reviewboard.kde.org/r/105856/diff/
> 
> 
> Testing
> -------
> 
> I have tested the feature by looking in “kdevplatform” for ‘All Open’ (as in “All Open Files”, one of the optiona locations for Find In Files). I tried with the following depths: Full (-1), 0, 1, 2. Only the first one and the last one gave me a match, for the file kdevplatform(0)/plugins(1)/grepview(2)/grepdialog.cpp.
> 
> I did not write an automated test for the feature, although I did updated the current tests so the changes do not break it.
> 
> Since the current tests do not actually test for different recursion values (only recursion=true), I didn’t feel like spending time on testing the change (specially since I have never written one in C++/Qt before). But if it were considered a requirement to apply the patch, I would give it a try.
> 
> 
> Thanks,
> 
> Adrián Chaves Fernández
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120816/cac0c48b/attachment.html>