Review Request 107473: Changes in processing playlist files

Matěj Laitl matej at laitl.cz
Tue Feb 19 13:37:38 UTC 2013 


> 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.
> 
> Bart Cerneels wrote:
>     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".

No, I propose the *other* way around, that url is *always* needed in constructors, and *never* needed in save methods. This also solves some potential problems like overwriting other playlists etc.


> On Feb. 9, 2013, 12:55 a.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/PlaylistFile.h, lines 86-93
> > <http://git.reviewboard.kde.org/r/107473/diff/5/?file=112906#file112906line86>
> >
> >     Why are the 2 very similar methods needed? I'd like if they could be factored into just one, perhaps the one with the QTextStream argument. Why are then public and not protected?
> 
> Tatjana Gornak wrote:
>     QTextStream uses a unicode stream buffer, but in case of XSPF playlist we can entrust encoding detection job to QDomDocument

Okay, leave preserve both methods now, we'll see in future.


> 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]
> 
> Bart Cerneels wrote:
>     Use case: Browsing playlist treeview with closed playlists. QAbscractItemModel::fetchMore() and so on.
>     
>     The whole idea is delayed loading right?

Okay okay, keep it for now. We can have another look on this once it is merged.

@Bart: the idea is *asynchronous* (i.e. not blocking the main thread) rather than delayed.


> 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)?)
> 
> Bart Cerneels wrote:
>     Here your point of removing filename as c'tor argument and as a member becomes even more valid.

Ugh, my point was kinda other way around, see above.


> 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.
> 
> Bart Cerneels wrote:
>     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.

We already handle them, just at another place. Now that various implementations of file playlists are united, the remote-loading hack in Playlists::loadPlaylistFile() doesn't need to exist anymore (my understanding is that remote loading in Playlists::loadPlaylistFile() existed just to avoid duplication among individual playlist file implementations).

In other words, my view is: if a method accepts KUrl &url (as opposed to QString &filePath), it should obviously handle remote urls.


- Matěj


-----------------------------------------------------------
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/20130219/d09da856/attachment-0001.html>

More information about the Amarok-devel mailing list