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 19, 2012, 1:38 p.m.)


Review request for Amarok, Bart Cerneels, Ralf Engels, and Sven Krohlas.


Changes
-------

Update EngineController: ditch canDecode() commit msg and ChangeLog to reflect the fact it fixes bug 303253.

Bart, I think we should move this from "postponed after 2.6" to "included in 2.6", my responsibility.


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).

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 (updated)
-----

  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 (updated)
-------

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/f08e7f4f/attachment.html>