From ce332f5b4506649e48ff6a018fb7811dc41ea2d8 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 22 Jan 2024 15:51:42 +0100 Subject: [PATCH] [New] Do not migrate bookmarks to an older book. At least, it must be explicitly asked by the user. --- include/library.h | 5 +++-- src/library.cpp | 31 ++++++++++++++++++++++++------- test/library.cpp | 26 +++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/include/library.h b/include/library.h index efa932968..afbdea129 100644 --- a/include/library.h +++ b/include/library.h @@ -280,9 +280,10 @@ class Library: public std::enable_shared_from_this * "Better book", will be determined using heuristics based on book name, flavour and date. * @param source the source bookId of the bookmarks to migrate. + * @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); + int migrateBookmarks(const std::string& source, bool allowDowngrade = false); /** * Migrate bookmarks @@ -439,7 +440,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) const; + std::string getBestTargetBookId(const Bookmark& bookmark, bool allowDowngrade) 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 fa09ea7a3..5ed0f6b82 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); + changed += migrateBookmarks(sourceBook, true); } return changed; } -std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { +std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDowngrade) const { // Search for a existing book with the same name auto book_filter = Filter(); if (!bookmark.getBookName().empty()) { @@ -189,10 +189,17 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { book_filter.query("title:\""+bookmark.getBookTitle() + "\""); } auto targetBooks = filter(book_filter); + sort(targetBooks, DATE, false); + // Remove source book auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId()); if (foundIT != targetBooks.end()) { - targetBooks.erase(foundIT); + if (allowDowngrade) { + targetBooks.erase(foundIT); + } else { + // targetBooks is sorted by date, remove the source and all oldest books + targetBooks.erase(foundIT, targetBooks.end()); + } } if (targetBooks.empty()) { @@ -202,15 +209,25 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { if (targetBooks.size() != 1) { // We have several, reduce to same flavour auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour())); + sort(flavouredTargetBooks, DATE, false); + // Remove source book + auto foundIT = std::find(flavouredTargetBooks.begin(), flavouredTargetBooks.end(), bookmark.getBookId()); + if (foundIT != flavouredTargetBooks.end()) { + if (allowDowngrade) { + flavouredTargetBooks.erase(foundIT); + } else { + // targetBooks is sorted by date, remove the source and all oldest books + flavouredTargetBooks.erase(foundIT, flavouredTargetBooks.end()); + } + } if (!flavouredTargetBooks.empty()) { targetBooks = flavouredTargetBooks; } - sort(targetBooks, DATE, false); } return targetBooks[0]; } -int Library::migrateBookmarks(const std::string& source) { +int Library::migrateBookmarks(const std::string& source, bool allowDowngrade) { std::lock_guard lock(m_mutex); Bookmark firstBookmarkToChange; @@ -225,8 +242,8 @@ int Library::migrateBookmarks(const std::string& source) { return 0; } - std::string betterBook = getBestTargetBookId(firstBookmarkToChange); - + std::string betterBook = getBestTargetBookId(firstBookmarkToChange, allowDowngrade); + if (betterBook.empty()) { return 0; } diff --git a/test/library.cpp b/test/library.cpp index 2712df5cd..7f7813314 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -569,9 +569,12 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour"); - ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 0); // No more bookId1 bookmark + // 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+"_date", bookId2), 2); + ASSERT_EQ(lib->migrateBookmarks(bookId1+"_date", std::string(bookId2)), 2); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); @@ -590,7 +593,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", bookId1), 1); + ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", std::string(bookId1)), 1); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); @@ -613,6 +616,23 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour"); } +TEST_F(LibraryTest, MigrateBookmarkOlder) +{ + auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_date"; + + auto book1 = lib->getBookById(bookId1); + + lib->addBookmark(createBookmark(book1)); + + auto onlyValidBookmarks = lib->getBookmarks(); + + ASSERT_EQ(onlyValidBookmarks.size(), 1); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); + + ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); + ASSERT_EQ(lib->migrateBookmarks(bookId1, true), 1); +} + TEST_F(LibraryTest, sanityCheck) {