> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote: > > src/DirectoryLoader.cpp, lines 168-169 > > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116572#file116572line168> > > > > Here you would have to deal with both async-loading and sync-loading > > playlists (you don't want to add assumption that all file playlists load > > asyncly), I suggest the following: > > > > (...) > > playlist->triggerTrackLoad(); // playlist track loading is on demand. > > if( playlist->trackCount() >= 0 ) > > loadingFinished( playlist ); // fool ourselves because nobody else > > would trigger this > > else > > subscribeTo( playlist ); // let loadingFinished() be called by the > > playlist > > > > I also think you can use playlist.data() instead of the ::staticCast() > > and let compiler find PlaylistPtr constructor. > > Tatjana Gornak wrote: > Hm, I think I do not understand your point. > > Current code works for both types of loading, because DirectoryLoader is > observer and it requires only > notification that loading is finished (and this notification should be > triggered irrespective of loading type). > > And it seems to me, that in code above there is a chance that we > subscribe to playlist after loading is finished. > > Matěj Laitl wrote: > > Current code works for both types of loading, because DirectoryLoader > is observer and it requires only > > notification that loading is finished (and this notification should be > triggered irrespective of loading type). > > If "this notification should be triggered irrespective of loading type" > is true, then your code is correct. Discussion whether this assumption holds > below: > > a) if we limit ourselves to PlaylistFiles (which is okay as the code > explicitly uses PlaylistFilePtr), this is nearly true with a couple of > exceptions: > 1. if someone calls triggerTrackLoad() twice, he might expect that > tracksLoaded() observer method is triggered twice, too. I suggest > PlaylistFile::triggerTrackLoad() is modified to call tracksLoaded() before > returning in case m_trackLoaded is true. This is not a problem of this call > site, but an API design problem for places that take Playlist pointer and > don't know whether triggerTrackLoad() is already called. > 2. PlaylistFileLoaderJob::run() has multiple (failure) return points > that don't call m_playlist->notifyObserversTracksLoaded(). This will cause > current DirectoryLoader code to hang infinitely. I'd be much safer if it were > triggered even in error cases. I suggest creating > PlaylistFileLoaderJob::slotDone() private slot and connecting it to its > done() signal in its constructor and calling notifyObserversTracksLoaded() in > its slotDone() signal. > > b) while this piece of code would work with just above changes, I suggest > that we enforce the same behaviour for all Playlists. I suggest we add > notifyObserversTracksLoaded() to default implementation of triggerTrackLoad() > (and document it so) so that most other playlists will behave well and > tweaking other implementations of triggerTrackLoad(). > > In short, I'd like if every call to triggerTrackLoad() would trigger > tracksLoaded() notification, sooner or later. This way it will make playlist > handling much less error prone in corner cases. [OTOH, triggering > tracksLoaded() every time even when already may be suboptimal especially when > there are multiple observers of the playlist; what is your opinion on that > matter?]
I like this solution, despite its sub-optimality, because handling different corner case more likely will lead to an error in the future. - Tatjana ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review28555 ----------------------------------------------------------- On March 11, 2013, 1:57 a.m., Tatjana Gornak wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107473/ > ----------------------------------------------------------- > > (Updated March 11, 2013, 1:57 a.m.) > > > Review request for Amarok. > > > Description > ------- > > > I've started my changes with an implementation of a ASXPlaylist class, > due this work I've found that implementation of different playlist file > types has different logic, as result I end up with a rewriting > playlist's implementations. > > As example of difference: > -- Constructor M3UPlaylist::M3UPlaylist( const KUrl &url ) sets a url, but > does not load playlist, therefore loading of playlist are > posponed till data from playlist are actualy needed (lazy loading) > On the other hand constructor XSPFPlaylist::XSPFPlaylist( const > KUrl &url, bool autoAppend ) loads playlist. > > The main idea of proposed changes is to create a unify code for > processing playlist files: > -- lazy loading through LazyLoadPlaylist > -- common functionality was moved to PlaylistFile. > > It is worth to be noticed that this patch changes structure of > playlist's classes, bit does not changes their behavior, even if this behavior > is inconsistent in some cases. > > In following patch-requests I plan to submit ASX Playlist parser and > organize playlists processing in more consistent way. > > > This addresses bug 291934. > https://bugs.kde.org/show_bug.cgi?id=291934 > > > Diffs > ----- > > src/CMakeLists.txt c03541f > src/DirectoryLoader.h e53a30d > src/DirectoryLoader.cpp bd7fa34 > src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d > src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b > src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 > src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae > src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb > src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 > src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION > src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION > src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 > src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd > src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 > src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 > src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 > src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 8fe4b51 > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 > src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce > src/core/playlists/Playlist.h 018d2af > src/core/playlists/Playlist.cpp 36879a1 > src/playlist/PlaylistActions.h 75b71fd > src/playlist/PlaylistActions.cpp 00bf13a > src/playlist/PlaylistController.cpp e5bde8b > src/playlist/PlaylistRestorer.h PRE-CREATION > src/playlist/PlaylistRestorer.cpp PRE-CREATION > src/playlistmanager/PlaylistManager.h cbf65b0 > src/playlistmanager/PlaylistManager.cpp 89c754b > src/playlistmanager/SyncedPlaylist.h fce3d14 > src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 > src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 > src/playlistmanager/sql/SqlPlaylist.cpp 88e685b > tests/TestDirectoryLoader.h 0583a9e > tests/TestDirectoryLoader.cpp b98247d > tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 > tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 > tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 > tests/core/playlists/CMakeLists.txt e2e0ce4 > tests/core/playlists/TestPlaylistObserver.h PRE-CREATION > tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/107473/diff/ > > > Testing > ------- > > 1) All unit-tests were passed. > 2) For all playlists I've also checked loading and > saving through gui. > > > Thanks, > > Tatjana Gornak > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
