From 434becdf8ac81b50abe635a7c5bc8263d61dbcc9 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 29 Jan 2024 14:24:27 +0100 Subject: [PATCH] fixup! Introduce `migrateBookmarks` to move (invalid) bookmarks to new books. --- include/library.h | 9 +++++++-- src/library.cpp | 12 ++++++------ test/library.cpp | 11 ++++------- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/include/library.h b/include/library.h index 5804c373c..6aa36ff4a 100644 --- a/include/library.h +++ b/include/library.h @@ -55,6 +55,11 @@ enum supportedListMode { NOVALID = 1 << 5 }; +enum MigrationMode { + UPGRADE_ONLY = 0, + ALLOW_DOWNGRADE = 1, + }; + class Filter { public: // types using Tags = std::vector; @@ -283,7 +288,7 @@ class Library: public std::enable_shared_from_this * @param allowDowngrade `true` if "heuristics" can found a book older than the currently set. * @return The number of bookmarks updated. */ - int migrateBookmarks(const std::string& source, bool allowDowngrade = false); + int migrateBookmarks(const std::string& source, MigrationMode allowDowngrade = UPGRADE_ONLY); /** * Migrate bookmarks @@ -440,7 +445,7 @@ class Library: public std::enable_shared_from_this AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; std::vector getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; - std::string getBestTargetBookId(const Bookmark& bookmark, bool allowDowngrade) const; + std::string getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const; unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); diff --git a/src/library.cpp b/src/library.cpp index 08c4a3192..eb8369e95 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -169,12 +169,12 @@ int Library::migrateBookmarks() { } int changed = 0; for(auto& sourceBook:sourceBooks) { - changed += migrateBookmarks(sourceBook, true); + changed += migrateBookmarks(sourceBook, ALLOW_DOWNGRADE); } return changed; } -std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDowngrade) const { +std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const { // Search for a existing book with the same name auto book_filter = Filter(); if (!bookmark.getBookName().empty()) { @@ -194,7 +194,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow // Remove source book auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId()); if (foundIT != targetBooks.end()) { - if (allowDowngrade) { + if (migrationMode == ALLOW_DOWNGRADE) { targetBooks.erase(foundIT); } else { // targetBooks is sorted by date, remove the source and all oldest books @@ -213,7 +213,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow // Remove source book auto foundIT = std::find(flavouredTargetBooks.begin(), flavouredTargetBooks.end(), bookmark.getBookId()); if (foundIT != flavouredTargetBooks.end()) { - if (allowDowngrade) { + if (migrationMode == ALLOW_DOWNGRADE) { flavouredTargetBooks.erase(foundIT); } else { // targetBooks is sorted by date, remove the source and all oldest books @@ -227,7 +227,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow return targetBooks[0]; } -int Library::migrateBookmarks(const std::string& source, bool allowDowngrade) { +int Library::migrateBookmarks(const std::string& source, MigrationMode migrationMode) { std::lock_guard lock(m_mutex); Bookmark firstBookmarkToChange; @@ -242,7 +242,7 @@ int Library::migrateBookmarks(const std::string& source, bool allowDowngrade) { return 0; } - std::string betterBook = getBestTargetBookId(firstBookmarkToChange, allowDowngrade); + std::string betterBook = getBestTargetBookId(firstBookmarkToChange, migrationMode); if (betterBook.empty()) { return 0; diff --git a/test/library.cpp b/test/library.cpp index 7f7813314..0ae4d3290 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -569,12 +569,9 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour"); - // Suprisingly, `char *` make c++ prefers `migrateBookmarks(..., bool)` instead of `std::string`. - // [??] Is is a problem ? It should appear only if user use `char *` (so use C or literal string). - // A solution would be to rename one method, but which one ? - ASSERT_EQ(lib->migrateBookmarks(bookId1, std::string(bookId2)), 0); // No more bookId1 bookmark + ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 0); // No more bookId1 bookmark - ASSERT_EQ(lib->migrateBookmarks(bookId1+"_date", std::string(bookId2)), 2); + ASSERT_EQ(lib->migrateBookmarks(bookId1+"_date", bookId2), 2); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); @@ -593,7 +590,7 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour"); - ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", std::string(bookId1)), 1); + ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", bookId1), 1); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); @@ -630,7 +627,7 @@ TEST_F(LibraryTest, MigrateBookmarkOlder) EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); - ASSERT_EQ(lib->migrateBookmarks(bookId1, true), 1); + ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1); }