<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24773">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24773#inline-164025">View Inline</a><span style="color: #4b4d51; font-weight: bold;">trashimpl.cpp:1098</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Find lateest modification date</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">res</span><span class="p">.</span><span class="n">mtime</span> <span style="color: #aa2211">></span> <span class="n">latestModifiedDate</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">typo: latest</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24773#inline-164023">View Inline</a><span style="color: #4b4d51; font-weight: bold;">trashsizecache.cpp:144</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">trashFileInfo</span><span class="p">.</span><span class="n">exists</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span class="n">trashFileInfo</span><span class="p">.</span><span class="n">lastModified</span><span class="p">().</span><span class="n">toMSecsSinceEpoch</span><span class="p">()</span> <span style="color: #aa2211">></span> <span class="n">max_mtime</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">max_mtime</span> <span style="color: #aa2211">=</span> <span class="n">trashFileInfo</span><span class="p">.</span><span class="n">lastModified</span><span class="p">().</span><span class="n">toMSecsSinceEpoch</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This calls lastModified().toMSecsSinceEpoch() twice.</p>
<p style="padding: 0; margin: 8px;">How about a lambda for (at least)</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (lastModTime > max_mtime) {
max_mtime = lastModTime;
}</pre></div>
<p style="padding: 0; margin: 8px;">and then you can call that lambda with lastModified().toMSecsSinceEpoch()?<br />
I was thinking about suggesting such a lambda anyway since that logic exists 4 times in this code.</p>
<p style="padding: 0; margin: 8px;">In fact, checking that the trashinfo file exists is something that should be done in all code paths, no? So there's a lot more code that can be factored out, I would think.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24773#inline-164021">View Inline</a><span style="color: #4b4d51; font-weight: bold;">trashsizecache.cpp:168</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">usableCache</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// orphaned directories</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">const</span> <span class="n">qulonglong</span> <span class="n">size</span> <span style="color: #aa2211">=</span> <span class="n">DiscSpaceUtil</span><span style="color: #aa2211">::</span><span class="n">sizeOfPath</span><span class="p">(</span><span class="n">file</span><span class="p">.</span><span class="n">absoluteFilePath</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">orphaned sounds like "no .trashinfo file pointing to that dir".</p>
<p style="padding: 0; margin: 8px;">But that's not the case here. It's just items that have been added to the trash without being added to the cache file (many implementations do that, the cache is optional).</p>
<p style="padding: 0; margin: 8px;">So the directory is not listed in the cache, or is listed with a too old mtime i.e. it wasn't updated when adding another item into it.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24773#inline-164026">View Inline</a><span style="color: #4b4d51; font-weight: bold;">trashsizecache.cpp:171</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">sum</span> <span style="color: #aa2211">+=</span> <span class="n">size</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">add</span></span><span class="p">(</span><span class="n">file<span class="bright">Id</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">size</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">long</span></span><span class="bright"> </span><span class="n"><span class="bright">mtime</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">QFileInfo</span></span><span class="p">(</span><span class="n">file<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">absolutePath</span></span><span class="bright"></span><span class="p"><span class="bright">()).</span></span><span class="bright"></span><span class="n"><span class="bright">lastModified</span></span><span class="bright"></span><span class="p"><span class="bright">().</span></span><span class="bright"></span><span class="n"><span class="bright">toMSecsSinceEpoch</span></span><span class="bright"></span><span class="p"><span class="bright">()</span></span><span class="bright"> </span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">mtime</span> <span style="color: #aa2211">></span> <span class="n">max_mtime</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not technically correct, this is the mtime of the directory, while there could be much more recent items inside that directory.</p>
<p style="padding: 0; margin: 8px;">But I'm wondering how much effort we want to put into finding the latest mtime, this is starting to sound like a very expensive operation.</p>
<p style="padding: 0; margin: 8px;">So I'm actually find with this known bug, maybe with a comment about how this is a heuristic -- in a fallback for bad implementations.<br />
When kio_trash takes care of the trashing, we should never end up in this code path (right?)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24773">https://phabricator.kde.org/D24773</a></div></div><br /><div><strong>To: </strong>meven, Frameworks, ngraham, elvisangelaccio, dfaure<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>