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
