[prev in list] [next in list] [prev in thread] [next in thread]
List: kdevelop-devel
Subject: =?utf-8?q?Re=3A_Review_Request=3A_Find_In_Files_=E2=80=94_Allow_depth_def?= =?utf-8?q?inition_instea
From: "Andreas Pakulat" <apaku () gmx ! de>
Date: 2012-08-06 15:36:56
Message-ID: 20120806153656.7525.85612 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105856/#review16978
-----------------------------------------------------------
I think the change in the .ui file is much too big. I assume you merely replaced the \
existing checkbox with a spinbox, then the changes in the .ui file should be much \
smaller and localized to that area. In particular I don't see what exactly changed in \
it and what was just moved around. Can you try to either remove the non-changes via \
git or manually edit the original .ui file to change the widget type?
Regarding the test, I think it would be good to add one, but that'll unfortunately \
require a bit more than just adding some new testdata since as far as I can see \
there's no test for the flag either right now. And the replace-test which does create \
files does not look like being able to create subdirs etc. which is needed for a \
recursive test.
- Andreas Pakulat
On Aug. 4, 2012, 6:18 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. 4, 2012, 6:18 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
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105856/">http://git.reviewboard.kde.org/r/105856/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think the change in \
the .ui file is much too big. I assume you merely replaced the existing checkbox with \
a spinbox, then the changes in the .ui file should be much smaller and localized to \
that area. In particular I don't see what exactly changed in it and what was just \
moved around. Can you try to either remove the non-changes via git or manually edit \
the original .ui file to change the widget type?
Regarding the test, I think it would be good to add one, but that'll \
unfortunately require a bit more than just adding some new testdata since as far as I \
can see there's no test for the flag either right now. And the replace-test which \
does create files does not look like being able to create subdirs etc. which is \
needed for a recursive test.</pre> <br />
<p>- Andreas</p>
<br />
<p>On August 4th, 2012, 6:18 p.m., Adrián Chaves Fernández wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Adrián Chaves Fernández.</div>
<p style="color: grey;"><i>Updated Aug. 4, 2012, 6:18 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plugins/grepview/grepdialog.h <span style="color: grey">(d4a1db6)</span></li>
<li>plugins/grepview/grepdialog.cpp <span style="color: grey">(d1f4bf7)</span></li>
<li>plugins/grepview/grepfindthread.h <span style="color: \
grey">(b24520c)</span></li>
<li>plugins/grepview/grepfindthread.cpp <span style="color: \
grey">(bc39d87)</span></li>
<li>plugins/grepview/grepjob.h <span style="color: grey">(c357b77)</span></li>
<li>plugins/grepview/grepjob.cpp <span style="color: grey">(66df1a9)</span></li>
<li>plugins/grepview/grepwidget.ui <span style="color: grey">(94cfb00)</span></li>
<li>plugins/grepview/tests/findreplacetest.cpp <span style="color: \
grey">(ac0687d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105856/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
KDevelop-devel mailing list
KDevelop-devel@kdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic