Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix playlist import of external library features #1877

Merged
merged 9 commits into from
Nov 10, 2018
Merged

Fix playlist import of external library features #1877

merged 9 commits into from
Nov 10, 2018

Conversation

daschuer
Copy link
Member

This fixes the import feature of banshee playlists.

I have added a lost select() call and privatize the temporary table for banshee data.

@uklotzde uklotzde added this to the 2.1.6 milestone Oct 31, 2018
daschuer
daschuer commented Oct 31, 2018
View reviewed changes
src/library/baseexternallibraryfeature.cpp
// use the data because models with nested playlists need to use the
// full path/name of the playlist.
*pPlaylist = m_lastRightClickedIndex.data(Qt::UserRole).toString();
*pPlaylist = m_lastRightClickedIndex.data().toString();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now Idea when this was broken ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add at least a debug assertion before dereferencing pPlaylist:
DEBUG_ASSERT(pPlaylist);

@daschuer daschuer changed the title Fix playlist import of the Banshee feature Fix playlist import of external library features Oct 31, 2018
@daschuer
Copy link
Member Author

fixes https://bugs.launchpad.net/mixxx/+bug/1799932

@uklotzde
Copy link
Contributor

uklotzde commented Nov 1, 2018

Adding tracks from an external to an internal playlist works by drag&drop, but not when using the context menu -> missing/wrong tracks + warnings:

Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 11 , 19 ) in model BansheePlaylistModel(0x3d65c40)
Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 7 , 0 ) in model BansheePlaylistModel(0x3d65c40)
Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 8 , 3 ) in model BansheePlaylistModel(0x3d65c40)
Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 9 , 19 ) in model BansheePlaylistModel(0x3d65c40)
Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 8 , 0 ) in model BansheePlaylistModel(0x3d65c40)
Warning [Main]: QAbstractItemModel::endInsertRows:  Invalid index ( 8 , 3 ) in model BansheePlaylistModel(0x3d65c40)

Currently only tested for Banshee.

I also noticed that tracks in Banshee playlists are not numbered consecutively after tracks have been removed. But that doesn't seem to matter,

@daschuer
Copy link
Member Author

daschuer commented Nov 1, 2018

OK, now the right click track context menu works as well.

@daschuer
Copy link
Member Author

daschuer commented Nov 1, 2018

https://bugs.launchpad.net/mixxx/+bug/1800335 fixed

uklotzde
uklotzde reviewed Nov 2, 2018
View reviewed changes
src/library/banshee/bansheeplaylistmodel.cpp
@@ -327,6 +348,15 @@ TrackPointer BansheePlaylistModel::getTrack(const QModelIndex& index) const {
return pTrack;
}

TrackId BansheePlaylistModel::getTrackId(const QModelIndex& index) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is identical with the default implementation in BaseExternalPlaylistModel. Obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BansheePlaylistModel does not inherit BaseExternalPlaylistModel.
We fetch the Banshee database directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

uklotzde
uklotzde suggested changes Nov 3, 2018
View reviewed changes
src/library/banshee/bansheeplaylistmodel.cpp Outdated
if (m_playlistId >= 0) {
// Clear old playlist
m_playlistId = -1;
QSqlQuery query(m_pTrackCollection->database());
QString strQuery("DELETE FROM " BANSHEE_TABLE);
if (!query.exec(strQuery)) {
QString strQuery("DELETE FROM %1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty table remains. Please drop it entirely instead of only deleting all records:
DROP TABLE IF EXISTS %1
https://www.sqlite.org/lang_droptable.html

src/library/banshee/bansheeplaylistmodel.cpp Outdated
if (column == fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_TRACKID) ||
(PlayerManager::numPreviewDecks() == 0 &&
column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_PREVIEW))) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid anti-pattern:
BAD: if (condition) { return true }
GOOD: return condition

src/library/banshee/bansheeplaylistmodel.h Outdated
@@ -41,9 +43,11 @@ class BansheePlaylistModel : public BaseSqlTableModel {
private:
QString getFieldString(const QModelIndex& index, const QString& fieldName) const;
QVariant getFieldVariant(const QModelIndex& index, const QString& fieldName) const;
void deleteTempTable();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropTempTable()

src/library/baseexternallibraryfeature.cpp
// use the data because models with nested playlists need to use the
// full path/name of the playlist.
*pPlaylist = m_lastRightClickedIndex.data(Qt::UserRole).toString();
*pPlaylist = m_lastRightClickedIndex.data().toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add at least a debug assertion before dereferencing pPlaylist:
DEBUG_ASSERT(pPlaylist);

@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

Done

@uklotzde
Copy link
Contributor

uklotzde commented Nov 4, 2018

LGTM

Since this affects a released version and the PR fixes multiple issues another approval is recommended. I've only tested with a small, external Banshee library created for this purpose.

@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

Thank you. A iTunes test would be nice. Any volunteers?

@uklotzde
Copy link
Contributor

This should also be included in 2.2.0 final!

@uklotzde
Copy link
Contributor

Installed both iTunes and the build artefact on Windows 10, works as expected. Single and multiple tracks can be selected and added to Auto DJ or playlists, order of tracks is preserved.

@uklotzde uklotzde merged commit 0f5e3b9 into mixxxdj:2.1 Nov 10, 2018
@uklotzde
Copy link
Contributor

Merged 2.1 into 2.2. I'm confident that I resolved all merge conflicts correctly.

@daschuer daschuer deleted the banshee-fix branch September 7, 2021 21:08
This was referenced Aug 23, 2022
Closed
Closed
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.6
Development

Successfully merging this pull request may close these issues.
2 participants
@daschuer @uklotzde