<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 />
<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 13, 2012, 11:53 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">Significantly reworked the patch according to Ralf's and Bart's correct comments. The changes to tests we not needed at all as a result.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Summary (updated)</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: ditch canDecode(), fix supportedMimeTypes(): make it non-static, thread-safe even on first call. (squached patches, recent on top)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description (updated)</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: always call Phonon::availableMimeTypes() from main thread
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 when they are
deleted. 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 it (EngineController::supportedMimeTypes()) so that
this fragile code (hopefully) never breaks again.
EngineController: make supportedMimeTypes() non-static
Static methods have no sense in a singleton class. Additionally, it was
very hacky (and impossible in corner-cases) to keep it thread-safe.
Making it non-static will allow us to do some tricks so that the calls
are more robust.
EngineController: null-pointer checking in destructor
EngineController: remove unused and confusing destroy()
EngineController: ditch canDecode()
MetaFile::Track::isTrack() is partial replacement. Existing 3 calls to
canDecode() were in fact all related to MetaFile classes, so move the
method there, simplify it not to query phonon at all (and document it
can return false positives).
Works quite well for me and prevents failures in many tests. This
change is propelled by Bart's and Ralf's legitimate comments on
http://git.reviewboard.kde.org/r/105524/
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> (updated)</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>src/core-impl/meta/file/File.h <span style="color: grey">(caa7102fd901ccdac7c9f25ee0caeb554a4ee518)</span></li>
<li>src/core-impl/meta/file/File.cpp <span style="color: grey">(231c98521c3d6b1c609abc5799af47b88f048aee)</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>
</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>