"Join with previous" loses data (with fix)

[Steven Boswell]

I selected "Join with previous" while doing a library-rescan, and it deleted the set-log without moving its tracks into the previous set-log! There was a debug message written to stdout, explaining that the query failed because the database was locked, but it didn't recover gracefully from that, and lost data!

Also, "Join with previous" doesn't preserve the date/time that the track was added to the previous playlist -- instead, the current date/time was used.

Finally, the date/time last played is only displayed as the time in the set-log, instead of the date/time.

The enclosed patch fixes all three issues.

PlaylistDAO's appendTracksToPlaylist(), appendTrackToPlaylist(), and copyPlaylistTracks() have been modified to return a bool, indicating whether the operation succeeded. Also, copyPlaylistTracks() was rewritten to do the playlist-copy with a single SQL query, in addition to fixing the copy of the date/time added.

copyPlaylistTracks() is also used when duplicating playlists, and it seems reasonable to copy the date/time-added there too, instead of setting it to the current date/time.

This patch is against the master branch. It should integrate without trouble into the 1.11 release branch.

[RJ Skerry-Ryan (rryan)]

Thanks Steven! Committed to lp:mixxx/1.11

Minor style issue for future ref:

- if (newPlaylistId != -1) {
+ if (newPlaylistId != -1
+ && m_playlistDao.copyPlaylistTracks(oldPlaylistId, newPlaylistId))
         emit(showTrackModel(m_pPlaylistTableModel));
- }

When an expression is dangling across lines, please keep the logic operator (e.g. the &&) at the end of the line so that as you scan, you naturally see the expression is continued because the binary operator is there without its right operand. Also, single line bodies of if statements should have braces.

The time-only in session playlists is intentional -- Daniel did that when he wrote the feature, I think to allow you to more easily tell how far into your set you played the track. If that was the intention then I think we should be showing the (date - first-track-start-date) as a duration instead. Daniel -- can you comment?

[Daniel Schürmann (daschuer)]

It is fine for me to display the date as well.
For me it is more relevant when the track was played absolute instead of relative.
RJ can you please commit this change as well?

For the style issue, i am fan of this:
if (newPlaylistId != -1 &&
        m_playlistDao.copyPlaylistTracks(oldPlaylistId, newPlaylistId)) {
    emit(showTrackModel(m_pPlaylistTableModel));
}

Double indentation and allays braces.

[RJ Skerry-Ryan (rryan)]

SGTM -- committed.

On Sun, May 12, 2013 at 7:14 PM, Daniel Schürmann <
<email address hidden>> wrote:

> It is fine for me to display the date as well.
> For me it is more relevant when the track was played absolute instead of
> relative.
> RJ can you please commit this change as well?
>
> For the style issue, i am fan of this:
> if (newPlaylistId != -1 &&
> m_playlistDao.copyPlaylistTracks(oldPlaylistId, newPlaylistId)) {
> emit(showTrackModel(m_pPlaylistTableModel));
> }
>
> Double indentation and allays braces.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1179098
>
> Title:
> "Join with previous" loses data (with fix)
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1179098/+subscriptions
>

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7026