-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Library: keep hidden tracks in history #4725
Conversation
src/library/dao/playlistdao.cpp
Outdated
@@ -757,6 +757,10 @@ void PlaylistDAO::removeTracksFromPlaylists(const QList<TrackId>& trackIds) { | |||
it != playlistsTrackIsInCopy.constEnd(); ++it) { | |||
if (it.key() == trackId) { | |||
const auto playlistId = it.value(); | |||
// keep tracks in history playlusts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is called removeTracksFromPlaylists() without any exceptions. Now it contains some special logic in the implementation that is not mentioned anywhere. Functions should do what they are promising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if it was removeTracksFromAllPlaylists() I'd fully agree.
What do you propose?
We could simply name it removeTracksFromPlaylistsExceptHistory()
since we agreed to keep all tracks in history playlists, no matter if tracks are purged or hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @uklotzde
ac2fdfd
to
79da2d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hijacking playlists for storing play logs was probably not the optimal decision. Now we have to cope with that.
LGTM
Remove hidden tracks from playlists but keep them in the history.
https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Hidden.20tracks.20in.20playlists.20.2F.20crates.20.2Fhistory
Interestingly the counterpart to not remove hidden tracks when setting the History table model is already in place:
mixxx/src/library/setlogfeature.cpp
Lines 22 to 33 in 5b6e2e0
mixxx/src/library/playlisttablemodel.cpp
Lines 113 to 128 in 5b6e2e0