Skip to content
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

Make highlights respect word selection mode #742

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hrdl-github
Copy link
Contributor

@hrdl-github hrdl-github commented Jun 10, 2023

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:

  • Implement RTL support
  • Embedding selections should follow the same logic, which at the moment only support highlights of entire words

@ahrm
Copy link
Owner

ahrm commented Jun 10, 2023

Do you have any thoughts on this, @ahrm?

No, I guess we'll have to try both and see which one works better.

@hrdl-github
Copy link
Contributor Author

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

@hrdl-github hrdl-github marked this pull request as ready for review June 10, 2023 11:52
@hrdl-github
Copy link
Contributor Author

I'm observing some issues with one of my documents. I will look into this before reopening this.

@hrdl-github hrdl-github marked this pull request as draft June 10, 2023 12:31
@hrdl-github hrdl-github marked this pull request as ready for review June 10, 2023 14:10
@hrdl-github
Copy link
Contributor Author

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.

@hrdl-github
Copy link
Contributor Author

d5c85fa allows preserving the old rendering behaviour of highlights read from the database.

@hrdl-github
Copy link
Contributor Author

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).

@ahrm
Copy link
Owner

ahrm commented Jun 17, 2023

Thanks for your awesome work. Note that I am working on the next version of sioyek (as I have described here: #734 (comment)), 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).

@hrdl-github
Copy link
Contributor Author

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.

@ahrm ahrm mentioned this pull request Jun 18, 2023
The word selection state may be relevant after the mouse button has been released
This seems to stabilise selections that involve characters such as full
stops.
This allows highlights to be expanded to entail entire words when read
from the database or when embedded into PDF files.
This effectively replaces a selected highlight by one with the original
start and end point modified to match the selection's word boundaries.
@hrdl-github
Copy link
Contributor Author

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?

@ahrm
Copy link
Owner

ahrm commented Jan 26, 2024

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.

@hrdl-github
Copy link
Contributor Author

That's completely fine -- I'll open an issue. Most of it was fairly straightforward. Getting 189b82a right was what took most of the time -- feel free to use this in whatever way you see fit.

@hrdl-github
Copy link
Contributor Author

Right, I created one back in June: #741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exact_highlight_select uses mouse event's coordinates instead of highlight's coordinates
2 participants