Migrate bookmarks between books #1043

mgautierfr · 2024-01-19T16:01:22Z

Contrary to suggested in #1037 (comment), we don't create new bookmark but update existing one (we don't delete bookmark).

If we create a new one without removing the wrong ones, we would create new bookmarks on each migration.

codecov · 2024-01-19T16:16:31Z

Codecov Report

Attention: Patch coverage is 53.54331% with 59 lines in your changes missing coverage. Please review.

Project coverage is 41.03%. Comparing base (c768d05) to head (6b05eeb).
Report is 116 commits behind head on main.

Files with missing lines	Patch %	Lines
src/library.cpp	52.33%	3 Missing and 48 partials ⚠️
src/libxml_dumper.cpp	0.00%	0 Missing and 5 partials ⚠️
src/bookmark.cpp	70.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
+ Coverage   39.36%   41.03%   +1.66%     
==========================================
  Files          58       58              
  Lines        4075     4187     +112     
  Branches     2245     2303      +58     
==========================================
+ Hits         1604     1718     +114     
+ Misses       1091     1003      -88     
- Partials     1380     1466      +86

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/library.cpp

include/library.h

test/library.cpp

src/library.cpp

mgautierfr · 2024-01-22T15:50:13Z

Ready for a new review. New commit are either fixup! or marked with [New].

There is an open question about overload resolution between migrateBookmarks(const std::string& source, bool allowDowngrade) and migrateBookmarks(const std::string& source const std::string& target).
If used with a char* (as a literal string), cpp resolves overload to use the bool version.

veloman-yunkan

Please rebase&fixup for the next review

include/library.h

src/library.cpp

include/library.h

test/library.cpp

kelson42 · 2024-01-26T06:57:57Z

@BPerlakiH Looks good to you (in terme of feature and interface)?

BPerlakiH · 2024-01-26T22:39:06Z

@BPerlakiH Looks good to you (in terme of feature and interface)?

Yes, initially it looks good to me.

test/library.cpp

src/library.cpp

include/library.h

kelson42 · 2024-02-06T17:49:53Z

@mgautierfr It‘s important that the piece of code detecting if a new book is available (to amke a bookmark/note update) is public and dedicated, so we can move notes on kiwix android the same way (but in kotlin), see kiwix/kiwix-android#3632

mgautierfr · 2024-02-07T16:36:03Z

Ready for new review.
Previous fixup! have been already fixup.
New commits start from 7db8e03

src/library.cpp

include/library.h

src/library.cpp

mgautierfr · 2024-02-12T16:15:57Z

@mgautierfr It‘s important that the piece of code detecting if a new book is available (to amke a bookmark/note update) is public and dedicated, so we can move notes on kiwix android the same way (but in kotlin), see kiwix/kiwix-android#3632

I have tried to make that by making our internal (new) method public, but in fact detecting that a new book is available is not the same thing that we are doing here.

The PR is trying to resolve invalid bookmark, not migrate valid bookmark to new ones.
It seems pretty close at first sight, but it is not the same.
As @veloman-yunkan, it triggers some inconsistency. I've tried to mitigate that with documentation, but it may not be enough.

mgautierfr · 2024-02-13T15:47:19Z

Rebased on main and previous fixup commits have been fixed up.

New commits start at d41a9ad

The questions about surprising behavior of migrationMode have been fixed (hopefully)

veloman-yunkan

I only looked at the new commits.
For the next (hopefully final) review please fully rebase & fixup the PR (including any fixes addressing the new comments).

src/library.cpp

test/library.cpp

veloman-yunkan

OK, I decided to perform a full review on the final changes (rather than commit-wise) in order to increase the chances of the next review being the last one. Here are a few more things that caught my eye:

include/library.h

test/library.cpp

mgautierfr · 2024-02-14T16:43:29Z

Ready for another review. All commits are fixed-up.
I have closed all discussion taken into account.
We have 3 left open discussions. We could go with the current code if we agree or we will need another round depending of our your/@kelson42 feedback.

(macOs failing is new and not related to this PR, seems to be a (broken) update in homebrew)

mgautierfr · 2024-02-15T11:18:39Z

Next round of review.

I have rewritten the algorithm using input from @veloman-yunkan. It is indeed simpler.
fixup! commits have been inserted in the git history to reduce potential conflict at next rebase-fixup.

veloman-yunkan

This is a pre-approval.

Please include the proposed final touches in the rebase&fixup and let there be green light!

include/library.h

src/library.cpp

mgautierfr · 2024-02-15T13:23:06Z

Please include the proposed final touches in the rebase&fixup and let there be green light!

Done

This allow us to internally call thread_safe function from already locked context.

We will need them to test flavour/date bookmarks migration.

If there is no book with the same flavour, but book with same name and different flavour, we do the migration to the other book.

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

On top of modifying the existing test, the commit also make `MigrateBookmark` test fails as `migrateBookmarks` now migrates from `wrong-book-id-noname` to `Dummy id`. Fix will be provided in next commit.

At indexation time, double quote are ignored, so a title as `TED "talks" - Business` is indexed as `ted talks business`. By removing the quotes, we ensure that our title "phrase" is not closed too early and we correctly search for `ted PHRASE talks PHRASE business` instead of `ted AND talks AND business`.

`MigrationMode` was kind of defined in the context of an internal mode used by `migrateBookmark(...)`. But now, with `getBestTargetBookId`, it is broken. This commit fix that and the associated implementation. Now `UPGRADE_ONLY` will make `getBestTargetBookId` return only newer books. and `ALLOW_DOWNGRADE` will return older books only if current book is invalid.

mgautierfr requested a review from veloman-yunkan January 19, 2024 16:01

mgautierfr force-pushed the bookmarks_migrations branch from 2d3feca to 0a9584c Compare January 19, 2024 16:09

kelson42 self-requested a review January 20, 2024 08:00

veloman-yunkan requested changes Jan 20, 2024

View reviewed changes

mgautierfr force-pushed the bookmarks_migrations branch 2 times, most recently from 80b5b1c to 80a680a Compare January 22, 2024 15:47

mgautierfr requested a review from veloman-yunkan January 22, 2024 15:47

veloman-yunkan reviewed Jan 23, 2024

View reviewed changes

include/library.h Outdated Show resolved Hide resolved

include/library.h Outdated Show resolved Hide resolved

src/library.cpp Outdated Show resolved Hide resolved

include/library.h Outdated Show resolved Hide resolved

test/library.cpp Outdated Show resolved Hide resolved

rgaudin mentioned this pull request Jan 27, 2024

Handling bookmarks from previous zim files kiwix/kiwix-apple#601

Open

mgautierfr force-pushed the bookmarks_migrations branch from 80a680a to 434becd Compare January 29, 2024 13:25

mgautierfr requested a review from veloman-yunkan January 29, 2024 13:25

veloman-yunkan requested changes Jan 29, 2024

View reviewed changes

mgautierfr force-pushed the bookmarks_migrations branch from 434becd to 9196479 Compare February 7, 2024 16:34

mgautierfr requested a review from veloman-yunkan February 7, 2024 16:36

veloman-yunkan requested changes Feb 8, 2024

View reviewed changes

mgautierfr force-pushed the bookmarks_migrations branch from 9196479 to 9ae7f7c Compare February 13, 2024 15:44

mgautierfr requested a review from veloman-yunkan February 14, 2024 09:52

veloman-yunkan requested changes Feb 14, 2024

View reviewed changes

mgautierfr force-pushed the bookmarks_migrations branch from 9ae7f7c to 39134f6 Compare February 14, 2024 16:39

mgautierfr requested a review from veloman-yunkan February 14, 2024 16:43

mgautierfr force-pushed the bookmarks_migrations branch from 39134f6 to 05e1b6b Compare February 15, 2024 11:03

mgautierfr requested review from veloman-yunkan and kelson42 February 15, 2024 11:18

veloman-yunkan requested changes Feb 15, 2024

View reviewed changes

include/library.h Outdated Show resolved Hide resolved

src/library.cpp Outdated Show resolved Hide resolved

mgautierfr force-pushed the bookmarks_migrations branch from 65de5d3 to be18c03 Compare February 15, 2024 13:22

mgautierfr requested a review from veloman-yunkan February 15, 2024 13:23

mgautierfr added 18 commits February 15, 2024 14:52

Use a recursive_mutex instead of a mutex.

9b9c61a

This allow us to internally call thread_safe function from already locked context.

[Test] Add missing name in sample library.xml

82cb113

[Test] Add missing flavour in books.

bf1ab03

Test bookmarks serializations.

903f476

Also store book's name in bookmark.

5a0644d

Add book's flavour in bookmark.

699f96c

Allow bookmark to be created from a Book and url/title.

a546eff

Allow to filter books by flavour.

b16f6b9

Introduce migrateBookmarks to move (invalid) bookmarks to new books.

8d97686

[Test] Introduce variant books in sample library.

14c9530

We will need them to test flavour/date bookmarks migration.

Only migrate bookmarks to books with the same flavour.

167e0dc

If there is no book with the same flavour, but book with same name and different flavour, we do the migration to the other book.

Do not migrate bookmarks to an older book.

f3a6043

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

Make getBestTargetBookId public.

3e9d50f

Update tests to check book's title with double quotes (")

7a0ab3a

On top of modifying the existing test, the commit also make `MigrateBookmark` test fails as `migrateBookmarks` now migrates from `wrong-book-id-noname` to `Dummy id`. Fix will be provided in next commit.

Add a getBestTargetBookId directly taking bookName, flavour and date.

73b855c

Add a small test on getBestTargetBookId and flavour.

6b05eeb

mgautierfr force-pushed the bookmarks_migrations branch from be18c03 to 6b05eeb Compare February 15, 2024 13:53

veloman-yunkan approved these changes Feb 15, 2024

View reviewed changes

kelson42 merged commit 1babbc0 into main Feb 15, 2024
16 checks passed

kelson42 deleted the bookmarks_migrations branch February 15, 2024 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate bookmarks between books #1043

Migrate bookmarks between books #1043

mgautierfr commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

mgautierfr commented Jan 22, 2024

veloman-yunkan left a comment

kelson42 commented Jan 26, 2024

BPerlakiH commented Jan 26, 2024

kelson42 commented Feb 6, 2024 •

edited

Loading

mgautierfr commented Feb 7, 2024

mgautierfr commented Feb 12, 2024

mgautierfr commented Feb 13, 2024

veloman-yunkan left a comment

veloman-yunkan left a comment

mgautierfr commented Feb 14, 2024

mgautierfr commented Feb 15, 2024

veloman-yunkan left a comment

mgautierfr commented Feb 15, 2024

Migrate bookmarks between books #1043

Migrate bookmarks between books #1043

Conversation

mgautierfr commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

mgautierfr commented Jan 22, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

kelson42 commented Jan 26, 2024

BPerlakiH commented Jan 26, 2024

kelson42 commented Feb 6, 2024 • edited Loading

mgautierfr commented Feb 7, 2024

mgautierfr commented Feb 12, 2024

mgautierfr commented Feb 13, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Feb 14, 2024

mgautierfr commented Feb 15, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Feb 15, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

kelson42 commented Feb 6, 2024 •

edited

Loading