> On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.h, line 117 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2895#file2895line117> > > > > Delete stuff rather than commenting it :-) > > Ralf Engels wrote: > It was unused (after my refactoring) and redundant as you can get the > state from the media object. > > Alex Merry wrote: > But that doesn't change my comment - you should delete the code if it's > not needed. It can always be reclaimed from the git history.
You were right. It's deleted. > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.h, line 350 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2895#file2895line350> > > > > But not when it resumes, right? This should be clarified in the docs. Wow. very pedantic. For resuming we have the trackPlaying signal. But I will update the comments anyway. > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.h, line 386 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2895#file2895line386> > > > > Reading the apidocs, I'm completely confused about the difference > > between trackMetadataChanged and currentMetadataChanged, or what other > > changes I would get by also connecting to trackChanged. > > > > Are trackMetadataChanged and currentMetadataChanged identical apart > > from the argument type? If so, this should be explicit in the apidocs. > > > > Most clients will just want to know when the "title of the > > currently-playing track" changes, either because a different track is > > playing or because the title property of the current track has changed > > (edited/stream/whatever). What other use cases are there for connecting to > > a metadata changed signal? Yes it's confusing. But at least we have some comments to it. But I will give more docu. > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.h, line 425 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2895#file2895line425> > > > > The apidocs should be clearer about "userSeek" - when it is false, it > > means that the position change is due to normal playback progression. > > > > Perhaps this should be split into two signals, one for each value of > > userSeek - would this be more efficient, with a reduced number of listeners > > when the track is just progressing normally? > > > > Perhaps seeked( qint64 position ) and trackPositionChanged( qint64 > > position ), with the former being connected to the latter? Not many objects are connecting to this signal and I have not seen that as a big problem. None of the connected objects cared about userSeek so it might as well be removed. > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.h, line 437 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2895#file2895line437> > > > > This doesn't seem to be emitted or used anywhere. this was a feature request but I really forgot to emit it. > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.cpp, line 555 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2896#file2896line555> > > > > m_currentAlbum = 0, surely? upps > On 2010-10-31 20:14:08, Alex Merry wrote: > > src/EngineController.cpp, line 1040 > > <http://git.reviewboard.kde.org/r/100120/diff/1/?file=2896#file2896line1040> > > > > Shouldn't m_currentAlbum be set to 0 as well? Very right. And not just this one case was wrong. - Ralf ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100120/#review239 ----------------------------------------------------------- On 2010-10-31 15:53:10, Ralf Engels wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100120/ > ----------------------------------------------------------- > > (Updated 2010-10-31 15:53:10) > > > Review request for Amarok. > > > Summary > ------- > > Remove EngineObserver and the observer pattern. > Instead we have several new signals and functions in EngineController. > Also the EngineController is now a MetaObserver for the current track and > exporting the relevant signals. > > This patch improves the tread safety and reduces the complexity of the code > by a lot. > Over 600 lines of codes could be removed while at the same time fixing the > image update problems and several small problems where classes listened for > the album meta changed but forgot that the album could change when a user > changed the track. > > > Diffs > ----- > > playground/src/context/applets/coverbling/CoverBlingApplet.h a3ff113 > playground/src/context/applets/coverbling/CoverBlingApplet.cpp a2a50e7 > playground/src/context/applets/video/Video.h 1d52f0b > playground/src/context/applets/video/Video.cpp 8471497 > src/ActionClasses.h aa67ec7 > src/ActionClasses.cpp 61e8af8 > src/App.cpp e6006b9 > src/EngineController.h aae4503 > src/EngineController.cpp 65b3f2e > src/KNotificationBackend.h 02cc9d8 > src/KNotificationBackend.cpp b98391c > src/MainWindow.h 4593d47 > src/MainWindow.cpp 9102558 > src/TrayIcon.h d1ffd43 > src/TrayIcon.cpp 47f56bf > src/amarokurls/AmarokUrlHandler.cpp 1d8cd46 > src/context/ContextView.h a3f36fb > src/context/ContextView.cpp ccfe4f3 > src/context/applets/photos/PhotosApplet.h 4cac9ea > src/context/applets/photos/PhotosApplet.cpp 0b47da9 > src/context/applets/similarartists/SimilarArtistsApplet.h 3a2d7ea > src/context/applets/similarartists/SimilarArtistsApplet.cpp 0cf2c76 > src/context/applets/videoclip/VideoclipApplet.h a0704c6 > src/context/applets/videoclip/VideoclipApplet.cpp 789173e > src/context/engines/current/CurrentEngine.h 0369065 > src/context/engines/current/CurrentEngine.cpp 2c32f75 > src/context/engines/labels/LabelsEngine.h 626a030 > src/context/engines/labels/LabelsEngine.cpp 65278b4 > src/context/engines/lyrics/LyricsEngine.h 771bf6f > src/context/engines/lyrics/LyricsEngine.cpp 527cd0d > src/context/engines/similarartists/SimilarArtistsEngine.h fea9bf5 > src/context/engines/similarartists/SimilarArtistsEngine.cpp 97bc109 > src/context/engines/upcomingevents/UpcomingEventsEngine.h c57b004 > src/context/engines/upcomingevents/UpcomingEventsEngine.cpp 66131fd > src/context/engines/wikipedia/WikipediaEngine.h 5d53082 > src/context/engines/wikipedia/WikipediaEngine.cpp 4c65799 > src/core-impl/meta/stream/Stream_p.h cd217bf > src/core-impl/meta/timecode/TimecodeObserver.h b727196 > src/core-impl/meta/timecode/TimecodeObserver.cpp dea809d > src/core/CMakeLists.txt 5863ca1 > src/core/engine/EngineObserver.h 5a93062 > src/core/engine/EngineObserver.cpp 7d5728b > src/dbus/mpris1/PlayerHandler.h 8f90f27 > src/dbus/mpris1/PlayerHandler.cpp 0afeb59 > src/dbus/mpris2/Mpris2DBusHandler.h a767c79 > src/dbus/mpris2/Mpris2DBusHandler.cpp 59afbf3 > src/dialogs/ScriptManager.h 6104dcf > src/dialogs/ScriptManager.cpp e98f45f > src/dynamic/BiasSolver.cpp bf8e3c8 > src/dynamic/biases/EchoNest.h 9a6ecc7 > src/dynamic/biases/EchoNest.cpp 16f26f4 > src/mac/GrowlInterface.h 3bb35d2 > src/mac/GrowlInterface.cpp 862d019 > src/playlist/PlaylistActions.h 035ff77 > src/playlist/PlaylistActions.cpp 2358b29 > src/playlist/PlaylistController.cpp d38c9d6 > src/playlist/view/listview/PrettyItemDelegate.cpp ab19ccc > src/scriptengine/AmarokEngineScript.h 55e275c > src/scriptengine/AmarokEngineScript.cpp 15f7b6e > src/services/lastfm/ScrobblerAdapter.h ef7a7c1 > src/services/lastfm/ScrobblerAdapter.cpp 1b4da4a > src/services/lastfm/biases/LastFmBias.h 6ba571d > src/services/lastfm/biases/LastFmBias.cpp 4093104 > src/statemanagement/DefaultApplicationController.cpp 8205aab > src/statusbar/StatusBar.h b785714 > src/statusbar/StatusBar.cpp 48df821 > src/toolbar/CurrentTrackToolbar.h c8293f3 > src/toolbar/CurrentTrackToolbar.cpp 22408ea > src/toolbar/MainToolbar.h fc29ba2 > src/toolbar/MainToolbar.cpp abeedc6 > src/toolbar/SlimToolbar.h dcfaf22 > src/toolbar/SlimToolbar.cpp e69df7c > src/toolbar/VolumePopupButton.h a0f5d56 > src/toolbar/VolumePopupButton.cpp fb5fcb1 > src/widgets/Osd.h b80b37e > src/widgets/Osd.cpp af4b24b > src/widgets/ProgressWidget.h 3fba490 > src/widgets/ProgressWidget.cpp 211cea7 > src/widgets/VolumeWidget.h 910e9f1 > src/widgets/VolumeWidget.cpp e1d01f1 > tests/core-impl/meta/multi/CMakeLists.txt 8a51249 > tests/playlist/CMakeLists.txt 01703fe > > Diff: http://git.reviewboard.kde.org/r/100120/diff > > > Testing > ------- > > > Thanks, > > Ralf > >
_______________________________________________ Amarok-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/amarok-devel
