Review Request 110081: Launching amarok with --cdplay adds CD in playlist

[Matěj Laitl]

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


Thanks for fixing this annoying bug. You've however opened a can of worms and now we'll abuse you to do the cleanup. :-) (sorry)


ChangeLog
<http://git.reviewboard.kde.org/r/110081/#comment23295>

    add: "; patch by Tatjana Gornak." (mind hard like break at 90 chars)



src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/110081/#comment23291>

    I think this would be best solved by audio-destroying helper RAII QObject subclass AudioCdPlaybackStarter.
    
    MainWindow::playAudioCd() would just create it (perhaps if it is not already alive, QWeakPointer is your friend) and stop caring.
    
    AudioCdPlaybackStarter would in its constructor:
     * check whether AudioCd collection is already available; if it is, it would start playback and destroy itself
     * else it would connect to CollectionManager to watch for newly added collections, it would also setup a suicide timer to let's say 30s



src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/110081/#comment23292>

    Oh this is ugly! I think you don't need to know that AudioCd collection uses MemoryCollection at all. Just use its generic queryMaker to get all its tracks. (would require an extra private slot, but hat won't hurt in AudioCdPlaybackStarter)



src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/110081/#comment23293>

    m_waitingForCd must go...



src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/110081/#comment23294>

    bye bye...



src/core-impl/collections/audiocd/AudioCdCollection.cpp
<http://git.reviewboard.kde.org/r/110081/#comment23290>

    Oh, if your at it, it would be best if you removed this extremely ugly hack. I think AudioCdCollection shouldn't know about mainwindow or command line at all.


- Matěj Laitl


On April 19, 2013, 6:44 a.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110081/
> -----------------------------------------------------------
> 
> (Updated April 19, 2013, 6:44 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Main problem was that adding CD to playlist was connected to signal collectionReady, but this signal emits before collection becomes available in CollectionManager::instance()->viewableCollections(). So I changed collectionReady to collectionAdded signal from CollectionManager (since CollectionManager is responsible for adding new collection to a store). 
> 
> My concern is that collectionAdded emits only if some conditions are fulfilled. It seems to me that they are always fulfilled in considered case, but I'm not sure.
> 
> 
> This addresses bug 279188.
>     https://bugs.kde.org/show_bug.cgi?id=279188
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/audiocd/AudioCdCollection.cpp 3dfa7c3 
>   ChangeLog cc8b166 
>   src/MainWindow.cpp 4273d8a 
> 
> Diff: http://git.reviewboard.kde.org/r/110081/diff/
> 
> 
> Testing
> -------
> 
> Launching amarok with --cdplay adds CD in playlist
> 
> 
> Thanks,
> 
> Tatjana Gornak
> 
>

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