> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote: > > Tatjana, thanks for your continued effort on this review. :-) There seem to > > be a couple of problematic places that may cause regressions, please take a > > look at them so this can be merged. (which I hope can be done rather soon, > > this patch contains incredible amount of clean-ups) > > Matěj Laitl wrote: > To answer your question > > PlaylistFile::triggerTrackLoad implements async loading. > > In some cases triggerTrackLoad uses as if it is synchronous > > (e.g. DirectoryLoader, PlaylistController, PlaylistBrowserModel) > > > > I see two solutions of this problem: > > 1) For now perform async loading in trigerTackLoad only on request. > > Thereby loading will be async only if it was requested: > > playlist->makeLoadingAsync(); > > In the future support of async loading can be added everywhere > > and code for sync loading support can be removed. > > 2) Add async loading support to each place where triggerTrackLoad > > is called. > > > > I chose first solution. If someone thinks that second solution > > is better, then I need some additional time to figure out how to > > implement async loading support in remaining cases. > > In long term, the second solution is better. However as you've already > implemented stop-gap code for the 1st solution (and it isn't scary at all), > keep the first one in this review request. You can always convert more users > of file playlists to being async in future review requests and as you've > said, once all are async, support for sync loading could be dropped. > > BTW: are you a student eligible for Google Summer of Code?
Yes, I am eligible to participate in GSoC. > On March 5, 2013, 12:11 a.m., Matěj Laitl wrote: > > src/playlist/PlaylistRestorer.cpp, lines 114-119 > > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116591#file116591line114> > > > > processTracks() can get called multiple times, but this code doesn't > > seem to be ready for it. Why? This code executes only once after all tracks were processed ('return' above prevents it to execute if not all entities in m_tracks were processed). - Tatjana ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review28555 ----------------------------------------------------------- On March 2, 2013, 6:06 p.m., Tatjana Gornak wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107473/ > ----------------------------------------------------------- > > (Updated March 2, 2013, 6:06 p.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 92650c3 > src/DirectoryLoader.h e53a30d > src/DirectoryLoader.cpp bd7fa34 > src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 > 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.cpp 2dcc0cd > src/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION > src/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION > 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/playlists/Playlist.h b4e4f40 > src/core/playlists/Playlist.cpp 2d57e6b > src/playlist/PlaylistActions.h e8e541b > src/playlist/PlaylistActions.cpp 00bf13a > src/playlist/PlaylistRestorer.h PRE-CREATION > src/playlist/PlaylistRestorer.cpp PRE-CREATION > src/playlistmanager/PlaylistManager.h cbf65b0 > src/playlistmanager/PlaylistManager.cpp 89c754b > src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 > tests/TestDirectoryLoader.h 0583a9e > tests/TestDirectoryLoader.cpp b98247d > tests/core-impl/meta/multi/TestMetaMultiTrack.cpp 8e9e47d > tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 > tests/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION > tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION > 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 > > 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
