search

FreeLanguageTools / vocabsieve

Simple sentence mining tool for language learning
GNU General Public License v3.0
392 stars 30 forks source link

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

Closed adriafarres closed 3 months ago

adriafarres commented 8 months ago

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.

1over137 commented 8 months ago

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 commented 8 months ago

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

1over137 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.