Musicbrainz tagging improvements:

* Fix a bug where the song title would be used for the album field
  * Get the album's year as well
  * Include all releases for a song in the results list
  * Remove duplicate albums
  * Sort results

src/core/songloader.cpp

@@ -201,6 +201,7 @@ void SongLoader::AudioCDTagsLoaded(const QString& artist, const QString& album,
     song.set_title(ret.title_);
     song.set_length_nanosec(ret.duration_msec_ * kNsecPerMsec);
     song.set_track(track_number);
+    song.set_year(ret.year_);
     // We need to set url: that's how playlist will find the correct item to update
     song.set_url(QUrl(QString("cdda://%1").arg(track_number++)));
     songs_ << song;

src/core/utilities.cpp

@@ -32,6 +32,7 @@
 #include <QTemporaryFile>
 #include <QUrl>
 #include <QWidget>
+#include <QXmlStreamReader>
 #include <QtDebug>
 #include <QtGlobal>
 
@@ -383,6 +384,17 @@ quint16 PickUnusedPort() {
   }
 }
 
+void ConsumeCurrentElement(QXmlStreamReader* reader) {
+  int level = 1;
+  while (level != 0 && !reader->atEnd()) {
+    switch (reader->readNext()) {
+      case QXmlStreamReader::StartElement: ++level; break;
+      case QXmlStreamReader::EndElement:   --level; break;
+      default: break;
+    }
+  }
+}
+
 }  // namespace Utilities
 
 

src/core/utilities.h

@@ -29,6 +29,7 @@
 
 class QIODevice;
 class QMouseEvent;
+class QXmlStreamReader;
 
 namespace Utilities {
   QString PrettyTime(int seconds);
@@ -75,6 +76,12 @@ namespace Utilities {
   bool IsMouseEventInWidget(const QMouseEvent* e, const QWidget* widget);
 
 
+  // Reads all children of the current element, and returns with the stream
+  // reader either on the EndElement for the current element, or the end of the
+  // file - whichever came first.
+  void ConsumeCurrentElement(QXmlStreamReader* reader);
+
+
   enum ConfigPath {
     Path_Root,
     Path_AlbumCovers,

src/devices/cddadevice.cpp

@@ -144,8 +144,9 @@ void CddaDevice::AudioCDTagsLoaded(const QString& artist, const QString& album,
     song.set_album(album);
     song.set_title(ret.title_);
     song.set_length_nanosec(ret.duration_msec_ * kNsecPerMsec);
-    song.set_id(track_number);
     song.set_track(track_number);
+    song.set_year(ret.year_);
+    song.set_id(track_number);
     // We need to set url: that's how playlist will find the correct item to update
     song.set_url(QUrl(QString("cdda://%1/%2").arg(unique_id()).arg(track_number++)));
     songs << song;

src/internet/somafmservice.cpp

@@ -23,6 +23,7 @@
 #include "core/network.h"
 #include "core/player.h"
 #include "core/taskmanager.h"
+#include "core/utilities.h"
 #include "globalsearch/globalsearch.h"
 #include "globalsearch/somafmsearchprovider.h"
 #include "ui/iconloader.h"
@@ -151,7 +152,7 @@ void SomaFMService::ReadChannel(QXmlStreamReader& reader, StreamList* ret) {
 
           stream.url_ = url;
         } else {
-          ConsumeElement(reader);
+          Utilities::ConsumeCurrentElement(&reader);
         }
         break;
 
@@ -170,20 +171,6 @@ Song SomaFMService::Stream::ToSong() const {
   return ret;
 }
 
-void SomaFMService::ConsumeElement(QXmlStreamReader& reader) {
-  int level = 1;
-  while (!reader.atEnd()) {
-    switch (reader.readNext()) {
-      case QXmlStreamReader::StartElement: level++; break;
-      case QXmlStreamReader::EndElement:   level--; break;
-      default: break;
-    }
-
-    if (level == 0)
-      return;
-  }
-}
-
 void SomaFMService::Homepage() {
   QDesktopServices::openUrl(QUrl(kHomepage));
 }

src/internet/somafmservice.h

@@ -81,7 +81,6 @@ private slots:
 
 private:
   void ReadChannel(QXmlStreamReader& reader, StreamList* ret);
-  void ConsumeElement(QXmlStreamReader& reader);
   void PopulateStreams();
 
 private:

src/musicbrainz/musicbrainzclient.cpp

@@ -18,14 +18,17 @@
 #include "musicbrainzclient.h"
 #include "core/logging.h"
 #include "core/network.h"
+#include "core/utilities.h"
 
 #include <QCoreApplication>
 #include <QNetworkReply>
+#include <QSet>
 #include <QXmlStreamReader>
 #include <QtDebug>
 
 const char* MusicBrainzClient::kTrackUrl = "http://musicbrainz.org/ws/2/recording/";
 const char* MusicBrainzClient::kDiscUrl = "http://musicbrainz.org/ws/1/release/";
+const char* MusicBrainzClient::kDateRegex = "^[12]\\d{3}";
 const int MusicBrainzClient::kDefaultTimeout = 5000; // msec
 
 MusicBrainzClient::MusicBrainzClient(QObject* parent)
@@ -118,16 +121,18 @@ void MusicBrainzClient::DiscIdRequestFinished() {
   while (!reader.atEnd()) {
     QXmlStreamReader::TokenType token = reader.readNext();
     if (token == QXmlStreamReader::StartElement && reader.name() == "track") {
-      Result track = ParseTrack(&reader);
-      if (!track.title_.isEmpty()) {
-        ret << track;
+      ResultList tracks = ParseTrack(&reader);
+      foreach (const Result& track, tracks) {
+        if (!track.title_.isEmpty()) {
+          ret << track;
+        }
       }
     } else if (token == QXmlStreamReader::EndElement && reader.name() == "track-list") {
       break;
     }
   }
 
-  emit Finished(artist, album, ret);
+  emit Finished(artist, album, UniqueResults(ret));
 }
 
 
@@ -151,18 +156,21 @@ void MusicBrainzClient::RequestFinished() {
   QXmlStreamReader reader(reply);
   while (!reader.atEnd()) {
     if (reader.readNext() == QXmlStreamReader::StartElement && reader.name() == "recording") {
-      Result track = ParseTrack(&reader);
-      if (!track.title_.isEmpty()) {
-        ret << track;
+      ResultList tracks = ParseTrack(&reader);
+      foreach (const Result& track, tracks) {
+        if (!track.title_.isEmpty()) {
+          ret << track;
+        }
       }
     }
   }
 
-  emit Finished(id, ret);
+  emit Finished(id, UniqueResults(ret));
 }
 
-MusicBrainzClient::Result MusicBrainzClient::ParseTrack(QXmlStreamReader* reader) {
-  Result ret;
+MusicBrainzClient::ResultList MusicBrainzClient::ParseTrack(QXmlStreamReader* reader) {
+  Result result;
+  QList<Release> releases;
 
   while (!reader->atEnd()) {
     QXmlStreamReader::TokenType type = reader->readNext();
@@ -171,13 +179,13 @@ MusicBrainzClient::Result MusicBrainzClient::ParseTrack(QXmlStreamReader* reader
       QStringRef name = reader->name();
 
       if (name == "title") {
-        ret.title_ = reader->readElementText();
+        result.title_ = reader->readElementText();
       } else if (name == "length") {
-        ret.duration_msec_ = reader->readElementText().toInt();
+        result.duration_msec_ = reader->readElementText().toInt();
       } else if (name == "artist") {
-        ParseArtist(reader, &ret.artist_);
+        ParseArtist(reader, &result.artist_);
       } else if (name == "release") {
-        ParseAlbum(reader, &ret.album_, &ret.track_);
+        releases << ParseRelease(reader);
       }
     }
 
@@ -186,6 +194,10 @@ MusicBrainzClient::Result MusicBrainzClient::ParseTrack(QXmlStreamReader* reader
     }
   }
 
+  ResultList ret;
+  foreach (const Release& release, releases) {
+    ret << release.CopyAndMergeInto(result);
+  }
   return ret;
 }
 
@@ -203,21 +215,37 @@ void MusicBrainzClient::ParseArtist(QXmlStreamReader* reader, QString* artist) {
   }
 }
 
-void MusicBrainzClient::ParseAlbum(QXmlStreamReader* reader, QString* album, int* track) {
+MusicBrainzClient::Release MusicBrainzClient::ParseRelease(QXmlStreamReader* reader) {
+  Release ret;
+
   while (!reader->atEnd()) {
     QXmlStreamReader::TokenType type = reader->readNext();
 
     if (type == QXmlStreamReader::StartElement) {
       QStringRef name = reader->name();
       if (name == "title") {
-        *album = reader->readElementText();
+        ret.album_ = reader->readElementText();
+      } else if (name == "date") {
+        QRegExp regex(kDateRegex);
+        if (regex.indexIn(reader->readElementText()) == 0) {
+          ret.year_ = regex.cap(0).toInt();
+        }
       } else if (name == "track-list") {
-        *track = reader->attributes().value("offset").toString().toInt() + 1;
+        ret.track_ = reader->attributes().value("offset").toString().toInt() + 1;
+        Utilities::ConsumeCurrentElement(reader);
       }
     }
 
     if (type == QXmlStreamReader::EndElement && reader->name() == "release") {
-      return;
+      break;
     }
   }
+
+  return ret;
+}
+
+MusicBrainzClient::ResultList MusicBrainzClient::UniqueResults(const ResultList& results) {
+  ResultList ret = QSet<Result>::fromList(results).toList();
+  qSort(ret);
+  return ret;
 }

src/musicbrainz/musicbrainzclient.h

@@ -18,6 +18,7 @@
 #ifndef MUSICBRAINZCLIENT_H
 #define MUSICBRAINZCLIENT_H
 
+#include <QHash>
 #include <QMap>
 #include <QObject>
 #include <QXmlStreamReader>
@@ -40,16 +41,41 @@ public:
   MusicBrainzClient(QObject* parent = 0);
 
   struct Result {
-    Result() : duration_msec_(0), track_(0) {}
+    Result() : duration_msec_(0), track_(0), year_(-1) {}
+
+    bool operator <(const Result& other) const {
+      #define cmp(field) \
+        if (field < other.field) return true; \
+        if (field > other.field) return false;
+
+      cmp(track_);
+      cmp(year_);
+      cmp(title_);
+      cmp(artist_);
+      return false;
+
+      #undef cmp
+    }
+
+    bool operator ==(const Result& other) const {
+      return title_ == other.title_ &&
+             artist_ == other.artist_ &&
+             album_ == other.album_ &&
+             duration_msec_ == other.duration_msec_ &&
+             track_ == other.track_ &&
+             year_ == other.year_;
+    }
 
     QString title_;
     QString artist_;
     QString album_;
     int duration_msec_;
     int track_;
+    int year_;
   };
   typedef QList<Result> ResultList;
 
+
   // Starts a request and returns immediately.  Finished() will be emitted
   // later with the same ID.
   void Start(int id, const QString& mbid);
@@ -75,13 +101,31 @@ private slots:
   void DiscIdRequestFinished();
 
 private:
-  static Result ParseTrack(QXmlStreamReader* reader);
+  struct Release {
+    Release() : track_(0), year_(0) {}
+
+    Result CopyAndMergeInto(const Result& orig) const {
+      Result ret(orig);
+      ret.album_ = album_;
+      ret.track_ = track_;
+      ret.year_ = year_;
+      return ret;
+    }
+
+    QString album_;
+    int track_;
+    int year_;
+  };
+
+  static ResultList ParseTrack(QXmlStreamReader* reader);
   static void ParseArtist(QXmlStreamReader* reader, QString* artist);
-  static void ParseAlbum(QXmlStreamReader* reader, QString* album, int* track);
+  static Release ParseRelease(QXmlStreamReader* reader);
+  static ResultList UniqueResults(const ResultList& results);
 
 private:
   static const char* kTrackUrl;
   static const char* kDiscUrl;
+  static const char* kDateRegex;
   static const int kDefaultTimeout;
 
   QNetworkAccessManager* network_;
@@ -89,4 +133,13 @@ private:
   QMap<QNetworkReply*, int> requests_;
 };
 
+inline uint qHash(const MusicBrainzClient::Result& result) {
+  return qHash(result.album_) ^
+         qHash(result.artist_) ^
+         result.duration_msec_ ^
+         qHash(result.title_) ^
+         result.track_ ^
+         result.year_;
+}
+
 #endif // MUSICBRAINZCLIENT_H

src/musicbrainz/tagfetcher.cpp

@@ -116,6 +116,7 @@ void TagFetcher::TagsFetched(int index, const MusicBrainzClient::ResultList& res
     song.Init(result.title_, result.artist_, result.album_,
               result.duration_msec_ * kNsecPerMsec);
     song.set_track(result.track_);
+    song.set_year(result.year_);
     songs_guessed << song;
   }
 

src/playlistparsers/xmlparser.cpp

@@ -43,20 +43,3 @@ bool XMLParser::ParseUntilElement(QXmlStreamReader* reader, const QString& name)
   }
   return false;
 }
-
-void XMLParser::IgnoreElement(QXmlStreamReader* reader) const {
-  int level = 1;
-  while (level != 0 && !reader->atEnd()) {
-    QXmlStreamReader::TokenType type = reader->readNext();
-    switch (type) {
-      case QXmlStreamReader::StartElement:
-        ++level;
-        break;
-      case QXmlStreamReader::EndElement:
-        --level;
-        break;
-      default:
-        break;
-    }
-  }
-}

src/playlistparsers/xmlparser.h

@@ -33,7 +33,6 @@ class XMLParser : public ParserBase {
   XMLParser(LibraryBackendInterface* library, QObject* parent);
 
   bool ParseUntilElement(QXmlStreamReader* reader, const QString& element) const;
-  void IgnoreElement(QXmlStreamReader* reader) const;
 
   class StreamElement : public boost::noncopyable {
    public:

src/ui/trackselectiondialog.cpp

@@ -60,9 +60,10 @@ TrackSelectionDialog::TrackSelectionDialog(QWidget *parent)
 
   // Resize columns
   ui_->results->setColumnWidth(0, 50);  // Track column
-  ui_->results->setColumnWidth(1, 175); // Title column
-  ui_->results->setColumnWidth(2, 175); // Artist column
-  ui_->results->setColumnWidth(3, 175); // Album column
+  ui_->results->setColumnWidth(1, 50);  // Year column
+  ui_->results->setColumnWidth(2, 160); // Title column
+  ui_->results->setColumnWidth(3, 160); // Artist column
+  ui_->results->setColumnWidth(4, 160); // Album column
 }
 
 TrackSelectionDialog::~TrackSelectionDialog() {
@@ -198,8 +199,9 @@ void TrackSelectionDialog::AddDivider(const QString& text, QTreeWidget* parent)
 
 void TrackSelectionDialog::AddSong(const Song& song, int result_index, QTreeWidget* parent) const {
   QStringList values;
-  values << ((song.track() > 0) ? QString::number(song.track()) : QString());
-  values << song.title() << song.artist() << song.album();
+  values << ((song.track() > 0) ? QString::number(song.track()) : QString())
+         << ((song.year() > 0) ? QString::number(song.year()) : QString())
+         << song.title() << song.artist() << song.album();
 
   QTreeWidgetItem* item = new QTreeWidgetItem(parent, values);
   item->setData(0, Qt::UserRole, result_index);

src/ui/trackselectiondialog.ui

@@ -69,7 +69,7 @@
            </spacer>
           </item>
           <item>
-           <widget class="BusyIndicator" name="progress"/>
+           <widget class="BusyIndicator" name="progress" native="true"/>
           </item>
           <item>
            <spacer name="horizontalSpacer_2">
@@ -191,6 +191,11 @@
             <string>Track</string>
            </property>
           </column>
+          <column>
+           <property name="text">
+            <string>Year</string>
+           </property>
+          </column>
           <column>
            <property name="text">
             <string>Title</string>
@@ -216,7 +221,7 @@
    <item>
     <layout class="QHBoxLayout" name="horizontalLayout_2">
      <item>
-      <widget class="BusyIndicator" name="loading_label"/>
+      <widget class="BusyIndicator" name="loading_label" native="true"/>
      </item>
      <item>
       <widget class="QDialogButtonBox" name="button_box">