Review Request: EngineController: ditch canDecode(), fix supportedMimeTypes(): make it non-static, thread-safe even on first call. (squached patches, recent on top)

[Bart Cerneels]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105524/#review16125
-----------------------------------------------------------

Ship it!


Go ahead. The RC period will expose any serious problems.

- Bart Cerneels


On July 19, 2012, 1:38 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105524/
> -----------------------------------------------------------
> 
> (Updated July 19, 2012, 1:38 p.m.)
> 
> 
> Review request for Amarok, Bart Cerneels, Ralf Engels, and Sven Krohlas.
> 
> 
> Description
> -------
> 
> 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).
> 
> As a consequence, we show all audio and video files in file browser and
> in other places, even if they wouldn't be playable by the current
> phonon back-end. This is arguably a cleaner approach and at least lets
> users discover where the error is.
> 
> 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/
> 
> BUG: 303253
> FIXED-IN: 2.6
> 
> Revert "Prevent hang in testmetamultitrack."
> 
> This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97.
> 
> Proper fix will follow, see next commits.
> 
> 
> This addresses bug 303253.
>     https://bugs.kde.org/show_bug.cgi?id=303253
> 
> 
> Diffs
> -----
> 
>   ChangeLog 1d4f1e92fb5a033500c0f18d3dca257a89f1a139 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/browsers/filebrowser/FileBrowser.cpp 567ff799df7ef8bfcd93de73ee120bfc5be634b7 
>   src/browsers/filebrowser/FileView.cpp 8eae4b6731ebfcb7310d3d719687989be125de92 
>   src/core-impl/collections/support/CollectionManager.cpp 9085b5a27d9a7ce94d2325d94bec5fce8d126abe 
>   src/core-impl/meta/file/File.h caa7102fd901ccdac7c9f25ee0caeb554a4ee518 
>   src/core-impl/meta/file/File.cpp 231c98521c3d6b1c609abc5799af47b88f048aee 
>   tests/CMakeLists.txt 901c716a600bca63639939f4e62dbd89d9db707f 
>   tests/TestEngineController.h PRE-CREATION 
>   tests/TestEngineController.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105524/diff/
> 
> 
> Testing
> -------
> 
> Works, fixes testm3uplaylist, should fix bug 303253. (waiting for original reporter, I cannot reproduce)
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120719/8423d69e/attachment-0001.html>