> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > I think you started with the wrong assumption that lazy loading of the > > contents of the playlist file is required. > > We have some use cases for PlaylistFile: > > 1) Make playlists found in the collection folders available through the > > PlaylistBrowser using PlaylistFileProvider. > > 2) Restore the session with contents of the playback queue using > > XSPFPlaylist saved to $KDEHOME/share/apps/amarok/current.xspf. > > 3) Wherever we need to load or save a playlist file, locally or remote. > > Like the "Add to playlist..." or "Export playlist..." actions in the menu. > > > > Like you can tell from my inline comments, the support for remote loading > > needs to be removed from these classes. Fetching the content can be done > > outside of these classes using load(). The name and other playlist metadata > > is either in the content (XSPF) or derived from the filename (M3U, PLS). > > New PlaylistFile derived classes should use whatever the format has > > available, preferably from the content. > > > > Since you want to add a new Playlist class, what is you use case for it. Is > > there any need to load remote content in use case 1) or 2) (might be > > specific about the format)? > > > > I propose you don't refactor PlaylistFile just yet and implement > > ASXPlaylist so it works as needed for the above use cases. After that we'll > > have a better understanding for refactoring. I'm sure clean-up and refactor > > will be needed.
> I think you started with the wrong assumption that lazy loading of the > contents of the playlist file is required. I don't agree, Myriam doesn't agree. Remember we have a bug "Amarok starts up several minutes when playlists on NFS..." > Like you can tell from my inline comments, the support for remote loading > needs to be removed from these classes. Why? I would do it differently, (No need to call to playlistcontroller) but why not have it? > Fetching the content can be done outside of these classes using load(). Yes, but no reason to do that from my PoV. > The name and other playlist metadata is either in the content (XSPF) or > derived from the filename (M3U, PLS). New PlaylistFile derived classes should > use whatever the format has available, preferably from the content. Yes. > Since you want to add a new Playlist class, what is you use case for it. Is > there any need to load remote content in use case 1) or 2) (might be specific > about the format)? This was just a helper class. If you read my comments, I already proposed doing it in PlaylistFile directly and I'm working on a patch that will enable it. > I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist > so it works as needed for the above use cases. After that we'll have a better > understanding for refactoring. I'm sure clean-up and refactor will be needed. Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial. Tatjana, plese wait for me to finish my preparatory patch. (today hopefully) - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22621 ----------------------------------------------------------- On Nov. 26, 2012, 12:10 p.m., Tatjana Gornak wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107473/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2012, 12:10 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. > > > Diffs > ----- > > src/CMakeLists.txt b70147d > src/core-impl/playlists/types/file/LazyPlaylistFile.h PRE-CREATION > src/core-impl/playlists/types/file/LazyPlaylistFile.cpp PRE-CREATION > src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 > src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 > 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 3ba154a > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 > src/core/playlists/Playlist.h 5cf11f2 > src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 > tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 > tests/core-impl/playlists/types/file/TestLazyPlaylistFile.h PRE-CREATION > tests/core-impl/playlists/types/file/TestLazyPlaylistFile.cpp PRE-CREATION > tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b > > 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
