[New] Do not migrate bookmarks to an older book.

At least, it must be explicitly asked by the user.

include/library.h

@@ -280,9 +280,10 @@ class Library: public std::enable_shared_from_this<Library>
    * "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<Library>
   AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const;
   std::vector<std::string> 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);

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<std::recursive_mutex> 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;
   }

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-bookmark-id", bookId1), 1);
+    ASSERT_EQ(lib->migrateBookmarks("invalid-bookmark-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)
 {