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

[Matěj Laitl]

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

(Updated July 13, 2012, 11:53 a.m.)


Review request for Amarok, Ralf Engels and Sven Krohlas.


Changes
-------

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.


Summary (updated)
-----------------

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


Description (updated)
-------

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.


Diffs (updated)
-----

  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 
  tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 4bbf29be01aef9043a64075a196ac04a544134cb 

Diff: http://git.reviewboard.kde.org/r/105524/diff/


Testing
-------

Works, fixes testm3uplaylist


Thanks,

Matěj Laitl

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120713/7674abeb/attachment.html>