Regarding issue #6468 (tagreader problems)

ext/clementine-tagreader/tagreaderworker.cpp

@@ -46,6 +46,15 @@ void TagReaderWorker::MessageArrived(const pb::tagreader::Message& message) {
     reply.mutable_save_file_response()->set_success(tag_reader_.SaveFile(
         QStringFromStdString(message.save_file_request().filename()),
         message.save_file_request().metadata()));
+  } else if (message.has_save_song_tag_to_file_request()) {
+    reply.mutable_save_song_tag_to_file_response()->set_success(
+        tag_reader_.UpdateSongTag(
+            QStringFromStdString(
+                message.save_song_tag_to_file_request().filename()),
+            QStringFromStdString(
+                message.save_song_tag_to_file_request().tagname()),
+            QStringFromStdString(
+                message.save_song_tag_to_file_request().tagvalue())));
   } else if (message.has_save_song_statistics_to_file_request()) {
     reply.mutable_save_song_statistics_to_file_response()->set_success(
         tag_reader_.SaveSongStatisticsToFile(

ext/libclementine-tagreader/tagreader.cpp

@@ -49,6 +49,8 @@
 #include <tag.h>
 #include <tdebuglistener.h>
 #include <textidentificationframe.h>
+#include <tfile.h>
+#include <tpropertymap.h>
 #include <trueaudiofile.h>
 #include <tstring.h>
 #include <unsynchronizedlyricsframe.h>
@@ -306,8 +308,8 @@ void TagReader::ReadFile(const QString& filename,
     }
 
     if (items.contains("BPM")) {
-      Decode(items["BPM"].toStringList().toString(", "), nullptr,
-             song->mutable_performer());
+      song->set_bpm(
+          TStringToQString(items["BPM"].toString()).trimmed().toFloat());
     }
 
     if (items.contains("PERFORMER")) {
@@ -502,7 +504,12 @@ void TagReader::ReadFile(const QString& filename,
           song->set_score(score);
         }
       }
-
+      if (items.contains("tmpo")) {
+        float bpm = (float)items["tmpo"].toInt();
+        if (song->bpm() <= 0 && bpm > 0) {
+          song->set_bpm(bpm);
+        }
+      }
       if (items.contains("\251wrt")) {
         Decode(items["\251wrt"].toStringList().toString(", "), nullptr,
                song->mutable_composer());
@@ -772,42 +779,6 @@ void TagReader::ParseOggTag(const TagLib::Ogg::FieldListMap& map,
     Decode(map["UNSYNCEDLYRICS"].front(), codec, song->mutable_lyrics());
 }
 
-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 ? QString()
-                                             : QString::number(song.disc())),
-      true);
-  vorbis_comments->addField(
-      "COMPILATION",
-      QStringToTaglibString(song.compilation() ? QString::number(1)
-                                               : QString()),
-      true);
-
-  // Try to be coherent, the two forms are used but the first one is preferred
-
-  vorbis_comments->addField("ALBUMARTIST",
-                            StdStringToTaglibString(song.albumartist()), true);
-  vorbis_comments->removeField("ALBUM ARTIST");
-
-  vorbis_comments->addField("LYRICS", StdStringToTaglibString(song.lyrics()),
-                            true);
-  vorbis_comments->removeField("UNSYNCEDLYRICS");
-}
-
 void TagReader::SetFMPSStatisticsVorbisComments(
     TagLib::Ogg::XiphComment* vorbis_comments,
     const pb::tagreader::SongMetadata& song) const {
@@ -872,108 +843,68 @@ bool TagReader::SaveFile(const QString& filename,
                          const pb::tagreader::SongMetadata& song) const {
   if (filename.isNull()) return false;
 
-  qLog(Debug) << "Saving tags to" << filename;
-
   std::unique_ptr<TagLib::FileRef> fileref(factory_->GetFileRef(filename));
-
   if (!fileref || fileref->isNull())  // The file probably doesn't exist
     return false;
 
-  fileref->tag()->setTitle(StdStringToTaglibString(song.title()));
-  fileref->tag()->setArtist(StdStringToTaglibString(song.artist()));  // TPE1
-  fileref->tag()->setAlbum(StdStringToTaglibString(song.album()));
-  fileref->tag()->setGenre(StdStringToTaglibString(song.genre()));
-  fileref->tag()->setComment(StdStringToTaglibString(song.comment()));
-  fileref->tag()->setYear(song.year() <= 0 - 1 ? 0 : song.year());
-  fileref->tag()->setTrack(song.track() <= 0 - 1 ? 0 : song.track());
-
-  auto saveApeTag = [&](TagLib::APE::Tag* tag) {
-    tag->addValue(
-        "disc",
-        QStringToTaglibString(song.disc() <= 0 ? QString()
-                                               : QString::number(song.disc())),
-        true);
-    tag->addValue("bpm",
-                  QStringToTaglibString(song.bpm() <= 0 - 1
-                                            ? QString()
-                                            : QString::number(song.bpm())),
-                  true);
-    tag->setItem("composer",
-                 TagLib::APE::Item(
-                     "composer", TagLib::StringList(song.composer().c_str())));
-    tag->setItem("grouping",
-                 TagLib::APE::Item(
-                     "grouping", TagLib::StringList(song.grouping().c_str())));
-    tag->setItem("performer",
-                 TagLib::APE::Item("performer", TagLib::StringList(
-                                                    song.performer().c_str())));
-    tag->setItem(
-        "album artist",
-        TagLib::APE::Item("album artist",
-                          TagLib::StringList(song.albumartist().c_str())));
-    tag->setItem("lyrics",
-                 TagLib::APE::Item("lyrics", TagLib::String(song.lyrics())));
-    tag->addValue("compilation",
-                  QStringToTaglibString(song.compilation() ? QString::number(1)
-                                                           : QString()),
-                  true);
-  };
+  // only basic tags as provided by ripper
+  TagLib::Tag* t = fileref->tag();
+  t->setTitle(StdStringToTaglibString(song.title()));
+  t->setArtist(StdStringToTaglibString(song.artist()));
+  t->setAlbum(StdStringToTaglibString(song.album()));
+  t->setGenre(StdStringToTaglibString(song.genre()));
+  t->setComment(StdStringToTaglibString(song.comment()));
+  t->setYear(song.year() <= 0 - 1 ? 0 : song.year());
+  t->setTrack(song.track() <= 0 - 1 ? 0 : song.track());
+  // if more tags are required propertymap() shall be used
 
-  if (TagLib::MPEG::File* file =
-          dynamic_cast<TagLib::MPEG::File*>(fileref->file())) {
-    TagLib::ID3v2::Tag* tag = file->ID3v2Tag(true);
-    SetTextFrame("TPOS",
-                 song.disc() <= 0 ? 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("TOPE", song.performer(), tag);
-    SetUnsyncLyricsFrame(song.lyrics(), tag);
-    // Skip TPE1 (which is the artist) here because we already set it
-    SetTextFrame("TPE2", song.albumartist(), tag);
-    SetTextFrame("TCMP", song.compilation() ? QString::number(1) : QString(),
-                 tag);
-  } else if (TagLib::FLAC::File* file =
-                 dynamic_cast<TagLib::FLAC::File*>(fileref->file())) {
-    TagLib::Ogg::XiphComment* tag = file->xiphComment();
-    SetVorbisComments(tag, song);
-  } else if (TagLib::MP4::File* file =
-                 dynamic_cast<TagLib::MP4::File*>(fileref->file())) {
-    TagLib::MP4::Tag* tag = file->tag();
-    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().c_str());
-    tag->itemListMap()["\251grp"] = TagLib::StringList(song.grouping().c_str());
-    tag->itemListMap()["\251lyr"] = TagLib::StringList(song.lyrics().c_str());
-    tag->itemListMap()["aART"] = TagLib::StringList(song.albumartist().c_str());
-    tag->itemListMap()["cpil"] =
-        TagLib::StringList(song.compilation() ? "1" : "0");
-  } else if (TagLib::APE::File* file =
-                 dynamic_cast<TagLib::APE::File*>(fileref->file())) {
-    saveApeTag(file->APETag(true));
-  } else if (TagLib::MPC::File* file =
-                 dynamic_cast<TagLib::MPC::File*>(fileref->file())) {
-    saveApeTag(file->APETag(true));
-  } else if (TagLib::WavPack::File* file =
-                 dynamic_cast<TagLib::WavPack::File*>(fileref->file())) {
-    saveApeTag(file->APETag(true));
-  }
+  bool ret = fileref->save();
 
-  // Handle all the files which have VorbisComments (Ogg, OPUS, ...) in the same
-  // way;
-  // apart, so we keep specific behavior for some formats by adding another
-  // "else if" block above.
-  if (TagLib::Ogg::XiphComment* tag =
-          dynamic_cast<TagLib::Ogg::XiphComment*>(fileref->file()->tag())) {
-    SetVorbisComments(tag, song);
+#ifdef Q_OS_LINUX
+  if (ret) {
+    // Linux: inotify doesn't seem to notice the change to the file unless we
+    // change the timestamps as well. (this is what touch does)
+    utimensat(0, QFile::encodeName(filename).constData(), nullptr, 0);
   }
+#endif  // Q_OS_LINUX
 
+  return ret;
+}
+
+// Replacing SaveFile for single tag editting (e.g. in playlist)
+// as full store operation (especially when file was mp4) failed
+// This code performs single tag insert/update/delete and
+// relies on taglib's PropertyMap to handle file variants.
+bool TagReader::UpdateSongTag(const QString& filename, const QString& tagname,
+                              const QString& tagvalue) const {
+  // qLog(Debug) << "UpdateSongTag of " << filename << " tag " << tagname << "
+  // value " << tagvalue;
+
+  if (filename.isNull()) return false;
+
+  std::unique_ptr<TagLib::FileRef> fileref(factory_->GetFileRef(filename));
+  if (!fileref || fileref->isNull())  // The file probably doesn't exist
+    return false;
+
+  // according taglib tagwriter example
+  // to save basic attributes see SaveFile
+
+  // no check for available ones
+  TagLib::PropertyMap map = fileref->file()->properties();
+
+  TagLib::String key = QStringToTaglibString(tagname);
+
+  if (tagvalue.isEmpty())
+    map.erase(key);
+  else
+    // inserts too if not yet
+    map.replace(key, QStringToTaglibString(tagvalue));
+
+  // qLog(Debug) << TStringToQString(map.toString());
+
+  fileref->file()->setProperties(map);
   bool ret = fileref->save();
+
 #ifdef Q_OS_LINUX
   if (ret) {
     // Linux: inotify doesn't seem to notice the change to the file unless we

ext/libclementine-tagreader/tagreader.h

@@ -18,10 +18,10 @@
 #ifndef TAGREADER_H
 #define TAGREADER_H
 
+#include <taglib/xiphcomment.h>
+
 #include <QByteArray>
 #include <QFileInfo>
-
-#include <taglib/xiphcomment.h>
 #include <memory>
 
 #include "config.h"
@@ -62,6 +62,9 @@ class TagReader {
                 const pb::tagreader::SongMetadata& song) const;
   // Returns false if something went wrong; returns true otherwise (might
   // returns true if the file exists but nothing has been written inside because
+  bool UpdateSongTag(const QString& filename, const QString& tagname,
+                     const QString& tagvalue) const;
+
   // statistics tag format is not supported for this kind of file)
   bool SaveSongStatisticsToFile(const QString& filename,
                                 const pb::tagreader::SongMetadata& song) const;
@@ -97,7 +100,8 @@ class TagReader {
       const pb::tagreader::SongMetadata& song) const;
 
   void GuessArtistAndTitle(pb::tagreader::SongMetadata* song) const;
-  void GuessAlbum(const QFileInfo &info, pb::tagreader::SongMetadata* song) const;
+  void GuessAlbum(const QFileInfo& info,
+                  pb::tagreader::SongMetadata* song) const;
 
   pb::tagreader::SongMetadata_Type GuessFileType(
       TagLib::FileRef* fileref) const;

ext/libclementine-tagreader/tagreadermessages.proto

@@ -106,6 +106,16 @@ message ReadCloudFileResponse {
   optional SongMetadata metadata = 1;
 }
 
+message SaveSongTagToFileRequest {
+  optional string filename = 1;
+  optional string tagname = 2;
+  optional string tagvalue = 3;
+}
+
+message SaveSongTagToFileResponse {
+  optional bool success = 1;
+}
+
 message SaveSongStatisticsToFileRequest {
   optional string filename = 1;
   optional SongMetadata metadata = 2;
@@ -147,4 +157,8 @@ message Message {
 
   optional SaveSongRatingToFileRequest save_song_rating_to_file_request = 14;
   optional SaveSongRatingToFileResponse save_song_rating_to_file_response = 15;
+  optional SaveSongTagToFileRequest save_song_tag_to_file_request = 16;
+
+  optional SaveSongTagToFileResponse save_song_tag_to_file_response = 17;
+
 }

src/core/tagreaderclient.cpp

@@ -77,6 +77,20 @@ TagReaderReply* TagReaderClient::SaveFile(const QString& filename,
   return worker_pool_->SendMessageWithReply(&message);
 }
 
+TagReaderReply* TagReaderClient::UpdateSongTag(const QString& filename,
+                                               const QString& tagname,
+                                               const QVariant& tagvalue) {
+  pb::tagreader::Message message;
+  pb::tagreader::SaveSongTagToFileRequest* req =
+      message.mutable_save_song_tag_to_file_request();
+
+  req->set_filename(DataCommaSizeFromQString(filename));
+  req->set_tagname(DataCommaSizeFromQString(tagname));
+  req->set_tagvalue(DataCommaSizeFromQString(tagvalue.toString()));
+
+  return worker_pool_->SendMessageWithReply(&message);
+}
+
 TagReaderReply* TagReaderClient::UpdateSongStatistics(const Song& metadata) {
   pb::tagreader::Message message;
   pb::tagreader::SaveSongStatisticsToFileRequest* req =
@@ -166,6 +180,8 @@ bool TagReaderClient::SaveFileBlocking(const QString& filename,
 
   bool ret = false;
 
+  qLog(Debug) << "TagReaderClient::SaveFileBlocking: " << filename;
+
   TagReaderReply* reply = SaveFile(filename, metadata);
   if (reply->WaitForFinished()) {
     ret = reply->message().save_file_response().success();

src/core/tagreaderclient.h

@@ -46,6 +46,8 @@ class TagReaderClient : public QObject {
 
   ReplyType* ReadFile(const QString& filename);
   ReplyType* SaveFile(const QString& filename, const Song& metadata);
+  ReplyType* UpdateSongTag(const QString& filename, const QString& tagname,
+                           const QVariant& tagvalue);
   ReplyType* UpdateSongStatistics(const Song& metadata);
   ReplyType* UpdateSongRating(const Song& metadata);
   ReplyType* IsMediaFile(const QString& filename);