diff --git a/taglib/CMakeLists.txt b/taglib/CMakeLists.txt index aa304fb3..4c8e5332 100644 --- a/taglib/CMakeLists.txt +++ b/taglib/CMakeLists.txt @@ -377,7 +377,6 @@ if(WITH_MP4) mp4/mp4stem.cpp mp4/mp4itemfactory.cpp mp4/mp4chapter.cpp - mp4/mp4chapterholder.cpp mp4/mp4nerochapterlist.cpp mp4/mp4qtchapterlist.cpp ) diff --git a/taglib/mp4/mp4chapter.cpp b/taglib/mp4/mp4chapter.cpp index baa1a26c..2222748f 100644 --- a/taglib/mp4/mp4chapter.cpp +++ b/taglib/mp4/mp4chapter.cpp @@ -61,6 +61,16 @@ MP4::Chapter &MP4::Chapter::Chapter::operator=(const Chapter &other) MP4::Chapter &MP4::Chapter::Chapter::operator=( Chapter &&other) noexcept = default; +bool MP4::Chapter::operator==(const Chapter &other) const +{ + return title() == other.title() && startTime() == other.startTime(); +} + +bool MP4::Chapter::operator!=(const Chapter &other) const +{ + return !(*this == other); +} + void MP4::Chapter::swap(Chapter &other) noexcept { using std::swap; diff --git a/taglib/mp4/mp4chapter.h b/taglib/mp4/mp4chapter.h index 91627bb2..839f75d8 100644 --- a/taglib/mp4/mp4chapter.h +++ b/taglib/mp4/mp4chapter.h @@ -68,6 +68,16 @@ namespace TagLib { */ Chapter &operator=(Chapter &&other) noexcept; + /*! + * Returns \c true if the chapter and \a other contain the same data. + */ + bool operator==(const Chapter &other) const; + + /*! + * Returns \c true if the chapter and \a other differ in data. + */ + bool operator!=(const Chapter &other) const; + /*! * Exchanges the content of the object with the content of \a other. */ diff --git a/taglib/mp4/mp4chapterholder.cpp b/taglib/mp4/mp4chapterholder.cpp deleted file mode 100644 index 8d9c1ddb..00000000 --- a/taglib/mp4/mp4chapterholder.cpp +++ /dev/null @@ -1,44 +0,0 @@ -/************************************************************************** - copyright : (C) 2006 by Urs Fleisch - email : ufleisch@users.sourceforge.net - **************************************************************************/ - -/*************************************************************************** - * This library is free software; you can redistribute it and/or modify * - * it under the terms of the GNU Lesser General Public License version * - * 2.1 as published by the Free Software Foundation. * - * * - * This library is distributed in the hope that it will be useful, but * - * WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * - * Lesser General Public License for more details. * - * * - * You should have received a copy of the GNU Lesser General Public * - * License along with this library; if not, write to the Free Software * - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * - * 02110-1301 USA * - * * - * Alternatively, this file is available under the Mozilla Public * - * License Version 1.1. You may obtain a copy of the License at * - * http://www.mozilla.org/MPL/ * - ***************************************************************************/ - -#include "mp4chapterholder.h" - -using namespace TagLib; - -MP4::ChapterList MP4::ChapterHolder::chapters() const -{ - return chapterList; -} - -void MP4::ChapterHolder::setChapters(const ChapterList &chapters) -{ - chapterList = chapters; - modified = true; -} - -bool MP4::ChapterHolder::isModified() const -{ - return modified; -} diff --git a/taglib/mp4/mp4chapterholder.h b/taglib/mp4/mp4chapterholder.h index 94e8a52f..72752ba4 100644 --- a/taglib/mp4/mp4chapterholder.h +++ b/taglib/mp4/mp4chapterholder.h @@ -39,17 +39,22 @@ namespace TagLib { /*! * Get list of chapters. */ - ChapterList chapters() const; + ChapterList chapters() const { return chapterList; } /*! * Set list of chapters. */ - void setChapters(const ChapterList &chapters); + void setChapters(const ChapterList &chapters) { chapterList = chapters; } /*! * Returns \c true if the list of chapters has been modified. */ - bool isModified() const; + bool isModified() const { return modified; } + + /*! + * Set if the contained chapters are modified. + */ + void setModified(bool chaptersModified) { modified = chaptersModified; } protected: ChapterList chapterList; @@ -84,8 +89,15 @@ namespace TagLib { { if (!holder) { holder = std::make_unique(); + // The chapters have not been read before, so we do not know their + // current state and mark them as modified. Otherwise, the check below + // would not set the chapters if they are empty. + holder->setModified(true); + } + if(holder->isModified() || holder->chapters() != chapters) { + holder->setChapters(chapters); + holder->setModified(true); } - holder->setChapters(chapters); } /*! @@ -99,7 +111,11 @@ namespace TagLib { bool saveChaptersIfModified(std::unique_ptr &holder, TagLib::File *file) { if(holder && holder->isModified()) { - return holder->write(file); + if(holder->write(file)) { + holder->setModified(false); + return true; + } + return false; } return true; } diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index dc310385..d037a14f 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -34,6 +34,7 @@ #include "mp4atom.h" #include "mp4file.h" #include "mp4itemfactory.h" +#include "mp4chapterholder.h" #include "plainfile.h" #include #include "utils.h" @@ -69,6 +70,56 @@ namespace }; CustomItemFactory CustomItemFactory::factory; + + class MockChapterList : public MP4::ChapterHolder { + public: + static const MP4::ChapterList mockChapters; + + bool read(TagLib::File *) + { + chapterList = mockChapters; + ++readCount; + return true; + } + + bool write(TagLib::File *) + { + ++writeCount; + return true; + } + + int readCount = 0; + int writeCount = 0; + }; + + const MP4::ChapterList MockChapterList::mockChapters = { + MP4::Chapter("Mock", 123) + }; + + class MockChapterFile : public PlainFile { + public: + explicit MockChapterFile(FileName name) : PlainFile(name) + { + } + + MP4::ChapterList chapters() + { + return getChaptersLazy(chapterList, this); + } + + void setChapters(const MP4::ChapterList& chapters) + { + setChaptersLazy(chapterList, chapters); + } + + bool save() override + { + return MP4::saveChaptersIfModified(chapterList, this); + } + + std::unique_ptr chapterList; + }; + } // namespace class TestMP4 : public CppUnit::TestFixture @@ -121,6 +172,7 @@ class TestMP4 : public CppUnit::TestFixture CPPUNIT_TEST(testQTChapterListSingleEmptyTitleNotStripped); CPPUNIT_TEST(testNeroAndQTChaptersAreIndependent); CPPUNIT_TEST(testNeroChaptersAloneWhenNoQT); + CPPUNIT_TEST(testLazyReadingAndWritingChapters); CPPUNIT_TEST_SUITE_END(); public: @@ -1634,6 +1686,105 @@ public: } } + void testLazyReadingAndWritingChapters() + { + // No reads or writes if chapters are not used + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + f.save(); + CPPUNIT_ASSERT(!f.chapterList); + } + // Do not read if already read, do not write if not modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + auto chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->writeCount); + } + // Do not write if not modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + auto chapters = f.chapters(); + CPPUNIT_ASSERT(chapters == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + f.setChapters(MockChapterList::mockChapters); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->writeCount); + } + // Write if set without being read before + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + f.setChapters(MP4::ChapterList()); + f.save(); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(0, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + } + // Do write if modified + { + MockChapterFile f(TEST_FILE_PATH_C("no-tags.m4a")); + CPPUNIT_ASSERT(f.chapters() == MockChapterList::mockChapters); + CPPUNIT_ASSERT(f.chapterList); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + const auto chapters1 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + }); + f.setChapters(chapters1); + CPPUNIT_ASSERT(f.chapters() == chapters1); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters1); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->readCount); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + f.setChapters(chapters1); + f.save(); + CPPUNIT_ASSERT_EQUAL(1, f.chapterList->writeCount); + auto chapters2 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 2", 1), + }); + f.setChapters(chapters2); + CPPUNIT_ASSERT(f.chapters() == chapters2); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters2); + CPPUNIT_ASSERT_EQUAL(2, f.chapterList->writeCount); + chapters2 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 2", 2), + }); + f.setChapters(chapters2); + f.save(); + CPPUNIT_ASSERT_EQUAL(3, f.chapterList->writeCount); + f.setChapters(chapters2); + CPPUNIT_ASSERT(f.chapters() == chapters2); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters2); + CPPUNIT_ASSERT_EQUAL(3, f.chapterList->writeCount); + const auto chapters3 = MP4::ChapterList({ + MP4::Chapter("Chapter 1", 0), + MP4::Chapter("Chapter 3", 2), + }); + f.setChapters(chapters3); + CPPUNIT_ASSERT(f.chapters() == chapters3); + f.save(); + CPPUNIT_ASSERT(f.chapters() == chapters3); + CPPUNIT_ASSERT_EQUAL(4, f.chapterList->writeCount); + f.setChapters(MP4::ChapterList()); + CPPUNIT_ASSERT(f.chapters().isEmpty()); + f.save(); + CPPUNIT_ASSERT(f.chapters().isEmpty()); + CPPUNIT_ASSERT_EQUAL(5, f.chapterList->writeCount); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4);