Review Request 107473: Changes in processing playlist files

[Matěj Laitl]

> On March 27, 2013, 12:35 p.m., Matěj Laitl wrote:
> > src/core-impl/playlists/types/file/m3u/M3UPlaylist.h, lines 49-52
> > <http://git.reviewboard.kde.org/r/107473/diff/9-10/?file=120161#file120161line49>
> >
> >     Private? Should be "protected".
> 
> Tatjana Gornak wrote:
>     Sorry, originally I've planned to make PlaylistFile::savePlaylist private instead (but forgot to change it). My point is that if we do not need to invoke virtual function from derived classes, but only customize the behaviour, then function can be private.
>     
>     But ok, protected will work as well.

> My point is that if we do not need to invoke virtual function from derived classes, but only customize the behaviour, then function can be private.

Hmm, this is right, but let's say it's ugly detail of the C++ standard. As a general rule, we try to keep the same access level when subclassing (with a few exceptions that make methods in subclasses more public). I'll soon submit a patch that fixes this line, no need to do it yourself.


- Matěj


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


On March 27, 2013, 3:41 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated March 27, 2013, 3:41 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/CMakeLists.txt c03541f 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae 
>   src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 
>   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 8fe4b51 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce 
>   src/core/playlists/Playlist.h 018d2af 
>   src/core/playlists/Playlist.cpp 36879a1 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistController.cpp e5bde8b 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/SyncedPlaylist.h fce3d14 
>   src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   src/playlistmanager/sql/SqlPlaylist.cpp 88e685b 
>   tests/TestDirectoryLoader.h 0583a9e 
>   tests/TestDirectoryLoader.cpp b98247d 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 
>   tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 
>   tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 
>   tests/core/playlists/CMakeLists.txt e2e0ce4 
>   tests/core/playlists/TestPlaylistObserver.h PRE-CREATION 
>   tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION 
> 
> 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/20130327/db75b500/attachment.html>