sioyek
sioyek copied to clipboard
Make highlights respect word selection mode
Fixes #741. Selections are saved using start and end positions, which usually result in two different selections depending on whether exact_highlight_select
is set. Ideally the selection and resulting highlights should coincide across sessions, be consistent with each other, and be stable when embedding into a PDF file.
The main issue with this is that the word selection mode is not persisted in the database. To work around this I attempted to change the selection start and end positions, which often causes too much text to be highlighted when reloading the highlights from the database. I'll try to see if those using the character boxes centres helps. Alternatively I'll try move the selection start and end points such that there is no overlap with other character boxes. Do you have any thoughts on this, @ahrm?
Two minor tasks remain:
- [x] Implement RTL support
- [x] Embedding selections should follow the same logic, which at the moment only support highlights of entire words
Do you have any thoughts on this, @ahrm?
No, I guess we'll have to try both and see which one works better.
Taking the centre along the vertical axis seems to work reasonably well. I've tested this on a handful of documents, including an RTL document from https://fa.wikipedia.org/w/index.php?title=%D9%88%DB%8C%DA%98%D9%87:DownloadAsPdf&page=%D8%B2%D8%A8%D8%A7%D9%86_%D9%81%D8%A7%D8%B1%D8%B3%DB%8C&action=show-download-screen
I'm observing some issues with one of my documents. I will look into this before reopening this.
I will be testing this during the next few days of ordinary usage. Migrating existing highlights is going to be an issue for those that relied on word selection mode, I think. Most of my past highlights don't cover the beginning and ending of their first and last word, respectively.
https://github.com/ahrm/sioyek/pull/742/commits/d5c85fa46ac329798bb74b68df424eaf54bef3a7 allows preserving the old rendering behaviour of highlights read from the database.
The latest change allows replacing individual highlights with ones with updated start and end points. I haven't experienced any issues with my changes these days. If needed, I can add an expand_all_highlights
command. Do you have an opinion on this?
The only part missing in my eyes would be to document this change and potential workarounds (either setting legacy_word_select
or by updating highlights that were meant to be word-based selections using eh / expand_highlight
).
Thanks for your awesome work. Note that I am working on the next version of sioyek (as I have described here: https://github.com/ahrm/sioyek/discussions/734#discussioncomment-6036425), which will be a major release and changes many aspects of sioyek including database schema. For example things like this will be different in new sioyek:
void DocumentView::expand_highlight_with_index(int index) {
Highlight hl = get_highlight_with_index(index);
delete_highlight_with_index(index);
add_highlight(hl.selection_begin, hl.selection_end, true, hl.type);
}
because now every highlight will have a unique UUID which makes it simple to just update a highlight's properties instead of deleting and re-adding it. So I just wanted to give you a heads up that some parts of this code may have to be modified when the new version launches, so you may want to wait for the new release before devoting too much more time to this (of course the changes are not that drastic and it is relatively easy to modify this code to work with the new sioyek).
That's great news, and thanks for the heads-up. I'd be happy to integrate the changes once you think that your private branch is ready --- ideally shortly before you make a new release to bundle non-trivial changes, but of course I'm fine with whatever timeline you think makes sense.
Should I try to adapt this to the development version? I haven't been able to run the development version yet, possibly because of the shaders not compiling at runtime for some reason. Does it make sense to have this ready before the next release?
I think I may have forgotten about this PR and have implemented parts of this myself :/. So it definitely needs some adapting before beings accepted. Alternatively if you are busy and there are still issues with the development version you could open an issue and I will implement it myself.
That's completely fine -- I'll open an issue. Most of it was fairly straightforward. Getting https://github.com/ahrm/sioyek/pull/742/commits/189b82a3c0e6b621ad7c587d2ff5d4367a867fc0 right was what took most of the time -- feel free to use this in whatever way you see fit.
Right, I created one back in June: #741