Highlight notes not getting synced

[righteous-coder]

Symptom

Some highlights' notes don't get synced.

• ver: 2.3.0.0
• platform: windows 10

I think this is what happened

1. Create a highlight with no note on device A.
2. Sync highlights between device A and B. The highlight is copied to B. These copies have the same datetime.
3. Add add a note on the highlight on device A (or B, doesn't matter). Apparently this does NOT update the datetime.
4. Sync highlights again. KoHighlights sees they have the same datetime and thus doesn't try to sync the note.

I checked the code and I see it chooses to not sync as long as the highlights have the same datetime.

KoHighlights/main.py

Lines 2583 to 2585 in 7504ac0

	if hi_text == trg_hi.get("text", ""):
	if src_hi["datetime"] == trg_hi["datetime"]:
	break # it's the exact same annotation

Suggestion

Even if two highlights have the same datetime, I want KoHighlights to check the notes and if one of them is empty the non-empty note should be copied. If both have notes then... maybe show a popup window and let the user resolve the conflict.

[noembryo]

Well, something is wrong here..
When editing a note, the code of the update_comment function updates the annotation's date.

If you don't mean adding a note within KOHighlights, but in KOReader, then you might be right.
I haven't check to see if this updates the datetime too..

Adding a lot of if in the nested for loops that check the annotations, will slow this process a lot, so I want to avoid that as much as possible.
I'll have to think about it a little..

[righteous-coder]

If you don't mean adding a note within KOHighlights, but in KOReader, then you might be right.

Yes, I meant KOReader. I'm sorry, I should've been clear.

[righteous-coder]

Adding a lot of if in the nested for loops that check the annotations, will slow this process a lot, so I want to avoid that as much as possible.

From my understanding, it's testing all pairs of annotations from two (or more) lua files. So most pairs end up not passing the hi_text == trg_hi.get("text", "") test.

So I don't think it'd add much overhead if we add the note checking/syncing logic (after the above test).

[righteous-coder]

I feel that you could speed up this function (merge_new_highs) by doing a set intersection operation to find matching annotations instead of doing N^2 comparisons, but I'm not sure

[noembryo]

So I don't think it'd add much overhead if we add the note checking/syncing logic (after the above test).

Yes, you're right.
But I wonder if a better/simpler solution would be to have KOReader change the datetime of the annotation if a note was edited. Perhaps opening an issue about it?
But maybe, I should do it anyway.. We'll see

I feel that you could speed up this function (merge_new_highs) by doing a set intersection operation to find matching annotations instead of doing N^2 comparisons, but I'm not sure

The sets being the source and target lists?

[righteous-coder]

The sets being the source and target lists?

Yes. The set of source texts (str) and the set of target texts, specifically.

Perhaps opening an issue about it?
But maybe, I should do it anyway.. We'll see

That'd be nice. Or I could do it, too.

[noembryo]

From the first look at the code, the change you ask is looking very difficult.

I want KoHighlights to check the notes and if one of them is empty the non-empty note should be copied.

This is easy, but I think is not always ok to do it, since you might want to delete a note.

If both have notes then... maybe show a popup window and let the user resolve the conflict.

I can't create a popup from this function, because I think it is used by the CLI too.
No it doesn't..

Or I could do it, too.

OK. Perhaps it's better..

[righteous-coder]

Created a issue here: koreader/koreader#12957

[righteous-coder]

This might be slightly off-topic but can I ask why KoHighlights comapres text rather than pos0 and pos1? Is it so that you can sync book files that are not completely the same byte-to-byte but are basically the same book?

[noembryo]

No. The books must be the same and there is a md5checksum check to ensure that.
There is a "hack" that let's you sync different ePubs, that the user knows they contain the same book (re-saved by some program and got different md5checksum).

The reason for the text check, has more to do with how the original comparison was made in the old format metadata, and maybe also because one check might be better than two..? 😃
I don't think it's really the faster way though..

[righteous-coder]

I see. At least for the new format, maybe it make more sense to check pos0 and pos1 because unrelated annotations can have the same text? It might be just me but I often highlight a single word so there's a pretty good chance that happens.

[noembryo]

Hmm.. This is interesting. I never thought about this test case.
Perhaps it needs to change..