<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/112173/">http://git.reviewboard.kde.org/r/112173/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2013, 1:29 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">QFileInfo is more portable. Why not just write || info.isSymlink() in the first if?</pre>
</blockquote>
</blockquote>
<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 know QFileInfo is more portable. I use it exclusively whenever I can, but the logic it uses to calculate the size of a symlink will not work for this use case ; so I fail to see how simply adding "|| info.isSymlink()" to the first if statement solves this problem. That is why I said see the documentation for QFileInfo. Simply put info.size() returns the size of the actual file the symlink points to instead of the size of the symlink itself. You do not want that in this case because it will trigger an incorrect error message.
There is a very simple test for this. Create a symlink to a file that is larger than the size of your trashcan. Make sure your trashcan is empty. Now delete the symlink by clicking on Move to Trash in Dolphin and Konqueror and see what you get.
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 20th, 2013, 1:29 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/112173/diff/1/?file=183601#file183601line43" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/trash/discspaceutil.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">43</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">bool</span> <span class="n">isDir</span><span class="p">(</span><span class="kt">mode_t</span> <span class="n">mode</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">And if you really want to use stat, then this is a bit convoluted. No need to write a perfect isDir(), you're only using it in an "else" branch, i.e. you know that isFile() returned false already.
IMHO just else if (S_ISDIR(buff.st_mode)) would be simpler and easier to read.
(and folding isFile, too)
But anyway, QFileInfo can do the job just fine IMHO.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I will simplify the patch and only handle the case of a symlink by itself and leave everything else alone. Perhaps that would make the problem as well as the fix more clearer.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On August 20th, 2013, 1:22 p.m. UTC, Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Runtime, David Faure and Tobias Koenig.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Aug. 20, 2013, 1:22 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;">The attached patch fixes a bug where deleting a symlink to a very large file causes a "trash has reached its limit error. This happens because the code that is used to determine the amount of space available in the trashcan uses QFileInfo::size to determine the size of a file. This will not work because QFileInfo::size returns the size of the actual file the symlink points to and not the size of the symlink itself. See the documentation for QFileInfo.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=253776">253776</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kioslave/trash/discspaceutil.cpp <span style="color: grey">(8e76ece)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112173/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>