-
Notifications
You must be signed in to change notification settings - Fork 35
feat(koplugin): bulk sync annotations #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This reverts commit f7ec420.
We do not need to query the statistics database for book md5 values. We can just use the ones from the sidecar files, which we already have anyways.
…ionReader.cleanAnnotations
|
Hm, need to look into why the test was failing. Not sure why any of the changes I made here affected ths. |
|
The test is flaky :/ I hit this earlier this morning. |
| local logger = require("logger") | ||
| local DocSettings = require("docsettings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is me being unfamiliar with lua, but can't we define these once outside the functions and use them? Seems counter-intuitive to require every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah true, good catch! I had an issue with ReaderUI, which is only available when reading a book. So I guess I put all requires inside functions for no reason. Just improved it.
|
@tku137 can we chat directly on another platform? Discord or something else? |
Sure, I have a Discord account. I could send you a mail with my handle if you like? Or just send me an invite. |
Yup, perfect! Shoot an email at georgi@gar.dev |
| highlights = (stats and stats.highlights) or 0, | ||
| notes = (stats and stats.notes) or 0, | ||
| last_open = (summary and summary.modified) or os.time(), | ||
| total_read_time = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like total_read_time will always be 0? Wouldn't we lose that data if we sync like that?
|
The current sync method also bulk syncs all books, it just doesn't bulk sync with highlights, correct? I'm wondering if we should be keeping both of the syncs - seems kinda confusing. If this implementation bulk syncs all books with annotations, we can probably keep only that and remove the old one? Also, maybe it's a nice idea to sync only the current book on suspend? So maybe it's worth implementing a "Sync current book" alongside "Sync all books with all highlights"? |
This adds the possibility to bulk-sync annotations. It is probably a rarely used feature, I used it to have an initial sync of all book annotations without the need to open each book individually.
Unfortunately, getting annotations is only available for the currently opened book in koreaders api, see #79. Crawling the whole library directory for sidecar files would be pretty heavy. So in this PR, I use koreaders history api, which gives a list of books opened in the past. This includes read, reading, paused and so on books. So my thinking is: for a book to have highlights, it must have been opened at one point and thus is listed in the history.
Having a list of books from koreaders history, we then can loop over all these books and sync their annotations one by one.
Several improvements have been made along the way, for example: