Review Request 107473: Changes in processing playlist files

[Bart Cerneels]

> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.h, lines 71-73
> > <http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line71>
> >
> >     What is the difference between save() and savePlaylist()? Also, if we don't allow constructor w/out url specified, the url arguments here can go away. (which I'd like very much)
> >     
> >     It seems that save() is the public method for other code to use and savePlaylist() is the method for subclasses to implement. If this is the case, make it so:
> >      * make save() non-virtual
> >      * make savePlaylist() protected
> >     
> >     In each case, both need to be documented.

The url argument in the c'tor can go away, except for M3U playlist, which uses the filename as it's name() because it doesn't have that metadata in the "spec".


> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.h, line 94
> > <http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line94>
> >
> >     Thinking about it more, I think it is ridiculous to require calling triggerTrackLoad() manually. Just start the loading job in constructor and ditch triggerTrackLoad() altogether.
> >     
> >     [you may want to postpone working on this for the case other Amarok developers don't agree]

Use case: Browsing playlist treeview with closed playlists. QAbscractItemModel::fetchMore() and so on.

The whole idea is delayed loading right?


> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.h, lines 99-101
> > <http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line99>
> >
> >     Please document these more - including their relationship. Does calling setName() change filename? vice versa? (or perhaps setFileName() is redundant and can be removed (+1 from me)?)

Here your point of removing filename as c'tor argument and as a member becomes even more valid.


> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.cpp, lines 227-249
> > <http://git.reviewboard.kde.org/r/107473/diff/5/?file=112907#file112907line227>
> >
> >     This should probably take some code from Playlists::loadPlaylistFile() so that it supports also remote KUrls.

Do we really want to load remote (ex. http) playlist-files directly? Opens a whole other world of potential break points that need to be handled.


- Bart


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107473/#review27042
-----------------------------------------------------------


On Feb. 7, 2013, 3:57 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2013, 3:57 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/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 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core/playlists/Playlist.h b4e4f40 
>   src/playlist/PlaylistActions.h e8e541b 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   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 0f19e1b 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp f4643ca 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130211/722d699c/attachment-0001.html>