Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
openSUSE
amarok.7817
Fix-crash-during-musicbrainz-search.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File Fix-crash-during-musicbrainz-search.patch of Package amarok.7817
From 88ee85a8333cf54dc123fef5e07a1c92ab9f7261 Mon Sep 17 00:00:00 2001 From: Sergey Ivanov <123kash@gmail.com> Date: Sun, 20 Aug 2017 11:25:57 +0300 Subject: Fix crash during musicbrainz search. BUG: 328359 REVIEW: 130232 --- ChangeLog | 3 +- src/musicbrainz/MusicBrainzFinder.cpp | 6 +- src/musicbrainz/MusicBrainzFinder.h | 3 +- src/musicbrainz/MusicBrainzTagsItem.cpp | 188 +++++++++++-------------------- src/musicbrainz/MusicBrainzTagsItem.h | 6 +- src/musicbrainz/MusicBrainzTagsModel.cpp | 66 +++++++++-- src/musicbrainz/MusicBrainzTagsModel.h | 2 + src/musicbrainz/MusicBrainzXmlParser.cpp | 3 +- src/musicbrainz/MusicBrainzXmlParser.h | 2 +- 9 files changed, 141 insertions(+), 138 deletions(-) diff --git a/src/musicbrainz/MusicBrainzFinder.cpp b/src/musicbrainz/MusicBrainzFinder.cpp index 9a1b3bb..2598a0b 100644 --- a/src/musicbrainz/MusicBrainzFinder.cpp +++ b/src/musicbrainz/MusicBrainzFinder.cpp @@ -132,8 +132,7 @@ MusicBrainzFinder::sendNewRequest() } void -MusicBrainzFinder::gotAuthenticationRequest( const QNetworkReply *reply, - QAuthenticator *authenticator ) const +MusicBrainzFinder::gotAuthenticationRequest( const QNetworkReply *reply, QAuthenticator *authenticator ) { if( reply->url().host() == mb_host ) { @@ -644,8 +643,9 @@ MusicBrainzFinder::compileRequest( QUrl &url ) url.setPort( mb_port ); QNetworkRequest req( url ); - req.setRawHeader( "User-Agent" , "Amarok" ); + req.setRawHeader( "Accept", "application/xml"); req.setRawHeader( "Connection", "Keep-Alive" ); + req.setRawHeader( "User-Agent" , "Amarok" ); if( !m_timer->isActive() ) m_timer->start(); diff --git a/src/musicbrainz/MusicBrainzFinder.h b/src/musicbrainz/MusicBrainzFinder.h index beb1551..4dbd4a5 100644 --- a/src/musicbrainz/MusicBrainzFinder.h +++ b/src/musicbrainz/MusicBrainzFinder.h @@ -51,8 +51,7 @@ class MusicBrainzFinder : public QObject private slots: void sendNewRequest(); - void gotAuthenticationRequest( const QNetworkReply *reply, - QAuthenticator *authenticator ) const; + void gotAuthenticationRequest( const QNetworkReply *reply, QAuthenticator *authenticator ); void gotReplyError( QNetworkReply::NetworkError code ); void gotReply( QNetworkReply *reply ); diff --git a/src/musicbrainz/MusicBrainzTagsItem.cpp b/src/musicbrainz/MusicBrainzTagsItem.cpp index 0941a90..6430392 100644 --- a/src/musicbrainz/MusicBrainzTagsItem.cpp +++ b/src/musicbrainz/MusicBrainzTagsItem.cpp @@ -65,146 +65,75 @@ MusicBrainzTagsItem::child( const int row ) const void MusicBrainzTagsItem::appendChild( MusicBrainzTagsItem *newItem ) { - DEBUG_BLOCK + QWriteLocker lock( &m_childrenLock ); + m_childItems.append( newItem ); + newItem->setParent( this ); - if( newItem->track().isNull() ) + if( !newItem->data().isEmpty() ) { - delete newItem; - return; - } + newItem->recalcSimilarityRate(); - if( m_track.isNull() ) - { - // Root item. - bool found = false; +#define MAKE_DATA_LIST( k ) { QVariantList list; if( newItem->dataContains( k ) ) list.append( newItem->dataValue( k ) ); newItem->dataInsert( k, list ); } - /* - * A write lock is required, because with a read lock two or more threads - * referencing the same track could search for a matching item at the same time, - * fail, and queue up to create a new one, thus resulting in duplicates. - */ - QWriteLocker lock( &m_childrenLock ); - foreach( MusicBrainzTagsItem *item, m_childItems ) - { - if( item->track() == newItem->track() ) - { - found = true; - if( !newItem->data().isEmpty() ) - item->appendChild( newItem ); - break; - } - } + MAKE_DATA_LIST( MusicBrainz::TRACKID ); + MAKE_DATA_LIST( MusicBrainz::ARTISTID ); + MAKE_DATA_LIST( MusicBrainz::RELEASEID ); - if( !found ) - { - MusicBrainzTagsItem *newChild = new MusicBrainzTagsItem( this, newItem->track() ); - if( !newItem->data().isEmpty() ) - newChild->appendChild( newItem ); - m_childItems.append( newChild ); - } - } - else - { - if( m_track != newItem->track() ) - { - debug() << "Trying to insert track data to the wrong tree branch."; - delete newItem; - return; - } - - bool found = false; - newItem->setParent( this ); - - // Already locked in parent call (the same logic applies). - foreach( MusicBrainzTagsItem *item, m_childItems ) - { - if( newItem == item ) - { - found = true; - // Merge the two matching results. - debug() << "Track" << newItem->dataValue( MusicBrainz::TRACKID ).toString() << "already in the tree."; - item->mergeWith( newItem ); - delete newItem; - break; - } - } - - if( !found ) - { - newItem->dataInsert( MusicBrainz::SIMILARITY, - newItem->dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() + - newItem->dataValue( MusicBrainz::MUSICDNS ).toFloat() ); - - QVariantList trackList; - QVariantList artistList; - QVariantList releaseList; - if( newItem->dataContains( MusicBrainz::TRACKID ) ) - trackList.append( newItem->dataValue( MusicBrainz::TRACKID ) ); - if( newItem->dataContains( MusicBrainz::ARTISTID ) ) - artistList.append( newItem->dataValue( MusicBrainz::ARTISTID ) ); - if( newItem->dataContains( MusicBrainz::RELEASEID ) ) - releaseList.append( newItem->dataValue( MusicBrainz::RELEASEID ) ); - newItem->dataInsert( MusicBrainz::TRACKID, trackList ); - newItem->dataInsert( MusicBrainz::ARTISTID, artistList ); - newItem->dataInsert( MusicBrainz::RELEASEID, releaseList ); - - m_childItems.append( newItem ); - } +#undef MAKE_DATA_LIST } } void -MusicBrainzTagsItem::mergeWith( MusicBrainzTagsItem *item ) +MusicBrainzTagsItem::mergeData( const QVariantMap &tags ) { - /* - * The lock is inherited from appendChild(). This method is not supposed to be called - * elsewhere. - */ + if( tags.isEmpty() ) + return; + MusicBrainzTagsItem fakeItem( this, m_track, tags ); // Calculate the future score of the result when merged. - if( !item->dataContains( MusicBrainz::MUSICBRAINZ ) && - dataContains( MusicBrainz::MUSICBRAINZ ) ) - item->dataInsert( MusicBrainz::MUSICBRAINZ, - dataValue( MusicBrainz::MUSICBRAINZ ) ); - if( !item->dataContains( MusicBrainz::MUSICDNS ) && - dataContains( MusicBrainz::MUSICDNS ) ) - item->dataInsert( MusicBrainz::MUSICDNS, - dataValue( MusicBrainz::MUSICDNS ) ); - item->dataInsert( MusicBrainz::SIMILARITY, - item->dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() + - item->dataValue( MusicBrainz::MUSICDNS ).toFloat() ); + if( !fakeItem.dataContains( MusicBrainz::MUSICBRAINZ ) && dataContains( MusicBrainz::MUSICBRAINZ ) ) + fakeItem.dataInsert( MusicBrainz::MUSICBRAINZ, dataValue( MusicBrainz::MUSICBRAINZ ) ); + + if( !fakeItem.dataContains( MusicBrainz::MUSICDNS ) && dataContains( MusicBrainz::MUSICDNS ) ) + fakeItem.dataInsert( MusicBrainz::MUSICDNS, dataValue( MusicBrainz::MUSICDNS ) ); + + fakeItem.recalcSimilarityRate(); QVariantList trackList = dataValue( MusicBrainz::TRACKID ).toList(); QVariantList artistList = dataValue( MusicBrainz::ARTISTID ).toList(); QVariantList releaseList = dataValue( MusicBrainz::RELEASEID ).toList(); - if( item->score() > score() ) + if( fakeItem.score() > score() ) { // Update the score. - if( item->dataContains( MusicBrainz::MUSICBRAINZ ) ) - dataInsert( MusicBrainz::MUSICBRAINZ, - item->dataValue( MusicBrainz::MUSICBRAINZ ) ); - if( item->dataContains( MusicBrainz::MUSICDNS ) ) - dataInsert( MusicBrainz::MUSICDNS, - item->dataValue( MusicBrainz::MUSICDNS ) ); - dataInsert( MusicBrainz::SIMILARITY, - item->dataValue( MusicBrainz::SIMILARITY ) ); - - if( item->dataContains( MusicBrainz::TRACKID ) ) - trackList.prepend( item->dataValue( MusicBrainz::TRACKID ) ); - if( item->dataContains( MusicBrainz::ARTISTID ) ) - artistList.prepend( item->dataValue( MusicBrainz::ARTISTID ) ); - if( item->dataContains( MusicBrainz::RELEASEID ) ) - releaseList.prepend( item->dataValue( MusicBrainz::RELEASEID ) ); + if( fakeItem.dataContains( MusicBrainz::MUSICBRAINZ ) ) + dataInsert( MusicBrainz::MUSICBRAINZ, fakeItem.dataValue( MusicBrainz::MUSICBRAINZ ) ); + + if( fakeItem.dataContains( MusicBrainz::MUSICDNS ) ) + dataInsert( MusicBrainz::MUSICDNS, fakeItem.dataValue( MusicBrainz::MUSICDNS ) ); + + recalcSimilarityRate(); + + if( fakeItem.dataContains( MusicBrainz::TRACKID ) ) + trackList.prepend( fakeItem.dataValue( MusicBrainz::TRACKID ) ); + + if( fakeItem.dataContains( MusicBrainz::ARTISTID ) ) + artistList.prepend( fakeItem.dataValue( MusicBrainz::ARTISTID ) ); + + if( fakeItem.dataContains( MusicBrainz::RELEASEID ) ) + releaseList.prepend( fakeItem.dataValue( MusicBrainz::RELEASEID ) ); } else { - if( item->dataContains( MusicBrainz::TRACKID ) ) - trackList.append( item->dataValue( MusicBrainz::TRACKID ) ); - if( item->dataContains( MusicBrainz::ARTISTID ) ) - artistList.append( item->dataValue( MusicBrainz::ARTISTID ) ); - if( item->dataContains( MusicBrainz::RELEASEID ) ) - releaseList.append( item->dataValue( MusicBrainz::RELEASEID ) ); + if( fakeItem.dataContains( MusicBrainz::TRACKID ) ) + trackList.append( fakeItem.dataValue( MusicBrainz::TRACKID ) ); + + if( fakeItem.dataContains( MusicBrainz::ARTISTID ) ) + artistList.append( fakeItem.dataValue( MusicBrainz::ARTISTID ) ); + + if( fakeItem.dataContains( MusicBrainz::RELEASEID ) ) + releaseList.append( fakeItem.dataValue( MusicBrainz::RELEASEID ) ); } + dataInsert( MusicBrainz::TRACKID, trackList ); dataInsert( MusicBrainz::ARTISTID, artistList ); dataInsert( MusicBrainz::RELEASEID, releaseList ); @@ -476,10 +405,11 @@ MusicBrainzTagsItem::clearChoices() } bool -MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const +MusicBrainzTagsItem::isSimilar( const QVariantMap &tags ) const { QReadLocker lock( &m_dataLock ); -#define MATCH( k, t ) dataValue( k ).t() == item->dataValue( k ).t() + QVariant empty; +#define MATCH( k, t ) ( dataValue( k ).t() == ( tags.contains( k ) ? tags.value( k ) : empty ).t() ) /* * This is the information shown to the user: he will never be able to * distinguish between two tracks with the same information. @@ -494,3 +424,21 @@ MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const MATCH( Meta::Field::TRACKNUMBER, toInt ); #undef MATCH } + +bool +MusicBrainzTagsItem::operator==( const MusicBrainzTagsItem* item ) const +{ + return isSimilar( item->data() ); +} + +bool +MusicBrainzTagsItem::operator==( const Meta::TrackPtr &track) const +{ + return m_track == track; +} + +void +MusicBrainzTagsItem::recalcSimilarityRate() +{ + dataInsert( MusicBrainz::SIMILARITY, dataValue( MusicBrainz::MUSICBRAINZ ).toFloat() + dataValue( MusicBrainz::MUSICDNS ).toFloat() ); +} diff --git a/src/musicbrainz/MusicBrainzTagsItem.h b/src/musicbrainz/MusicBrainzTagsItem.h index 6852c9f..11efb52 100644 --- a/src/musicbrainz/MusicBrainzTagsItem.h +++ b/src/musicbrainz/MusicBrainzTagsItem.h @@ -42,6 +42,7 @@ class MusicBrainzTagsItem QVariantMap data() const; QVariant data( const int column ) const; void setData( const QVariantMap &tags ); + void mergeData( const QVariantMap &tags ); bool dataContains( const QString &key ) const; QVariant dataValue( const QString &key ) const; @@ -52,12 +53,15 @@ class MusicBrainzTagsItem bool chooseBestMatchFromRelease( const QStringList &releases ); void clearChoices(); + bool isSimilar( const QVariantMap &tags ) const; + bool operator==( const MusicBrainzTagsItem *item ) const; + bool operator==( const Meta::TrackPtr &track) const; private: void setParent( MusicBrainzTagsItem *parent ); - void mergeWith( MusicBrainzTagsItem *item ); void dataInsert( const QString &key, const QVariant &value ); + void recalcSimilarityRate(); MusicBrainzTagsItem *m_parent; QList<MusicBrainzTagsItem *> m_childItems; diff --git a/src/musicbrainz/MusicBrainzTagsModel.cpp b/src/musicbrainz/MusicBrainzTagsModel.cpp index 0ffb284..e7bde4c 100644 --- a/src/musicbrainz/MusicBrainzTagsModel.cpp +++ b/src/musicbrainz/MusicBrainzTagsModel.cpp @@ -204,7 +204,10 @@ MusicBrainzTagsModel::setData( const QModelIndex &index, const QVariant &value, Qt::ItemFlags MusicBrainzTagsModel::flags( const QModelIndex &index ) const { - if( !index.isValid() || !parent( index ).isValid() ) + if( !index.isValid() ) + return QAbstractItemModel::flags( index ); + + if( !parent( index ).isValid() ) // Disable items with no children. return QAbstractItemModel::flags( index ) ^ ( ( !static_cast<MusicBrainzTagsItem *>( index.internalPointer() )->childCount() )? @@ -248,22 +251,67 @@ MusicBrainzTagsModel::columnCount( const QModelIndex &parent ) const void MusicBrainzTagsModel::addTrack( const Meta::TrackPtr track, const QVariantMap tags ) { - QModelIndex parent; - int row = rowCount(); - for( int i = 0; i < m_rootItem->childCount(); i++ ) + DEBUG_BLOCK + + if( track.isNull() ) + return; + + QMutexLocker lock( &m_modelLock ); + + MusicBrainzTagsItem *trackItem = NULL; + QModelIndex trackIndex; + for( int i = 0; i < m_rootItem->childCount(); ++i ) { MusicBrainzTagsItem *item = m_rootItem->child( i ); if( track == item->track() ) { - parent = index( i, 0 ); - row = rowCount( parent ); + trackItem = item; + trackIndex = index( i, 0 ); + + break; + } + } + + if( !trackItem ) + { + trackItem = new MusicBrainzTagsItem( m_rootItem, track ); + + beginInsertRows( QModelIndex(), m_rootItem->childCount(), m_rootItem->childCount() ); + m_rootItem->appendChild( trackItem ); + endInsertRows(); + + trackIndex = index( m_rootItem->childCount() - 1, 0 ); + } + + if( tags.isEmpty() ) + { + warning() << "Search result contains no data for track: " << track->prettyName(); + return; + } + + MusicBrainzTagsItem *similarItem = NULL; + for( int i = 0; i < trackItem->childCount(); ++i ) + { + MusicBrainzTagsItem *item = trackItem->child( i ); + if( item->isSimilar( tags ) ) + { + similarItem = item; + + item->mergeData( tags ); + emit dataChanged( index( i, 0, trackIndex ), index(i, columnCount() - 1, trackIndex ) ); + break; } } - beginInsertRows( parent, row, row ); - m_rootItem->appendChild( new MusicBrainzTagsItem( m_rootItem, track, tags ) ); - endInsertRows(); + if( !similarItem ) + { + MusicBrainzTagsItem *item = new MusicBrainzTagsItem( trackItem, track, tags ); + + beginInsertRows( trackIndex, trackItem->childCount(), trackItem->childCount() ); + trackItem->appendChild( item ); + endInsertRows(); + } } QMap<Meta::TrackPtr, QVariantMap> diff --git a/src/musicbrainz/MusicBrainzTagsModel.h b/src/musicbrainz/MusicBrainzTagsModel.h index eaee7f8..3ea2fd2 100644 --- a/src/musicbrainz/MusicBrainzTagsModel.h +++ b/src/musicbrainz/MusicBrainzTagsModel.h @@ -21,6 +21,7 @@ #include "core/meta/forward_declarations.h" #include <QAbstractItemModel> +#include <QMutex> class MusicBrainzTagsItem; @@ -69,6 +70,7 @@ class MusicBrainzTagsModel : public QAbstractItemModel private: MusicBrainzTagsItem *m_rootItem; + mutable QMutex m_modelLock; }; #endif // MUSICBRAINZTAGSMDOEL_H diff --git a/src/musicbrainz/MusicBrainzXmlParser.cpp b/src/musicbrainz/MusicBrainzXmlParser.cpp index 290a275..40d36a3 100644 --- a/src/musicbrainz/MusicBrainzXmlParser.cpp +++ b/src/musicbrainz/MusicBrainzXmlParser.cpp @@ -26,7 +26,7 @@ #include <QStringList> #include <QVariantList> -MusicBrainzXmlParser::MusicBrainzXmlParser( QString &doc ) +MusicBrainzXmlParser::MusicBrainzXmlParser( const QString &doc ) : ThreadWeaver::Job() , m_doc( "musicbrainz" ) , m_type( 0 ) @@ -38,6 +38,7 @@ void MusicBrainzXmlParser::run() { DEBUG_BLOCK + QDomElement docElem = m_doc.documentElement(); parseElement( docElem ); } diff --git a/src/musicbrainz/MusicBrainzXmlParser.h b/src/musicbrainz/MusicBrainzXmlParser.h index c7f9e72..7b2876b 100644 --- a/src/musicbrainz/MusicBrainzXmlParser.h +++ b/src/musicbrainz/MusicBrainzXmlParser.h @@ -34,7 +34,7 @@ class MusicBrainzXmlParser : public ThreadWeaver::Job ReleaseGroup }; - explicit MusicBrainzXmlParser( QString &doc ); + explicit MusicBrainzXmlParser( const QString &doc ); void run(); -- cgit v0.11.2
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
