From c61d866cafa53a2b01d56cd8cbe19156e11fbe0e Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Fri, 16 Jan 2015 18:30:11 +0100 Subject: [PATCH 1/3] Fix album artist for FLAC/OGG files (with vorbis comments) --- ext/libclementine-tagreader/tagreader.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/ext/libclementine-tagreader/tagreader.cpp b/ext/libclementine-tagreader/tagreader.cpp index 5a35d680a..2f6ca4a39 100644 --- a/ext/libclementine-tagreader/tagreader.cpp +++ b/ext/libclementine-tagreader/tagreader.cpp @@ -502,10 +502,9 @@ void TagReader::ParseOggTag(const TagLib::Ogg::FieldListMap& map, 100); } -void TagReader::SetVorbisComments(TagLib::Ogg::XiphComment* vorbis_comments, - const pb::tagreader::SongMetadata& song) - const { - +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", @@ -524,6 +523,9 @@ void TagReader::SetVorbisComments(TagLib::Ogg::XiphComment* vorbis_comments, vorbis_comments->addField( "COMPILATION", StdStringToTaglibString(song.compilation() ? "1" : "0"), true); + + vorbis_comments->addField("ALBUM ARTIST", + StdStringToTaglibString(song.albumartist()), true); } void TagReader::SetFMPSStatisticsVorbisComments( @@ -540,7 +542,6 @@ void TagReader::SetFMPSStatisticsVorbisComments( void TagReader::SetFMPSRatingVorbisComments( TagLib::Ogg::XiphComment* vorbis_comments, const pb::tagreader::SongMetadata& song) const { - vorbis_comments->addField( "FMPS_RATING", QStringToTaglibString(QString::number(song.rating()))); } @@ -953,8 +954,8 @@ bool TagReader::ReadCloudFile(const QUrl& download_url, const QString& title, pb::tagreader::SongMetadata* song) const { qLog(Debug) << "Loading tags from" << title; - std::unique_ptr stream( - new CloudStream(download_url, title, size, authorisation_header, network_)); + std::unique_ptr stream(new CloudStream( + download_url, title, size, authorisation_header, network_)); stream->Precache(); std::unique_ptr tag; if (mime_type == "audio/mpeg" && title.endsWith(".mp3")) { @@ -963,8 +964,8 @@ bool TagReader::ReadCloudFile(const QUrl& download_url, const QString& title, TagLib::AudioProperties::Accurate)); } else if (mime_type == "audio/mp4" || (mime_type == "audio/mpeg" && title.endsWith(".m4a"))) { - tag.reset( - new TagLib::MP4::File(stream.get(), true, TagLib::AudioProperties::Accurate)); + tag.reset(new TagLib::MP4::File(stream.get(), true, + TagLib::AudioProperties::Accurate)); } #ifdef TAGLIB_HAS_OPUS else if ((mime_type == "application/opus" || mime_type == "audio/opus" || @@ -983,8 +984,8 @@ bool TagReader::ReadCloudFile(const QUrl& download_url, const QString& title, TagLib::ID3v2::FrameFactory::instance(), true, TagLib::AudioProperties::Accurate)); } else if (mime_type == "audio/x-ms-wma") { - tag.reset( - new TagLib::ASF::File(stream.get(), true, TagLib::AudioProperties::Accurate)); + tag.reset(new TagLib::ASF::File(stream.get(), true, + TagLib::AudioProperties::Accurate)); } else { qLog(Debug) << "Unknown mime type for tagging:" << mime_type; return false; From 2a7d4e6d5ef6d4baf8927e6af46f13ffc388c217 Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Sat, 17 Jan 2015 11:31:02 +0100 Subject: [PATCH 2/3] Improve edit tag when set values --- src/ui/edittagdialog.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/ui/edittagdialog.cpp b/src/ui/edittagdialog.cpp index 253f367e6..5e9c044b9 100644 --- a/src/ui/edittagdialog.cpp +++ b/src/ui/edittagdialog.cpp @@ -314,17 +314,18 @@ QVariant EditTagDialog::Data::value(const Song& song, const QString& id) { void EditTagDialog::Data::set_value(const QString& id, const QVariant& value) { if (id == "title") current_.set_title(value.toString()); - if (id == "artist") current_.set_artist(value.toString()); - if (id == "album") current_.set_album(value.toString()); - if (id == "albumartist") current_.set_albumartist(value.toString()); - if (id == "composer") current_.set_composer(value.toString()); - if (id == "performer") current_.set_performer(value.toString()); - if (id == "grouping") current_.set_grouping(value.toString()); - if (id == "genre") current_.set_genre(value.toString()); - if (id == "comment") current_.set_comment(value.toString()); - if (id == "track") current_.set_track(value.toInt()); - if (id == "disc") current_.set_disc(value.toInt()); - if (id == "year") current_.set_year(value.toInt()); + else if (id == "artist") current_.set_artist(value.toString()); + else if (id == "album") current_.set_album(value.toString()); + else if (id == "albumartist") current_.set_albumartist(value.toString()); + else if (id == "composer") current_.set_composer(value.toString()); + else if (id == "performer") current_.set_performer(value.toString()); + else if (id == "grouping") current_.set_grouping(value.toString()); + else if (id == "genre") current_.set_genre(value.toString()); + else if (id == "comment") current_.set_comment(value.toString()); + else if (id == "track") current_.set_track(value.toInt()); + else if (id == "disc") current_.set_disc(value.toInt()); + else if (id == "year") current_.set_year(value.toInt()); + else qLog(Warning) << "Unknown ID" << id; } bool EditTagDialog::DoesValueVary(const QModelIndexList& sel, From b96c1f060fbc9f703e436ab0bf656481e8725c9c Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Sat, 17 Jan 2015 12:11:33 +0100 Subject: [PATCH 3/3] Add tests to check the tag edition of FLAC/OGG files and make format --- src/ui/edittagdialog.cpp | 54 ++++++++++++++--------- tests/song_test.cpp | 95 ++++++++++++++++++++++++++++++++++------ 2 files changed, 115 insertions(+), 34 deletions(-) diff --git a/src/ui/edittagdialog.cpp b/src/ui/edittagdialog.cpp index 5e9c044b9..a69c67fbe 100644 --- a/src/ui/edittagdialog.cpp +++ b/src/ui/edittagdialog.cpp @@ -216,8 +216,8 @@ bool EditTagDialog::SetLoading(const QString& message) { return true; } -QList EditTagDialog::LoadData(const SongList& songs) - const { +QList EditTagDialog::LoadData( + const SongList& songs) const { QList ret; for (const Song& song : songs) { @@ -246,17 +246,16 @@ void EditTagDialog::SetSongs(const SongList& s, const PlaylistItemList& items) { ui_->song_list->clear(); // Reload tags in the background - QFuture > future = + QFuture> future = QtConcurrent::run(this, &EditTagDialog::LoadData, s); - QFutureWatcher >* watcher = - new QFutureWatcher >(this); + QFutureWatcher>* watcher = new QFutureWatcher>(this); watcher->setFuture(future); connect(watcher, SIGNAL(finished()), SLOT(SetSongsFinished())); } void EditTagDialog::SetSongsFinished() { - QFutureWatcher >* watcher = - dynamic_cast >*>(sender()); + QFutureWatcher>* watcher = + dynamic_cast>*>(sender()); if (!watcher) return; watcher->deleteLater(); @@ -313,19 +312,32 @@ QVariant EditTagDialog::Data::value(const Song& song, const QString& id) { } void EditTagDialog::Data::set_value(const QString& id, const QVariant& value) { - if (id == "title") current_.set_title(value.toString()); - else if (id == "artist") current_.set_artist(value.toString()); - else if (id == "album") current_.set_album(value.toString()); - else if (id == "albumartist") current_.set_albumartist(value.toString()); - else if (id == "composer") current_.set_composer(value.toString()); - else if (id == "performer") current_.set_performer(value.toString()); - else if (id == "grouping") current_.set_grouping(value.toString()); - else if (id == "genre") current_.set_genre(value.toString()); - else if (id == "comment") current_.set_comment(value.toString()); - else if (id == "track") current_.set_track(value.toInt()); - else if (id == "disc") current_.set_disc(value.toInt()); - else if (id == "year") current_.set_year(value.toInt()); - else qLog(Warning) << "Unknown ID" << id; + if (id == "title") + current_.set_title(value.toString()); + else if (id == "artist") + current_.set_artist(value.toString()); + else if (id == "album") + current_.set_album(value.toString()); + else if (id == "albumartist") + current_.set_albumartist(value.toString()); + else if (id == "composer") + current_.set_composer(value.toString()); + else if (id == "performer") + current_.set_performer(value.toString()); + else if (id == "grouping") + current_.set_grouping(value.toString()); + else if (id == "genre") + current_.set_genre(value.toString()); + else if (id == "comment") + current_.set_comment(value.toString()); + else if (id == "track") + current_.set_track(value.toInt()); + else if (id == "disc") + current_.set_disc(value.toInt()); + else if (id == "year") + current_.set_year(value.toInt()); + else + qLog(Warning) << "Unknown ID" << id; } bool EditTagDialog::DoesValueVary(const QModelIndexList& sel, @@ -676,7 +688,7 @@ void EditTagDialog::SaveData(const QList& data) { if (ref.current_.IsMetadataEqual(ref.original_)) continue; if (!TagReaderClient::Instance()->SaveFileBlocking( - ref.current_.url().toLocalFile(), ref.current_)) { + ref.current_.url().toLocalFile(), ref.current_)) { emit Error(tr("An error occurred writing metadata to '%1'") .arg(ref.current_.url().toLocalFile())); } diff --git a/tests/song_test.cpp b/tests/song_test.cpp index 65eac3c6a..4571ab84c 100644 --- a/tests/song_test.cpp +++ b/tests/song_test.cpp @@ -19,7 +19,7 @@ #include "tagreader.h" #include "core/song.h" #ifdef HAVE_LIBLASTFM - #include "internet/lastfm/lastfmcompat.h" +#include "internet/lastfm/lastfmcompat.h" #endif #include "gmock/gmock.h" @@ -64,7 +64,8 @@ class SongTest : public ::testing::Test { tag_reader.SaveFile(filename, pb_song); } - static void WriteSongStatisticsToFile(const Song& song, const QString& filename) { + static void WriteSongStatisticsToFile(const Song& song, + const QString& filename) { TagReader tag_reader; ::pb::tagreader::SongMetadata pb_song; song.ToProtobuf(&pb_song); @@ -79,7 +80,6 @@ class SongTest : public ::testing::Test { } }; - #ifdef HAVE_LIBLASTFM TEST_F(SongTest, InitsFromLastFM) { Song song; @@ -95,7 +95,7 @@ TEST_F(SongTest, InitsFromLastFM) { EXPECT_EQ("Baz", song.album()); EXPECT_EQ("Bar", song.artist()); } -#endif // HAVE_LIBLASTFM +#endif // HAVE_LIBLASTFM /*TEST_F(SongTest, InitsFromFile) { QTemporaryFile temp; @@ -156,14 +156,13 @@ TEST_F(SongTest, FMPSPlayCountBoth) { TEST_F(SongTest, FMPSUnrated) { QStringList files_to_test; - files_to_test << - ":/testdata/beep.m4a" << - ":/testdata/beep.mp3" << - ":/testdata/beep.flac" << - ":/testdata/beep.ogg" << - ":/testdata/beep.spx" << - ":/testdata/beep.wav" << - ":/testdata/beep.wma"; + files_to_test << ":/testdata/beep.m4a" + << ":/testdata/beep.mp3" + << ":/testdata/beep.flac" + << ":/testdata/beep.ogg" + << ":/testdata/beep.spx" + << ":/testdata/beep.wav" + << ":/testdata/beep.wma"; for (const QString& test_filename : files_to_test) { TemporaryResource r(test_filename); Song song = ReadSongFromFile(r.fileName()); @@ -240,6 +239,41 @@ TEST_F(SongTest, StatisticsOgg) { EXPECT_EQ(87, new_song.score()); } +TEST_F(SongTest, TagsOgg) { + TemporaryResource r(":/testdata/beep.ogg"); + { + Song song = ReadSongFromFile(r.fileName()); + song.set_title("beep title"); + song.set_artist("beep artist"); + song.set_album("beep album"); + song.set_albumartist("beep album artist"); + song.set_composer("beep composer"); + song.set_performer("beep performer"); + song.set_grouping("beep grouping"); + song.set_genre("beep genre"); + song.set_comment("beep comment"); + song.set_track(12); + song.set_disc(1234); + song.set_year(2015); + + WriteSongToFile(song, r.fileName()); + } + + Song new_song = ReadSongFromFile(r.fileName()); + EXPECT_EQ("beep title", new_song.title()); + EXPECT_EQ("beep artist", new_song.artist()); + EXPECT_EQ("beep album", new_song.album()); + EXPECT_EQ("beep album artist", new_song.albumartist()); + EXPECT_EQ("beep composer", new_song.composer()); + EXPECT_EQ("beep performer", new_song.performer()); + EXPECT_EQ("beep grouping", new_song.grouping()); + EXPECT_EQ("beep genre", new_song.genre()); + EXPECT_EQ("beep comment", new_song.comment()); + EXPECT_EQ(12, new_song.track()); + EXPECT_EQ(1234, new_song.disc()); + EXPECT_EQ(2015, new_song.year()); +} + TEST_F(SongTest, RatingFLAC) { TemporaryResource r(":/testdata/beep.flac"); { @@ -267,6 +301,41 @@ TEST_F(SongTest, StatisticsFLAC) { EXPECT_EQ(87, new_song.score()); } +TEST_F(SongTest, TagsFLAC) { + TemporaryResource r(":/testdata/beep.flac"); + { + Song song = ReadSongFromFile(r.fileName()); + song.set_title("beep title"); + song.set_artist("beep artist"); + song.set_album("beep album"); + song.set_albumartist("beep album artist"); + song.set_composer("beep composer"); + song.set_performer("beep performer"); + song.set_grouping("beep grouping"); + song.set_genre("beep genre"); + song.set_comment("beep comment"); + song.set_track(12); + song.set_disc(1234); + song.set_year(2015); + + WriteSongToFile(song, r.fileName()); + } + + Song new_song = ReadSongFromFile(r.fileName()); + EXPECT_EQ("beep title", new_song.title()); + EXPECT_EQ("beep artist", new_song.artist()); + EXPECT_EQ("beep album", new_song.album()); + EXPECT_EQ("beep album artist", new_song.albumartist()); + EXPECT_EQ("beep composer", new_song.composer()); + EXPECT_EQ("beep performer", new_song.performer()); + EXPECT_EQ("beep grouping", new_song.grouping()); + EXPECT_EQ("beep genre", new_song.genre()); + EXPECT_EQ("beep comment", new_song.comment()); + EXPECT_EQ(12, new_song.track()); + EXPECT_EQ(1234, new_song.disc()); + EXPECT_EQ(2015, new_song.year()); +} + #ifdef TAGLIB_WITH_ASF TEST_F(SongTest, RatingASF) { TemporaryResource r(":/testdata/beep.wma"); @@ -295,7 +364,7 @@ TEST_F(SongTest, StatisticsASF) { EXPECT_EQ(1337, new_song.playcount()); EXPECT_EQ(87, new_song.score()); } -#endif // TAGLIB_WITH_ASF +#endif // TAGLIB_WITH_ASF TEST_F(SongTest, RatingMP4) { TemporaryResource r(":/testdata/beep.m4a");