Commit 88ee85a8 authored by Sergey Ivanov's avatar Sergey Ivanov
Browse files

Fix crash during musicbrainz search.

BUG: 328359
REVIEW: 130232
parent 2702bb88
Amarok ChangeLog
================
(C) 2002-2016 the Amarok authors.
(C) 2002-2017 the Amarok authors.
VERSION 2.9.0
FEATURES:
......@@ -18,6 +18,7 @@ VERSION 2.9.0
* Fix MPRIS2 DesktopEntry name, makes media controls in Plasma 5.7 taskbar work
again; thanks to Antonio Rojas, Rex Dieter. (BR 365275)
* Auto-expand after search in Collection Browser works correctly again. (BR 335217)
* Fix crash during MusicBrainz search. (BR 328359)
VERSION 2.8.90
......
......@@ -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();
......
......@@ -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 );
......
......@@ -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() );
}
......@@ -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;
......
......@@ -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 = nullptr;
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 = nullptr;
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>
......
......@@ -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
......@@ -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 );
}
......
......@@ -34,7 +34,7 @@ class MusicBrainzXmlParser : public ThreadWeaver::Job
ReleaseGroup
};
explicit MusicBrainzXmlParser( QString &doc );
explicit MusicBrainzXmlParser( const QString &doc );
void run();
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment