-
-
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
fix/History: remove obsolete placeholder playlists #12494
Conversation
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.
Thank you for taking care.
const auto pls = m_playlistDao.getPlaylists(PlaylistDAO::PLHT_UNKNOWN); | ||
QStringList plsToDelete; | ||
for (const auto& pl : pls) { | ||
if (pl.second.startsWith(placeholderName)) { |
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.
auto above and second here is not well readable.
Not your fault. If you have the mood to improve it go ahead.
Else we can ignore it in this bufix PR
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.
Yeah, auto was simply faster to type.
I'll change it.
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.
LGTM Thank you.
I just noticed I made a mistake in 60198d5 (I had ~300
historyPlaceholder#
playlists in my database):the YEAR placeholders are not removed in the destructor because 1) the type is
PlaylistDAO::PLHT_UNKNOWN
which is not covered bydeleteAllUnlockedPlaylistsWithFewerTracks()
, and 2) if it was, they'd be skipped because they are locked.Note: this affects 2.4-beta users only, so we may remove this at some point (2.5?). Otoh the cleanup almost no time after the first execution.