<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/105524/">http://git.reviewboard.kde.org/r/105524/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 12th, 2012, 11:04 p.m., <b>Ralf Engels</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/105524/diff/3/?file=72272#file72272line77" style="color: black; font-weight: bold; text-decoration: underline;">tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void TestM3UPlaylist::initTestCase()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">70</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ThreadWeaver</span><span class="o">::</span><span class="n">Weaver</span><span class="o">::</span><span class="n">instance</span><span class="p">()</span><span class="o">-></span><span class="n"><span class="hl">finish</span></span><span class="p"><span class="hl">();</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">77</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k"><span class="hl">if</span></span><span class="p"><span class="hl">(</span></span><span class="hl"> </span><span class="o"><span class="hl">!</span></span><span class="n">ThreadWeaver</span><span class="o">::</span><span class="n">Weaver</span><span class="o">::</span><span class="n">instance</span><span class="p">()</span><span class="o">-></span><span class="n"><span class="hl">isIdle</span></span><span class="p"><span class="hl">()</span></span><span class="hl"> </span><span class="p"><span class="hl">)</span></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;">If we need to have special code here to ensure a sensible cleanup, might we not run into troubles in Amarok itself?
Could you add the old comment back?
The "Wait for other jobs, ..."</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;">Changing TestM3UPlaylist is apparently not needed at all in new patch version, altough I don't know whether this fill pass on Jenkins, too. Lets try without and perhaps resurrect this piece of patch.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 12th, 2012, 11:04 p.m., <b>Ralf Engels</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/105524/diff/3/?file=72274#file72274line38" style="color: black; font-weight: bold; text-decoration: underline;">tests/core/collections/support/TestTrackForUrlWorker.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<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">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Amarok</span><span class="o">::</span><span class="n">Components</span><span class="o">::</span><span class="n">setEngineController</span><span class="p">(</span> <span class="k">new</span> <span class="n">EngineController</span><span class="p">()</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;">What a sorry excuse for a singleton we have here.
We should look into that after the release and check if we can't do it better.</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;">Resolved by having CollectionManager::trackForUrl() not calling EngineController at all.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 12th, 2012, 11:04 p.m., <b>Ralf Engels</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/105524/diff/3/?file=72274#file72274line42" style="color: black; font-weight: bold; text-decoration: underline;">tests/core/collections/support/TestTrackForUrlWorker.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qRegisterMetaType</span><span class="o"><</span><span class="n">ThreadWeaver</span><span class="o">::</span><span class="n">Job</span><span class="o">*></span><span class="p">(</span> <span class="s">"ThreadWeaver::Job*"</span> <span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">42</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qRegisterMetaType</span><span class="o"><</span><span class="n">ThreadWeaver</span><span class="o">::</span><span class="n">Job</span><span class="o">*></span><span class="p">(</span> <span class="s">"ThreadWeaver::Job*"</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 these two lines had me confused.
Why do we explicitely need to register the types? Isn't the Amarok library doing it on it's own?</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;">> Why do we explicitely need to register the types? Isn't the Amarok library doing it on it's own?
Yes, but the unit test doesn't reach that code. It is correct, because in Amarok this is shared between many code paths.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 12th, 2012, 11:04 p.m., <b>Ralf Engels</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/105524/diff/3/?file=72279#file72279line48" style="color: black; font-weight: bold; text-decoration: underline;">tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">TestSqlUserPlaylistProvider::dataPath( const QString &relPath )</pre></td>
</tr>
</tbody>
<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">47</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Amarok</span><span class="o">::</span><span class="n">Components</span><span class="o">::</span><span class="n">setEngineController</span><span class="p">(</span> <span class="k">new</span> <span class="n">EngineController</span><span class="p">()</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;">Is having the code in four places in the test cases really better than an additional check in CollectionManager::trackForUrl?
And again, it's just because of the one function call in trackForUrl that implements a function that is not documented and could be surprising for the user in some corner cases.</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;">Resolved by having CollectionManager::trackForUrl() not calling EngineController at all.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On July 12th, 2012, 2:15 p.m., Matěj Laitl 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 Amarok, Ralf Engels and Sven Krohlas.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated July 12, 2012, 2:15 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;">EngineController: use QFileInfo instead of apparently non-reentrant KFileItem
...remote files are not needed here, so QFileInfo does a good job. This
is the cause of last crashes in testm3uplaylist. (I have about 25%
probability of it crashing until this commit)
EngineController: make supportedMimeTypes() thread-safe even on first call
If supportedMimeTypes() is called for the first time in a thread,
Phonon::BackendCapabilities::availableMimeTypes() creates some QObject
subclasses in non-main thread and that causes problems later on, and is
the cause of testm3uplaylist failures. Add signal/slot/QSemaphore
trickery that causes Phonon::BackendCapabilities::availableMimeTypes()
is called from the main thread without performance penalty for 2nd and
next calls.
Test is added for EngineController::supportedMimeTypes() so that this
fragile code (hopefully) never breaks again.
This fixes (perhaps along with the next commit) 2 failing tests (
testm3uplaylist and apparently testplaylistmodels), so the fail count
is down to 4 here.
EngineController: make canDecode() and supportedMimeTypes() non-static
Static methods have no sense in a singleton class. Additionally, it was
very hacky (and impossible in corner-cases) to keep these thread-safe,
see testm3uplaylist failures. Making them non-static will allow us to
do some tricks so that the calls are more robust.
Never-used and confusing destory() method is removed, too.
Care is taken not to break existing tests.
v2: fix even more tests that have been added or fixed while this patch
slept.
Revert "Prevent hang in testmetamultitrack."
This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97.
Proper fix will follow, see next commits.</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;">Works, fixes testm3uplaylist</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>src/EngineController.h <span style="color: grey">(ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3)</span></li>
<li>src/EngineController.cpp <span style="color: grey">(83f0a6ed0a92ae992e1809800cee65d9349dc680)</span></li>
<li>src/browsers/filebrowser/FileBrowser.cpp <span style="color: grey">(567ff799df7ef8bfcd93de73ee120bfc5be634b7)</span></li>
<li>src/browsers/filebrowser/FileView.cpp <span style="color: grey">(8eae4b6731ebfcb7310d3d719687989be125de92)</span></li>
<li>src/core-impl/collections/support/CollectionManager.cpp <span style="color: grey">(9085b5a27d9a7ce94d2325d94bec5fce8d126abe)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(901c716a600bca63639939f4e62dbd89d9db707f)</span></li>
<li>tests/TestEngineController.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/TestEngineController.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp <span style="color: grey">(4bbf29be01aef9043a64075a196ac04a544134cb)</span></li>
<li>tests/core/collections/support/TestTrackForUrlWorker.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core/collections/support/TestTrackForUrlWorker.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core/meta/TestMetaTrack.cpp <span style="color: grey">(0adb7633538533202c50487b1eaff8e4ead150e9)</span></li>
<li>tests/dynamic/TestDynamicModel.h <span style="color: grey">(ce9934cf464172b75b1858f884a64c00cdcf4e24)</span></li>
<li>tests/dynamic/TestDynamicModel.cpp <span style="color: grey">(7387fbcbf28ad838fbf56076553da56d630451af)</span></li>
<li>tests/playlistmanager/file/TestPlaylistFileProvider.cpp <span style="color: grey">(931eefba03e55f4ae2beec089369087958311013)</span></li>
<li>tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp <span style="color: grey">(bb09515d617e8149e7e66e804ff46aa5ddb76045)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105524/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>