<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<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/D23875">View Revision</a></tr></table><br /><div><div><p>Sorry it took me a long time to review this. The commit didn't fully satisfy my need to understand the underlying problem, so I had to find the time to debug the issue (thanks for the unittest, it really helps with that).</p>
<p>AFAICS here's what happens:</p>
<ol class="remarkup-list">
<li class="remarkup-list-item">We create folder1 and immediately move the view into it</li>
<li class="remarkup-list-item">So by the time <tt style="background: #ebebeb; font-size: 13px;">itemsAddedInDirectory()</tt> (the DBus notification) is called for the parent directory, there is no KDirLister showing that directory anymore. Therefore this directory isn't listed again, it's just marked as dirty in the cache, for the next time the user will go into it (see <tt style="background: #ebebeb; font-size: 13px;">checkUpdate</tt> where it says "not in use, marked dirty"). That's the usual optimization to avoid re-listing directories that are not shown anymore.</li>
<li class="remarkup-list-item">Therefore the parent directory doesn't actually have a child item for folder1</li>
<li class="remarkup-list-item">We then process the KDirWatch notification (<tt style="background: #ebebeb; font-size: 13px;">processPendingUpdates</tt>) for the parent directory, which updates the mtime of that directory item. OK.</li>
<li class="remarkup-list-item">Steps 1-3 happen again the same way for <tt style="background: #ebebeb; font-size: 13px;">folder1/folder2</tt></li>
<li class="remarkup-list-item">Then, just like step 4, we process the KDirWatch notification for <tt style="background: #ebebeb; font-size: 13px;">folder1</tt>, but it can't find that item in the parent directory due to step 2 above. ASSERT.</li>
</ol>
<p>The heart of the issue is that <tt style="background: #ebebeb; font-size: 13px;">findByUrl</tt> first looks into "child items of the parent dir" then, for subdirs, falls back to the "." of the subdir (so it actually works for <tt style="background: #ebebeb; font-size: 13px;">folder1</tt>).<br />
On the other hand, <tt style="background: #ebebeb; font-size: 13px;">reinsert</tt> only supports child items, not "." items.<br />
This design is certainly quite tricky because for subdirs, the same URL is normally represented those two different items (except in this unusual scenario which skipped creating a child item in the parent directory...).</p>
<p>OK, so how to fix this. A KDirWatch notification for a directory is about updating the mtime and/or permissions of that directory. But that's already done in <tt style="background: #ebebeb; font-size: 13px;">handleFileDirty</tt>, as shown by <tt style="background: #ebebeb; font-size: 13px;">bin/kdirlistertest testDirPermissionChange</tt>. A permission change for an item we're not showing (folder1, while in folder2) also works (handleFileDirty ends up in checkUpdate which marks it as dirty; tested manually).<br />
So... overall we don't actually need to do a <tt style="background: #ebebeb; font-size: 13px;">processPendingUpdates</tt> for a KDirWatch notification for a directory in cache. It was added in commit <a href="https://phabricator.kde.org/R446:9c1e5d56cdfb844f49a41010d307882939971da1" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">9c1e5d56cdfb8</a> but <tt style="background: #ebebeb; font-size: 13px;">testDirPermissionChange</tt> actually passes without it (at least nowadays). In addition, adding pending updates in <tt style="background: #ebebeb; font-size: 13px;">updateDirectory</tt> called by <tt style="background: #ebebeb; font-size: 13px;">processPendingUpdates</tt> seems very unclean to me (cyclic layering).<br />
So my suggested fix is to actually remove some code :-)</p>
<p>I'll post a modified patch with my suggested changes.</p>
<p>Thanks for pushing me to debug this, it's .... not trivial.</p></div></div><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/D23875#inline-135577">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfilewidgettest.cpp:516</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">//created and entered, kdirlister would hit an assert (in reinsert()), bug 408801</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QUrl</span> <span class="n">url</span> <span style="color: #aa2211">=</span> <span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">QDir</span><span style="color: #aa2211">::</span><span class="n">tempPath</span><span class="p">()).</span><span class="n">adjusted</span><span class="p">(</span><span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">StripTrailingSlash</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">dir</span> <span style="color: #aa2211">=</span> <span class="n">url</span><span class="p">.</span><span class="n">toLocalFile</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This test fails when being run twice in a row (without the fix), or if /tmp/folder1 already exists for some unrelated reason.</p>
<p style="padding: 0; margin: 8px;">I suggest using QTemporaryDir instead of QDir::tempPath, so that the deletion of the QTemporaryDir cleans up in all cases (success or failure).</p>
<p style="padding: 0; margin: 8px;">I did it locally to debug this, here it is:</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);">QTemporaryDir tempDir;
QVERIFY(tempDir.isValid());
const QString dir = tempDir.path();
const QUrl url = QUrl::fromLocalFile(dir);</pre></div>
<p style="padding: 0; margin: 8px;">(and then you can remove the cleanup code at the end of the method)</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/D23875#inline-135582">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfilewidgettest.cpp:522</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 class="n">fw</span><span class="p">.</span><span class="n">show</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">fw</span><span class="p">.</span><span class="n">activateWindow</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">show() and activateWindow() are not needed for this unittest to demonstrate the failure.</p>
<p style="padding: 0; margin: 8px;">I'm trying to reduce the number of stuff popping up when running ctest, since that's rather annoying if I'm doing something else at the same time :-)</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/D23875#inline-135580">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfilewidgettest.cpp:533</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">// check that KFileWidget didn't crash</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVERIFY</span><span class="p">(</span><span class="n">QTest</span><span style="color: #aa2211">::</span><span class="n">qWaitForWindowActive</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">fw</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not needed, I get the assert even without this line.</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/D23875">https://phabricator.kde.org/D23875</a></div></div><br /><div><strong>To: </strong>ahmadsamir, Frameworks, dfaure<br /><strong>Cc: </strong>dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>