Review Request 107473: Changes in processing playlist files

Wed Mar 6 14:26:04 UTC 2013

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/core/playlists/Playlist.h, lines 201-207
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116586#file116586line201>
> >
> >     This is too specific to warrant 2 new methods and a class variable. Please add "bool asynchronous" optional argument to triggerTrackLoad(), defaulting to true. (and pass it as an argument to playlist loading job too)
> 
> Tatjana Gornak wrote:
>     This 2 methods together with sync loading in PlaylistFile::triggerTrackLoad are temporary solution and should be removed in some point in a future. 
>     
>     Optional argument in triggerTrackLoad will affect all inherited from Playlist classes, so it will be harder to remove this code. If it is ok, then I'll add this optional argument to triggerTrackLoad.

Okay, leave it for now. On the other hand, it could be more cleaner to default to m_async = true; and have makeLoadingSync(); This way it would be easier to find out calling sites that need to be ported to async loading and would have a bonus that you could write "KDE_DEPRECATED void makeLoadingSync() { m_async = false; }" to trigger compiler warnings on call sites. But I'll leave this up yo you.

In eiter case please add BIG FAT WARNING that this is just stop-gap measure until all callers are ported to async API.

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/DirectoryLoader.cpp, lines 168-169
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116572#file116572line168>
> >
> >     Here you would have to deal with both async-loading and sync-loading playlists (you don't want to add assumption that all file playlists load asyncly), I suggest the following:
> >     
> >     (...)
> >     playlist->triggerTrackLoad(); // playlist track loading is on demand.
> >     if( playlist->trackCount() >= 0 )
> >         loadingFinished( playlist ); // fool ourselves because nobody else would trigger this
> >     else
> >         subscribeTo( playlist ); // let loadingFinished() be called by the playlist
> >     
> >     I also think you can use playlist.data() instead of the ::staticCast() and let compiler find PlaylistPtr constructor.
> 
> Tatjana Gornak wrote:
>     Hm, I think I do not understand your point.
>     
>     Current code works for both types of loading, because DirectoryLoader is observer and it requires only
>     notification that loading is finished (and this notification should be triggered irrespective of loading type). 
>     
>     And it seems to me, that in code above there is a chance that we subscribe to playlist after loading is finished.

> Current code works for both types of loading, because DirectoryLoader is observer and it requires only
> notification that loading is finished (and this notification should be triggered irrespective of loading type).

If "this notification should be triggered irrespective of loading type" is true, then your code is correct. Discussion whether this assumption holds below:

a) if we limit ourselves to PlaylistFiles (which is okay as the code explicitly uses PlaylistFilePtr), this is nearly true with a couple of exceptions:
 1. if someone calls triggerTrackLoad() twice, he might expect that tracksLoaded() observer method is triggered twice, too. I suggest PlaylistFile::triggerTrackLoad() is modified to call tracksLoaded() before returning in case m_trackLoaded is true. This is not a problem of this call site, but an API design problem for places that take Playlist pointer and don't know whether triggerTrackLoad() is already called.
 2. PlaylistFileLoaderJob::run() has multiple (failure) return points that don't call m_playlist->notifyObserversTracksLoaded(). This will cause current DirectoryLoader code to hang infinitely. I'd be much safer if it were triggered even in error cases. I suggest creating PlaylistFileLoaderJob::slotDone() private slot and connecting it to its done() signal in its constructor and calling notifyObserversTracksLoaded() in its slotDone() signal.

b) while this piece of code would work with just above changes, I suggest that we enforce the same behaviour for all Playlists. I suggest we add notifyObserversTracksLoaded() to default implementation of triggerTrackLoad() (and document it so) so that most other playlists will behave well and tweaking other implementations of triggerTrackLoad().

In short, I'd like if every call to triggerTrackLoad() would trigger tracksLoaded() notification, sooner or later. This way it will make playlist handling much less error prone in corner cases. [OTOH, triggering tracksLoaded() every time even when already may be suboptimal especially when there are multiple observers of the playlist; what is your opinion on that matter?]

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/playlist/PlaylistRestorer.cpp, lines 114-119
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116591#file116591line114>
> >
> >     processTracks() can get called multiple times, but this code doesn't seem to be ready for it.
> 
> Tatjana Gornak wrote:
>     Why? This code executes only once after all tracks were processed ('return' above prevents it to execute if not all entities in m_tracks were processed). 
>     
>

I see, I misunderstood the code, sorry for confusion. (if we come up with something easier to understand in future, it would be certainly beneficial; on the other hand, the whole loading of sub-playlists in PlaylistRestorer seems to be little used and candidate for removal in future)

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/playlist/PlaylistRestorer.h, line 56
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116590#file116590line56>
> >
> >     Class-wise this is redundant with m_tracks and should be avoided. Just create local iterator in processTracks()
> 
> Tatjana Gornak wrote:
>     I can't see how local iterator will do the same job. 
>     
>     Consider example: we want load following playlist [track, playlist, track]. After playlist loading is complete, we invoke Playlist::Restorer::processTracks. On a second position of m_tracks it finds playlist instead of track, invokes loading of new playlist and stops track processing. And after new loading is finished we want to insert new tracks on second position. So we use m_position in order to track position where we should insert newly loaded tracks.

You're right, I misunderstood the code, see below.

- Matěj

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

On March 2, 2013, 6:06 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated March 2, 2013, 6:06 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 92650c3 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 
>   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.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION 
>   src/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION 
>   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/playlists/Playlist.h b4e4f40 
>   src/core/playlists/Playlist.cpp 2d57e6b 
>   src/playlist/PlaylistActions.h e8e541b 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   tests/TestDirectoryLoader.h 0583a9e 
>   tests/TestDirectoryLoader.cpp b98247d 
>   tests/core-impl/meta/multi/TestMetaMultiTrack.cpp 8e9e47d 
>   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 b25d059 
>   tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 
> 
> 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/20130306/6c887d8c/attachment-0001.html>