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.

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

Import from Rhythmbox playlist fix #11661

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jun 14, 2023

This fixes a regression since #11230 and Mixxx 2.3.4
that bulk importing tracks from playlists was broken.

@daschuer daschuer added this to the 2.3.6 milestone Jun 14, 2023
@daschuer daschuer changed the title Rhythmbox playlist fix Import from Rhythmbox playlist fix Jun 15, 2023
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

can you spend some more time making clazy happy? I think you'll need to add the appropriate overrides to all member functions of the class.

Comment on lines -94 to +95
BaseSqlTableModel* RhythmboxFeature::getPlaylistModelForPlaylist(const QString& playlist) {
BaseExternalPlaylistModel* pModel = new BaseExternalPlaylistModel(
this, m_pLibrary->trackCollections(),
"mixxx.db.model.rhythmbox_playlist",
"rhythmbox_playlists",
"rhythmbox_playlist_tracks",
m_trackSource);
std::unique_ptr<BaseSqlTableModel>
RhythmboxFeature::createPlaylistModelForPlaylist(const QString& playlist) {
Copy link
Member

Choose a reason for hiding this comment

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

just to make sure I understand the fix. The issue was basically that an oversight during refactoring resulted in the wrong member function being called?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the getPlaylistModelForPlaylist() is no longer called, instead the default base class implementation is used returning null.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

code LGTM. I don't use rythmbox so I can't really verify this fix easily

@daschuer
Copy link
Member Author

I can. And the coding mistake was obvious.
Merge?

@Swiftb0y
Copy link
Member

Sure...

@Swiftb0y Swiftb0y merged commit b2deaca into mixxxdj:2.3 Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants