Review Request: Multiple EngineController and related fixes, incl. fix for release_blocker bug 299890 (squashed commits, recent on top)

[Bart Cerneels]

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

Ship it!


I see no issues. Didn't test, but I assume you did extensively anyway.

Now go and roll 2.6 RC1 tarball or it will never happen!

- Bart Cerneels


On July 24, 2012, 4:07 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105610/
> -----------------------------------------------------------
> 
> (Updated July 24, 2012, 4:07 p.m.)
> 
> 
> Review request for Amarok and Bart Cerneels.
> 
> 
> Description
> -------
> 
> EngineController: don't do serious work in slotAboutToFinish()
> 
> ...because slotAboutToFinish() may be called twice (or not at all) per
> track by some Phonon backends (hi, vlc) - increase play count rather in
> slotNewTrackPlaying() or in slotFinished(). This also needs to change
> how m_currentTrack is handled, because slotNewTrackPlaying() needs to
> have the old one in m_currentTrack.
> 
> Also, PlaylistActions::requestNextTrack() is changed to be a read-only
> method that shouldn't change playlist state especially when there is no
> next track. PlaylistActions::reflectPlaybackFinished() is introduced to
> do the thing and is called from EngineController::slotFinished(), which
> is a much better place for it than slotAboutToFinish().
> 
> Reporters of CCed bugs, please re-test your bug with this commit
> applied, it is possible it has been resolved by this patch.
> 
> BUG: 299890
> CCBUG: 268892
> CCBUG: 277197
> CCBUG: 302652
> CCBUG: 303580
> CCBUG: 302240
> FIXED-IN: 2.6
> 
> MetaStream: match track by QUrl, not by QString
> 
> Fixes a bug where MetaStram::Track didn't update tags here because
> pretty url was used in signal, but it compared against encoded url
> (the one with %20 instead of spaces).
> 
> MetaStream: big clean-up, implement play statistics methods
> 
>  * remove many unused and useless methods such as setTitle() and
>    friends
>  * additionally take genre, comment, track number from Engine
>    Controller's signal
>  * implement rating, score, first & last play and play count. These are
>    rather debugging tool for EngineController, but they also give you
>    nice info on how many songs have played in the stream
>  * no need to reimplement observer pattern, use MetaBase implementation
>  * implement length, EngineController will emit length info soon
> 
> EngineController: introduce trackFinishedPlaying() signal
> 
> Used do call track->finishedPlaying() now, to be used in Last.fm
> scrobbling when it is emitted for streams, too.
> 
> EngineController: reduce code duplication among play() and stop()
> 
> 
> EngineController: rework slotMetaDataChanged(), nearly no functional change
> 
>  * get data from Phonon to meta QVariantMap more intelligently
>  * m_lastTrack and trackChanged had effect only in debug message, ditch
>    them
>  * note that m_currentTrack may be inaccurate there
>  * rename isMetadataSpam() to isInRecentMetaDataHistory() to describe
>    the functionality better
>  * less debug() spam, specifically be rather quiet for duplicate
>    signals from phonon
>  * Phonon doesn't provide track length, but it does provide track
>    description that we save as "comments" now.
>  * trackData() method was unused and likely meant for
>    slotMetaDataChanged(); ditch it
> 
> 
> This addresses bug 299890.
>     https://bugs.kde.org/show_bug.cgi?id=299890
> 
> 
> Diffs
> -----
> 
>   ChangeLog 1d4f1e92fb5a033500c0f18d3dca257a89f1a139 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/core-impl/meta/stream/Stream.h 2b0a5824eae49345807eef94a465e133996624a1 
>   src/core-impl/meta/stream/Stream.cpp b0fcfad5808b9ae428cb6612bec1737b3af82d3b 
>   src/core-impl/meta/stream/Stream_p.h 58b715f27cb43d014d5837f1afec9d60cb71cc48 
>   src/playlist/PlaylistActions.h 4f27658f48fa7af65c1fbc26d5c184c3f6070f78 
>   src/playlist/PlaylistActions.cpp 1be1d5af6313eb0058c7c65ba7304fa2bd1c00a1 
> 
> Diff: http://git.reviewboard.kde.org/r/105610/diff/
> 
> 
> Testing
> -------
> 
> Works for me, fixes mentioned bug and more smaller glitches.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

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