> 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. > > Matěj Laitl wrote: > > 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)
> 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. Experience has taught me otherwise. It's faster to implement and then refactor for code reuse and consistency then to assume possible refactoring beforehand. With limited time/problem-space understanding/experience/etc smaller functional patches are better than attempting large refactorings. It's better for the review process as well. > On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote: > > src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp, line 218 > > <http://git.reviewboard.kde.org/r/107473/diff/1/?file=96233#file96233line218> > > > > Here is the real reason why lazy loading is needed. Reading > > playlist-file contents from disk is too fast to bother, but getting the > > right Track objects from CollectionManager::trackForUrl() is problematic. > > It should either block until the track is ready (depends in TrackProviders) > > or using proxy loading like it is now. The proxy is temporary and gets > > initialized with metadata we know from the contents of this playlist file. > > Myriam Schweingruber wrote: > That is a totally wrong assumption, don't you remember that we introduced > it because reading playlists on start-up is a PITA? Please don't assume that > all users have their collection and playlists on an SSD. > Playlists should never be read unless you actually load it in the play > queue, not a second earlier. We reduced start up time by over a minute, and > some users reported start up delays of several minutes because playlist were > read on start. See also https://bugs.kde.org/show_bug.cgi?id=245654, you > introduced that yourself :) The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do. - Bart ----------------------------------------------------------- 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
