Koreader Sync (Canary) #3311
Koreader Sync (Canary) #3311
Conversation
Co-authored-by: Joe Milazzo <[email protected]>
|
@MFDeAngelo A few more things that I'll need implementation help on. In the KoreaderService, there are a lot of cases of data coming back null that aren't being handled. I'm going to augment the code with TODOs and push it up. Please branch off my branch and get everything in order. Some other items I need from you:
Reposted here to ensure to keep it consistent. |
|
|
||
| return builder.WithProgress(koreaderProgress) | ||
| .WithPercentage(progressDto?.PageNum, file.Pages) | ||
| .WithDeviceId(settingsDto.InstallId, userId) // TODO: Should we generate a hash for UserId + InstallId so that this DeviceId is unique to the user on the server? |
There was a problem hiding this comment.
Seems like a good idea. I've got that started on my branch. Will push when done with the rest.
|
|
||
| var file = await _unitOfWork.MangaFileRepository.GetByKoreaderHash(bookHash); | ||
|
|
||
| // TODO: How do we handle when file isn't found by hash? |
There was a problem hiding this comment.
I was looking at the Koreader source and it looks like if a response doesn't come back as expected, then Koreader will print a generic error. (Here is the callback code that handles the GetProgress Http response https://github.com/koreader/koreader/blob/master/plugins/kosync.koplugin/main.lua#L737 ) We have two options:
- Return a non 200. This will result in a generic error message for the Koreader user. This generic error can be interpreted as the book doesn't exist in Kavita (or potentially some other problem.)
- Return a normal response, and set the progress to the very beginning to the book. This will effectively say that the book hasn't been opened in Kavita (as it doesn't exist.) If we set the lastUpdated date to minValue, I don't think it will overwrite any user's progress either, but that will need to be tested.
There was a problem hiding this comment.
I plan on implementing option 2 unless you have a differing opinion.
There was a problem hiding this comment.
I would think Option 1 would be better because if the hash isn't found, it's likely the book was never downloaded. It is 100% an edge case but I would rather throw an error than return something that could be interpreted as the book does exist.
I'm curious to hear why you think Option 2 is a more suitable approach?
There was a problem hiding this comment.
As it turns out, only the Update request has a timestamp, not the Get request. I'll still try option 2, but we'll need to test it to make sure if you are half way through a book, then set up progress sync on Kavita, that it won't just take you back to the beginning.
There was a problem hiding this comment.
Ok, I'll switch to option 1.
My biggest concern is the error message returned to the user will say something like Sever responded with a 4xx or something to that extent. I figured that could be confusing as they might think something is wrong with the server/Koreader. As long as we make it clear that a generic response might be solved by uploading the book to Kavita, then we can get past that.
I preferred option 2 as since the book doesn't appear on the server, it's like you don't have any progress in the book. I'd have to test it, but I think Koreader might take the progress that is furthest along.
There was a problem hiding this comment.
But is this case we are designing around actually feasible for the user to get into? Wouldn't you have to have an arbitrary hash somehow and sync it?
There was a problem hiding this comment.
No, this is quite feasible. There are a couple of ways. From the most likely to least likely:
- Manually upload a book to your Koreader device that doesn't exist on Kavita. Then try to get progress. (The best way to combat this is to tell users to use ODPS to get your books onto Koreader in the first place.)
- Download a book to Koreader, then delete it from Kavita. (While unlikely, still possible.)
- Taking notes on a Koreader device appends data to the end of the file. (In fact this is why Koreader uses such a strange hashing algorithm.) If the size of the book happens to be just under a power of 4, then the appended data will be used to compute the hash, resulting in a different hash. While this is unlikely, it is still possible. Worse, there isn't much Kavita can do to avoid this, as it is a fundamental flaw that even the Koreader progress sync servers can't change. Koreader does provide an alternative to hashing the contents of the file, which is to hash the filename itself. I see this as an issue for Kavita, as it's pretty likely that filenames could be the same. It's really a tradeoff for if you rather a changed hash (hash the data) or a hash collision (hash the filename). Personally, I think it is much less likely for somebody to experience one of the collisions from adding notes to your book on Koreader. I doubt many koreader users are typing out notes, and most epubs are pretty big, so being close to a power of 4 is unlikely. I think it's a small enough likelyhood that most users will never come across it, but in the 0.001% chance it happens we will want to have an answer for what happened.
Given enough users each one of these cases will happen at least once.
| var userProgressDto = await _unitOfWork.AppUserProgressRepository.GetUserProgressDtoAsync(file.ChapterId, userId); | ||
| if (userProgressDto == null) | ||
| { | ||
| // TODO: Handle this case |
There was a problem hiding this comment.
If I'm not mistaken, this means the book exists on Kavita, but hasn't been opened.
I'm not sure how a progressDto is initialized (I've looked everywhere, but it isn't clear to me.) I think I'll need help initializing SeriesId, LibraryId, and VolumeId.
There was a problem hiding this comment.
That is correct, when a progressDto is null, it means there is no progress for the book. My concern here is that while we can send a fake progressDto to Koreader, we should realistically perform a save and create a progress entry on the DB, correct?
I can take care of the implementation, I need guidance on the flow of these methods. ReaderController has an example for saving progress.
There was a problem hiding this comment.
Yes, I think it makes the most sense to create and save the userProgressDto. It should act like the first time a user has opened the book, then KoreaderHelper.UpdateProgressDto will make sure that it has the correct page and bookScrollId.
I hope I was able to answer the question well. Please let me know if I didn't.
Canary testers, please test this out thoroughly. Test what happens when reading a fresh book, existing book, switching between Kavita and Koreader and report in https://discord.com/channels/821879810934439936/1298773623825367170 all experiences so we can get this merged into nightly.
Added