> On Nov. 26, 2012, 6:16 p.m., Matěj Laitl wrote: > > Hi, thanks for your work! > > > > First, I must acknowledge that our current file playlist code is a big mess > > and it certainly needs cleanup and code deduplication, your work is > > therefore greatly appreciated. I have reviewed just one file when it came > > to my mind: instead of introducing LazyPlaylistFile, the lazy-loading logic > > should be incorporated directly to PlaylistFile! There's no reason to have > > playlist files without lazy loading. > > > > Could you please rework the patch eliminating LazyPlaylistFile? Also please > > pay attention to code style (e.g. spaces around parameters etc.) and to > > document methods along the way. (you seem to add methods to PlaylistFile. > > Please keep the public interface minimal usable - the setName() / > > setFilename() / setUrl() looks really scary - what does what? Does the file > > get renamed?) I understand this is mainly existing mess, not your new code. > > :-| > > Matěj Laitl wrote: > Also thanks for writing test-case along with your patch! This is > appreciated. > > Tatjana Gornak wrote: > Hi, > > initially I've considered implementation of lazy-loading in > playlistfiles, but there is a problem with constant methods. For example: > virtual method Playlist::name() const. In case of XSPFPlaylist it calls > method XSPFPlaylist::title(), but to get title you need to load playlist, > thereore title() can not be a constant method. > > Matěj Laitl wrote: > I see. I've submitted https://git.reviewboard.kde.org/r/107484/ to ease > your work. With it, I propose the following: > > * PlaylistFile subclasses return cached name (initially just name part > of the filename/url) in name(), without triggering anything. (also applies to > description) > * PlaylistFile subclasses return already loaded tracks in tracks(), > without triggering anything > * only thing that triggers file loading is triggerTrackLoad(), where you > start a background job (ThreadWeaver::Job) to load the tracks. If the > playlists has metadata in file, you update it and call > notifyObserversMetadataChanged() > * PlaylistManager::downloadPlaylist() is moved as a helper method to > FilePlaylist and incorporated to the backgroundJob that performs the playlist > loading.
So you propose not to maintain the current behavior, but just go with it and refactor the playlists? - Edward Hades ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review22586 ----------------------------------------------------------- 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
