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]

> On July 12, 2012, 11:04 p.m., Ralf Engels wrote:
> > tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp, line 77
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72272#file72272line77>
> >
> >     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, ..."
> 
> Matěj Laitl wrote:
>     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.

Oh sorry, I lied. Will reupload the patch with comment re-added.

> If we need to have special code here to ensure a sensible cleanup, might we not run into troubles in Amarok itself?

I don't really know, perhaps is is already called by someone? We never had related crashed of normal Amarok run, only test crashes.


- Matěj


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


On July 13, 2012, 11:53 a.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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).
> 
> 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
> -----
> 
>   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/72b14cb5/attachment-0001.html>