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

Do not modify selection if the user has already selected text #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adriafarres
Copy link

Selecting multiple words instead of single clicking on top of a word results in a partially duplicated selection. (We essentially extend an already correct user selection, if that makes sense.)

I'm not sure if the field word is now representative of a selection, though.

Selecting multiple words instead of single clicking on top of a word
results in a partially duplicated selection.
@1over137
Copy link
Contributor

1over137 commented Mar 9, 2024

Thanks for the PR, but the reader is getting replaced soon anyways with a new one based on epub.js, so you may have to reimplement this on the new reader soon, I'll leave another message here when the branch is ready

@adriafarres
Copy link
Author

Perfect, thank you. I will use this locally meanwhile.

@1over137
Copy link
Contributor

Take a look at #141
I think the code used to split up the sentences is a bit broken too, it often captures overly long sentences.

@1over137
Copy link
Contributor

Can you take a look at the new code? In this version I couldn't get the original method to work, so I resorted to creating a span for each word, which is really slow.

@adriafarres
Copy link
Author

adriafarres commented Mar 22, 2024

Hey! I actually adapted a few days ago what I previously had to the new epub.js viewer. I'm a bit skeptical of even opening PR, as I'm doing some dirty things to handle some use cases I have (I use DeepL for example, and I'm propagating not only the selected word but the whole underlined sentence to all dictionaries so that I can choose whether the dictionary should translate the whole sentence or just the selection). As of now I'm playing around a bit with what I have to see how it feels.

I haven't updated master for the past few days, so I'm not sure if you have done something about it, but I like the "scrolled-doc" epub.js view much more than what is configured right now. Have you tried the different available options for epub.js?

Regarding the speed of creating a span for every sentence, I haven't really had an issue with it. I'm not a web developer, so I won't be able to give you a better answer than whatever I could end up with from Googling around for ideas.

@1over137
Copy link
Contributor

1over137 commented Mar 22, 2024

Most of the recent commits should be on the other modules, the reader hasn't been touched for a few days now.

but I like the "scrolled-doc" epub.js view much more than what is configured right now

Interesting. I do like it and it's probably better if you can keep all the features, but I'm not sure how easy it would be adapted into the current code which is a modified version of the abandoned ePubViewer project. If you have a working version with that, please do share it. I personally would rather not have to reimplement everything ePubViewer already has (progress saving, fonts, themes, colors, jumping to chapters, etc)

Regarding the speed of creating a span for every sentence

A span for every sentence would not be that slow, the issue is I had to create a span for each word, which did become slow. I think a better way to handle this would actually be overriding mousemove to store the coordinates and overriding click to generate a range from the coordinates instead. Maybe that would work better. Though, eventually I intend to integrate the known words database into the reader, which is probably going to require having a lot of words in its own span anyways.

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.

None yet

2 participants