Extend tag support: performer, grouping

The transaction handling while upgrading the database schema had to be revised.
Furthermore some QSqlQuery statements needed to be finished properly.

Fixes issue 2556

data/data.qrc

@@ -343,6 +343,7 @@
         <file>schema/schema-42.sql</file>
         <file>schema/schema-43.sql</file>
         <file>schema/schema-44.sql</file>
+        <file>schema/schema-45.sql</file>
         <file>schema/schema-4.sql</file>
         <file>schema/schema-5.sql</file>
         <file>schema/schema-6.sql</file>

data/schema/jamendo.sql

@@ -42,11 +42,14 @@ CREATE TABLE jamendo.songs (
   unavailable INTEGER DEFAULT 0,
 
   effective_albumartist TEXT,
-  etag TEXT
+  etag TEXT,
+
+  performer TEXT,
+  grouping TEXT
 );
 
 CREATE VIRTUAL TABLE jamendo.songs_fts USING fts3(
-  ftstitle, ftsalbum, ftsartist, ftsalbumartist, ftscomposer, ftsgenre, ftscomment,
+  ftstitle, ftsalbum, ftsartist, ftsalbumartist, ftscomposer, ftsperformer, ftsgrouping, ftsgenre, ftscomment,
   tokenize=unicode
 );
 

data/schema/schema-45.sql

@@ -0,0 +1,24 @@
+CREATE VIRTUAL TABLE playlist_items_fts USING fts3(
+  ftstitle, ftsalbum, ftsartist, ftsalbumartist, ftscomposer, ftsgenre, ftscomment,
+  tokenize=unicode
+);
+
+DELETE FROM %allsongstables_fts;
+
+DROP TABLE %allsongstables_fts;
+
+ALTER TABLE %allsongstables ADD COLUMN performer TEXT;
+
+ALTER TABLE %allsongstables ADD COLUMN grouping TEXT;
+
+CREATE VIRTUAL TABLE %allsongstables_fts USING fts3(
+  ftstitle, ftsalbum, ftsartist, ftsalbumartist, ftscomposer, ftsperformer, ftsgrouping, ftsgenre, ftscomment,
+  tokenize=unicode
+);
+
+INSERT INTO %allsongstables_fts (ROWID, ftstitle, ftsalbum, ftsartist, ftsalbumartist, ftscomposer, ftsperformer, ftsgrouping, ftsgenre, ftscomment)
+    SELECT ROWID, title, album, artist, albumartist, composer, performer, grouping, genre, comment
+    FROM %allsongstables;
+
+UPDATE schema_version SET version=45;
+

ext/libclementine-tagreader/tagreader.cpp

@@ -161,6 +161,12 @@ void TagReader::ReadFile(const QString& filename,
       if (!map["TCOM"].isEmpty())
         Decode(map["TCOM"].front()->toString(), NULL, song->mutable_composer());
 
+      if (!map["TIT1"].isEmpty()) // content group
+        Decode(map["TIT1"].front()->toString(), NULL, song->mutable_grouping());
+
+      if (!map["TPE1"].isEmpty()) // ID3v2: lead performer/soloist
+        Decode(map["TPE1"].front()->toString(), NULL, song->mutable_performer());
+
       if (!map["TPE2"].isEmpty()) // non-standard: Apple, Microsoft
         Decode(map["TPE2"].front()->toString(), NULL, song->mutable_albumartist());
 
@@ -260,6 +266,9 @@ void TagReader::ReadFile(const QString& filename,
       if(items.contains("\251wrt")) {
         Decode(items["\251wrt"].toStringList().toString(", "), NULL, song->mutable_composer());
       }
+      if(items.contains("\251grp")) {
+        Decode(items["\251grp"].toStringList().toString(" "), NULL, song->mutable_grouping());
+      }
       Decode(mp4_tag->comment(), NULL, song->mutable_comment());
     }
   }
@@ -398,6 +407,10 @@ void TagReader::ParseOggTag(const TagLib::Ogg::FieldListMap& map,
                                   pb::tagreader::SongMetadata* song) const {
   if (!map["COMPOSER"].isEmpty())
     Decode(map["COMPOSER"].front(), codec, song->mutable_composer());
+  if (!map["PERFORMER"].isEmpty())
+    Decode(map["PERFORMER"].front(), codec, song->mutable_performer());
+  if (!map["CONTENT GROUP"].isEmpty())
+    Decode(map["CONTENT GROUP"].front(), codec, song->mutable_grouping());
 
   if (!map["ALBUMARTIST"].isEmpty()) {
     Decode(map["ALBUMARTIST"].front(), codec, song->mutable_albumartist());
@@ -428,6 +441,8 @@ void TagReader::SetVorbisComments(TagLib::Ogg::XiphComment* vorbis_comments,
                                   const pb::tagreader::SongMetadata& song) const {
 
   vorbis_comments->addField("COMPOSER", StdStringToTaglibString(song.composer()), true);
+  vorbis_comments->addField("PERFORMER", StdStringToTaglibString(song.performer()), true);
+  vorbis_comments->addField("CONTENT GROUP", StdStringToTaglibString(song.grouping()), true);
   vorbis_comments->addField("BPM", QStringToTaglibString(song.bpm() <= 0 -1 ? QString() : QString::number(song.bpm())), true);
   vorbis_comments->addField("DISCNUMBER", QStringToTaglibString(song.disc() <= 0 -1 ? QString() : QString::number(song.disc())), true);
   vorbis_comments->addField("COMPILATION", StdStringToTaglibString(song.compilation() ? "1" : "0"), true);
@@ -501,6 +516,8 @@ bool TagReader::SaveFile(const QString& filename,
     SetTextFrame("TPOS", song.disc() <= 0 -1 ? QString() : QString::number(song.disc()), tag);
     SetTextFrame("TBPM", song.bpm() <= 0 -1 ? QString() : QString::number(song.bpm()), tag);
     SetTextFrame("TCOM", song.composer(), tag);
+    SetTextFrame("TIT1", song.grouping(), tag);
+    SetTextFrame("TPE1", song.performer(), tag);
     SetTextFrame("TPE2", song.albumartist(), tag);
     SetTextFrame("TCMP", std::string(song.compilation() ? "1" : "0"), tag);
   } else if (TagLib::FLAC::File* file = dynamic_cast<TagLib::FLAC::File*>(fileref->file())) {
@@ -511,6 +528,7 @@ bool TagReader::SaveFile(const QString& filename,
     tag->itemListMap()["disk"]    = TagLib::MP4::Item(song.disc() <= 0 -1 ? 0 : song.disc(), 0);
     tag->itemListMap()["tmpo"]    = TagLib::StringList(song.bpm() <= 0 -1 ? "0" : TagLib::String::number(song.bpm()));
     tag->itemListMap()["\251wrt"] = TagLib::StringList(song.composer());
+    tag->itemListMap()["\251grp"] = TagLib::StringList(song.grouping());
     tag->itemListMap()["aART"]    = TagLib::StringList(song.albumartist());
     tag->itemListMap()["cpil"]    = TagLib::StringList(song.compilation() ? "1" : "0");
   }

ext/libclementine-tagreader/tagreadermessages.proto

@@ -49,6 +49,8 @@ message SongMetadata {
   optional string art_automatic = 28;
   optional Type type = 29;
   optional string etag = 30;
+  optional string performer = 31;
+  optional string grouping = 32;
 }
 
 message ReadFileRequest {

src/core/database.cpp

@@ -37,7 +37,7 @@
 #include <QVariant>
 
 const char* Database::kDatabaseFilename = "clementine.db";
-const int Database::kSchemaVersion = 44;
+const int Database::kSchemaVersion = 45;
 const char* Database::kMagicAllSongsTables = "%allsongstables";
 
 int Database::sNextConnectionId = 1;
@@ -369,12 +369,16 @@ QSqlDatabase Database::Connect() {
   // Find Sqlite3 functions in the Qt plugin.
   StaticInit();
 
-  QSqlQuery set_fts_tokenizer("SELECT fts3_tokenizer(:name, :pointer)", db);
-  set_fts_tokenizer.bindValue(":name", "unicode");
-  set_fts_tokenizer.bindValue(":pointer", QByteArray(
-      reinterpret_cast<const char*>(&sFTSTokenizer), sizeof(&sFTSTokenizer)));
-  if (!set_fts_tokenizer.exec()) {
-    qLog(Warning) << "Couldn't register FTS3 tokenizer";
+  {
+    QSqlQuery set_fts_tokenizer("SELECT fts3_tokenizer(:name, :pointer)", db);
+    set_fts_tokenizer.bindValue(":name", "unicode");
+    set_fts_tokenizer.bindValue(":pointer", QByteArray(
+        reinterpret_cast<const char*>(&sFTSTokenizer), sizeof(&sFTSTokenizer)));
+    if (!set_fts_tokenizer.exec()) {
+      qLog(Warning) << "Couldn't register FTS3 tokenizer";
+    }
+    // Implicit invocation of ~QSqlQuery() when leaving the scope
+    // to release any remaining database locks!
   }
 
   if (db.tables().count() == 0) {
@@ -410,9 +414,8 @@ QSqlDatabase Database::Connect() {
     QSqlQuery q(QString("SELECT ROWID FROM %1.sqlite_master"
                         " WHERE type='table'").arg(key), db);
     if (!q.exec() || !q.next()) {
-      ScopedTransaction t(&db);
-      ExecFromFile(attached_databases_[key].schema_, db, 0);
-      t.Commit();
+      q.finish();
+      ExecSchemaCommandsFromFile(db, attached_databases_[key].schema_, 0);
     }
   }
 
@@ -421,10 +424,14 @@ QSqlDatabase Database::Connect() {
 
 void Database::UpdateMainSchema(QSqlDatabase* db) {
   // Get the database's schema version
-  QSqlQuery q("SELECT version FROM schema_version", *db);
   int schema_version = 0;
-  if (q.next())
-    schema_version = q.value(0).toInt();
+  {
+    QSqlQuery q("SELECT version FROM schema_version", *db);
+    if (q.next())
+      schema_version = q.value(0).toInt();
+    // Implicit invocation of ~QSqlQuery() when leaving the scope
+    // to release any remaining database locks!
+  }
 
   startup_schema_version_ = schema_version;
 
@@ -478,13 +485,12 @@ void Database::UpdateDatabaseSchema(int version, QSqlDatabase &db) {
     filename = ":/schema/schema.sql";
   else
     filename = QString(":/schema/schema-%1.sql").arg(version);
-
-  ScopedTransaction t(&db);
-
+
   if (version == 31) {
     // This version used to do a bad job of converting filenames in the songs
     // table to file:// URLs.  Now we do it properly here instead.
-
+    ScopedTransaction t(&db);
+
     UrlEncodeFilenameColumn("songs", db);
     UrlEncodeFilenameColumn("playlist_items", db);
 
@@ -493,61 +499,83 @@ void Database::UpdateDatabaseSchema(int version, QSqlDatabase &db) {
         UrlEncodeFilenameColumn(table, db);
       }
     }
+    qLog(Debug) << "Applying database schema update" << version
+      << "from" << filename;
+    ExecSchemaCommandsFromFile(db, filename, version - 1, &t);
+    t.Commit();
+  } else {
+    qLog(Debug) << "Applying database schema update" << version
+                << "from" << filename;
+    ExecSchemaCommandsFromFile(db, filename, version - 1);
   }
-
-  qLog(Debug) << "Applying database schema update" << version
-              << "from" << filename;
-  ExecFromFile(filename, db, version - 1);
-  t.Commit();
 }
 
 void Database::UrlEncodeFilenameColumn(const QString& table, QSqlDatabase& db) {
   QSqlQuery select(QString("SELECT ROWID, filename FROM %1").arg(table), db);
   QSqlQuery update(QString("UPDATE %1 SET filename=:filename WHERE ROWID=:id").arg(table), db);
-
   select.exec();
   if (CheckErrors(select)) return;
   while (select.next()) {
     const int rowid = select.value(0).toInt();
     const QString filename = select.value(1).toString();
-    
+
     if (filename.isEmpty() || filename.contains("://")) {
       continue;
     }
-    
+
     const QUrl url = QUrl::fromLocalFile(filename);
-    
+
     update.bindValue(":filename", url.toEncoded());
     update.bindValue(":id", rowid);
     update.exec();
     CheckErrors(update);
   }
 }
 
-void Database::ExecFromFile(const QString &filename, QSqlDatabase &db,
-                            int schema_version) {
+void Database::ExecSchemaCommandsFromFile(QSqlDatabase& db,
+                                          QString const& filename,
+                                          int schema_version,
+                                          ScopedTransaction const* outerTransaction) {
   // Open and read the database schema
   QFile schema_file(filename);
   if (!schema_file.open(QIODevice::ReadOnly))
     qFatal("Couldn't open schema file %s", filename.toUtf8().constData());
-  ExecCommands(QString::fromUtf8(schema_file.readAll()), db, schema_version);
+  ExecSchemaCommands(db, QString::fromUtf8(schema_file.readAll()), schema_version, outerTransaction);
 }
 
-void Database::ExecCommands(const QString& schema, QSqlDatabase& db,
-                            int schema_version) {
+void Database::ExecSchemaCommands(QSqlDatabase& db,
+                                  QString const& schema,
+                                  int schema_version,
+                                  ScopedTransaction const* outerTransaction) {
   // Run each command
-  QStringList commands(schema.split(";\n\n"));
+  QStringList const schemaCommands(schema.split(";\n\n"));
 
   // We don't want this list to reflect possible DB schema changes
   // so we initialize it before executing any statements.
-  QStringList tables = SongsTables(db, schema_version);
+  // If no outer transaction is provided the song tables need to
+  // be queried before beginning an inner transaction! Otherwise
+  // DROP TABLE commands on song tables may fail due to database
+  // locks.
+  QStringList const songTables(SongsTables(db, schema_version));
+
+  if (0 == outerTransaction) {
+    ScopedTransaction innerTransaction(&db);
+    ExecSongTablesCommands(db, songTables, schemaCommands);
+    innerTransaction.Commit();
+  } else {
+    ExecSongTablesCommands(db, songTables, schemaCommands);
+  }
+}
 
-  foreach (const QString& command, commands) {
+void Database::ExecSongTablesCommands(QSqlDatabase& db,
+                                      QStringList const& songTables,
+                                      QStringList const& commands) {
+  foreach (QString const& command, commands) {
     // There are now lots of "songs" tables that need to have the same schema:
     // songs, magnatune_songs, and device_*_songs.  We allow a magic value
     // in the schema files to update all songs tables at once.
     if (command.contains(kMagicAllSongsTables)) {
-      foreach (const QString& table, tables) {
+      foreach (QString const& table, songTables) {
         qLog(Info) << "Updating" << table << "for" << kMagicAllSongsTables;
         QString new_command(command);
         new_command.replace(kMagicAllSongsTables, table);

src/core/database.h

@@ -38,6 +38,7 @@ struct sqlite3_tokenizer_module;
 }
 
 class Application;
+class ScopedTransaction;
 
 class Database : public QObject {
   Q_OBJECT
@@ -55,8 +56,7 @@ class Database : public QObject {
   QMutex* Mutex() { return &mutex_; }
 
   void RecreateAttachedDb(const QString& database_name);
-  void ExecFromFile(const QString& filename, QSqlDatabase &db, int schema_version);
-  void ExecCommands(const QString& commands, QSqlDatabase &db, int schema_version);
+  void ExecSchemaCommands(QSqlDatabase &db, QString const& schema, int schema_version, ScopedTransaction const* outerTransaction = 0);
 
   int startup_schema_version() const { return startup_schema_version_; }
   int current_schema_version() const { return kSchemaVersion; }
@@ -70,6 +70,9 @@ class Database : public QObject {
  private:
   void UpdateMainSchema(QSqlDatabase* db);
 
+  void ExecSchemaCommandsFromFile(QSqlDatabase &db, QString const& filename, int schema_version, ScopedTransaction const* outerTransaction = 0);
+  void ExecSongTablesCommands(QSqlDatabase &db, QStringList const& songTables, QStringList const& commands);
+
   void UpdateDatabaseSchema(int version, QSqlDatabase& db);
   void UrlEncodeFilenameColumn(const QString& table, QSqlDatabase& db);
   QStringList SongsTables(QSqlDatabase& db, int schema_version) const;

src/core/mpris1.cpp

@@ -342,6 +342,8 @@ QVariantMap Mpris1::GetMetadata(const Song& song) {
   AddMetadata("audio-samplerate", song.samplerate(), &ret);
   AddMetadata("bpm", song.bpm(), &ret);
   AddMetadata("composer", song.composer(), &ret);
+  AddMetadata("performer", song.performer(), &ret);
+  AddMetadata("grouping", song.grouping(), &ret);
   if (song.rating() != -1.0) {
     AddMetadata("rating", song.rating() * 5, &ret);
   }

src/core/organiseformat.cpp

@@ -27,7 +27,8 @@ const char* OrganiseFormat::kBlockPattern = "\\{([^{}]+)\\}";
 const QStringList OrganiseFormat::kKnownTags = QStringList()
     << "title" << "album" << "artist" << "artistinitial" << "albumartist"
     << "composer" << "track" << "disc" << "bpm" << "year" << "genre"
-    << "comment" << "length" << "bitrate" << "samplerate" << "extension";
+    << "comment" << "length" << "bitrate" << "samplerate" << "extension"
+    << "performer" << "grouping";
 
 // From http://en.wikipedia.org/wiki/8.3_filename#Directory_table
 const char* OrganiseFormat::kInvalidFatCharacters = "\"*/\\:<>?|";
@@ -131,6 +132,8 @@ QString OrganiseFormat::TagValue(const QString &tag, const Song &song) const {
   else if (tag == "album")       value = song.album();
   else if (tag == "artist")      value = song.artist();
   else if (tag == "composer")    value = song.composer();
+  else if (tag == "performer")   value = song.performer();
+  else if (tag == "grouping")    value = song.grouping();
   else if (tag == "genre")       value = song.genre();
   else if (tag == "comment")     value = song.comment();
   else if (tag == "year")        value = QString::number(song.year());