[DictQuickLookup] NT: add text selection to the dictionary widget#13232
Conversation
|
Could you also describe the behavior a bit? How do you trigger the crosshairs? |
Absolutely. Initially, when you open the dictionary widget from the reader, FocusManager will function as usual. On Kindle 4 or devices with a keyboard, I have assigned the shortcuts screenkb + up/down or shift + up/down to initiate text selection (as remainder, on kindle text selection in reader is initiated with up/down so it is fairly similar). At this point, FocusManager is disabled, allowing the cursor keys (and press) to control the now-visible crosshairs. Pressing back should stop text selection and restore FocusManager’s control of the widget. (this is the case for the first three screenshots) I don't have a direct solution for |
some issuesthere is one thing I have noticed however, sometimes and really unexpectedly, if try to select something, something else is selected instead, for example. The following two screenshots show where I placed the crosshairs and where I eventually made the selection (first image), only for the selection to occur as seen in screenshot 2. In fact in that occasion, I could not select any other word in that same line except for that one. I tried to select the same word on kindle voyage and everything works as one would expect, not sure what is going on there. Perhaps
edit: apparently if I untick |
| text_selector = { indicator = nil, }, -- Add this for indicator visibility | ||
| highlight_text_selection = true, | ||
| html_link_tapped_callback = function(link) | ||
| self.html_dictionary_link_tapped_callback(self.dictionary, link) | ||
| end, | ||
| -- We should override the widget's paintTo method to draw our indicator | ||
| paintTo = function(widget, bb, x, y) | ||
| -- Call original paintTo from ScrollHtmlWidget | ||
| ScrollHtmlWidget.paintTo(widget, bb, x, y) | ||
| -- Draw our indicator on top if we have one | ||
| if widget.text_selector.indicator then | ||
| local rect = widget.text_selector.indicator |
There was a problem hiding this comment.
You're becoming good ! ... at user patching :)
I'm not sure this kind of user patching hack is welcome in the core codebase (I may have done a few in the custom toc/hidden flows PR, to not spread the infection to many things and keep it localized).
You don't need this crappy text_selector = { indicator = nil, } (a table so you can pass it and access this mutable indicator thingie).
You could as well just access self in here, and have indicator as a property of DictQuickLookup (with maybe a more verbose name).
As for this paintTo user patching, may be you could just pass a post_paintTo_func and check and call it in the ScrollText/HtmlWidgets if it exists (it's touching these other modules, but it's just 3 generic lines added). Dunno. @NiLuJe any preference or better idea ?
There was a problem hiding this comment.
I went ahead and followed your suggestion, any comments there?
There was a problem hiding this comment.
yeah no, it is just wrong. I just noticed it. Terrible job.
There was a problem hiding this comment.
@poire-z I could do with some help here, cause I am lost here now. The post_func doesn't draw the scrollbar and "scrolling" through the content doesn't update correctly the box. in short, it's a mess
There was a problem hiding this comment.
Well, reading 33e4e0d, I'm not sure I like my suggestion :)
May be just do as you originally did, but set it only when you know you need it, that is when the device is non-touch and has the capabilities. (And don't put the conditions all over again, just set a self.allow_key_selection (or something like that) in :init().)
There was a problem hiding this comment.
May be just do as you originally did, but set it only when you know you need it, that is when the device is non-touch and has the capabilities. (And don't put the conditions all over again, just set a self.allow_key_selection (or something like that) in :init().)
I'm not sure I follow. I think I do now
| --[====[ | ||
| The following methods are used to handle text selection in the dictionary widget for non-touch devices. | ||
| ]====] |
There was a problem hiding this comment.
We don't really need more folklore (you were allowed for the "typed on my commodore64", that's enough :)).
There was a problem hiding this comment.
coming from a person that "allegedly" enjoys reading, this is quite disheartening ;)
I only was trying to segregate the nt stuff so it doesn't get in anyone's way
| - Requires self.text_widget.text_selector.indicator to not exist | ||
| ]] | ||
| function DictQuickLookup:onStartTextSelectorIndicator() | ||
| if not (self.definition_widget and not self.text_widget.text_selector.indicator) then return false end |
There was a problem hiding this comment.
A bit hard to read condition, I stopped here for a minute trying to reformulate it to see what it checks.
if not self.definition_widget then return false end -- not yet set up
if self.text_widget.text_selector.indicator then return false end -- already startedwould read clearer.
| -- Update display before changing widget state to clear old position | ||
| UIManager:setDirty(self, function() | ||
| return "ui", self.definition_widget.dimen | ||
| end) | ||
| -- Update widget state | ||
| self.text_widget.text_selector.indicator = rect | ||
| -- Trigger refresh on the ScrollTextWidget/ScrollHtmlWidget itself | ||
| -- to ensure the paintTo method that draws the crosshair is called | ||
| UIManager:setDirty(self, function() | ||
| return "ui", self.definition_widget.dimen | ||
| end) |
There was a problem hiding this comment.
Your comment in the previous functions -- Mark definition widget area as dirty for clean re-draw was more correct than these ones.
You calling here twice setDirty won't trigger anything, they'll just mark the same stuff for refresh for the next time UIManager is given control back. As you continue your cpu-bound work in this function, nothing will happen after the first setDirty.
You may as well just add one setDirty at the end of this function.
| --[[ | ||
| @details This function manages the text selection process and subsequent actions: | ||
| - Initiates text selection on first press | ||
| - On second press (when selection is complete): | ||
| * Processes the selection | ||
| * Handles Wikipedia/Dictionary lookup based on hold duration | ||
| * Clears highlights after lookup | ||
| @note Wikipedia lookup can be toggled with a long hold (≥5 seconds) | ||
| @return boolean Returns true if the event was handled, false otherwise | ||
| @local variables | ||
| _text_selection_started: boolean tracking selection state | ||
| _hold_duration: timestamp for tracking hold duration | ||
| ]] |
There was a problem hiding this comment.
I've seen this kind of (really verbose) comments in a few @Commodore64user recent PRs. Are they extracted and displayed nicely in the doc @Frenzie ?
(I personally prefer reading the comments in the code, than having such long description at the top, having to glance up and down - I think I mentionned that before.)
There was a problem hiding this comment.
Probably not very nicely, but with two dashes it won't show in the docs regardless. I'd agree it should be much briefer to be useful. The internal functionality is internal.
This PR title is missing a "Non-Touch" element :)
The international stage may not really care :/ I apologize on its behalf.
No idea. I tried understanding the part of your code involved, no clue. |
| if self.is_wiki and selection_widget.highlight_start_idx then | ||
| -- For wiki content, extract the selected text using the indices | ||
| selected_text = selection_widget.text:sub( | ||
| selection_widget.highlight_start_idx, | ||
| selection_widget.highlight_end_idx | ||
| ) | ||
| else | ||
| -- For dictionary content, highlight_text should contain the complete selection | ||
| selected_text = selection_widget.highlight_text | ||
| end |
There was a problem hiding this comment.
Not sure why you need different methods, but you should say why :)
Have you tested it with a non-HTML dict ? Because wikipedia and text dict results should use the same TextBoxWidget.
There was a problem hiding this comment.
well selection_widget.highlight_text returns nil with wiki. highlight_text is a property of HTMLBoxWidget and wiki doesn't use that.
koreader/frontend/ui/widget/scrollhtmlwidget.lua
Lines 35 to 36 in 70fcc90
koreader/frontend/ui/widget/scrolltextwidget.lua
Lines 50 to 51 in 70fcc90
what is a non-html dictionary? ;) I don't think I have one, so no. Should I invert the check then? if self.is_html then selected_text = selection_widget.highlight_text else ... end
There was a problem hiding this comment.
You may investigate what have changed and how textboxwidget vs. htmlboxwidget differ. May be they should have a common method to get the selected text (it's possible @TnS-hun changed stuff with the indices and that the selected text is no longer readily available - if this is the case, I'd rather add to each of them a common method to get it, rather than having to handle the differences here like you do).
A non-html dict is one that doesn't have sametypesequence=h in its .ifo. Their results shows no bold/italic/font size differences, like Wikipedia results: plain boring text.
If you don't find any (they should be more common than =h though), just copy one of yours and change in its ifo its name and sametypesequence=h to =x: you'll get the html converted to plain boring text, which should suffice to test.
There was a problem hiding this comment.
There was a problem hiding this comment.
You may investigate what have changed and how textboxwidget vs. htmlboxwidget differ
I think I am going to be busy that day ;) but a quick blame yields bf35875
and this golden nugget sums it up really #12948 (review)
| if self.is_html then | ||
| -- For dictionary content, highlight_text should contain the complete text selection | ||
| selected_text = selection_widget.highlight_text | ||
| else | ||
| -- For wiki content, extract the selected text using the indices |
There was a problem hiding this comment.
Please test with a text dict, as your comments feel wrong.
There was a problem hiding this comment.
could you kindly turn the Wiktionary into one? I didn't really understand what you said in the other comment... ;) or better yet, if you could test it, that'd be great. Please
There was a problem hiding this comment.
I won't test it for you - you need to see it to update your comment.
Take your favorite dict, copy the blah.ifo file (present in its directory on your device) to blah.ifo.ORIG, edit the blah.ifo, change in it:
sametypesequence=h
to
sametypesequence=x.
After this, when showing results, you should have plain text (and no longer italic or bold or various font sizes), like with Wikipedia results.
There was a problem hiding this comment.
is this what you mean For non-html content, extract the selected text using the indices?
There was a problem hiding this comment.
With TextBoxWidget, extract the selected text using the indices
With HtmlBoxWidget, we can blah blah
But I'd rather wait for @TnS-hun thoughts before doing too much kitchening in here.
There was a problem hiding this comment.
So, do you confirm you have the same behaviour on this text dict as with Wikipedia results?
There was a problem hiding this comment.
yes, it worked just fine.
edit: I am not sure if it still considered self.is_html or not, but I was able to get definitions either way
| self.image_alt_face = Font:getFace("cfont", font_size_alt) | ||
| self.allow_text_selection = Device:hasScreenKB() or Device:hasKeyboard() | ||
| if self.allow_text_selection then | ||
| self.allow_nt_text_selection = Device:hasScreenKB() or Device:hasKeyboard() |
There was a problem hiding this comment.
If it's not not IsTouchDevice(), don't name it "nt_text_selection"...
Name it "key_text_selection".
| highlight_text_selection = true, | ||
| post_paintTo_func = function(widget, bb, x, y) | ||
| -- We should override the widget's paintTo method to draw our indicator | ||
| paintTo = function(widget, bb, x, y) |
There was a problem hiding this comment.
paintTo = self.allow_by_key_text_selection and function(widget, bb, x, y) ...
I don't think so, there's no crengine involved here. Have you tested how it works (wikipedia, text results) on a touch device, if you notice the same issue ? If yes, that's my problem, show me more. |
|
Okay, let me have another look. In the meantime i'll let you go back to Page Browser and Book Map ;) Edit: I'm sick of eating pears |
|
@poire-z I can replicate the issue on kindle voyage as well, so it is a you problem ;) (my favourite kind). you can test it yourself, search for the word |
|
I won't be able to replicate, too many variables. Do you have italic or not ? If yes, it's a html-dict and a @TnS-hun problem :). If not, it's still mine :/ |
have you even tried though? I gave you all the relevant ones, but again; 600x800 screen, wiktionary (no-etymology version), text justified and font size 20. It is an The screenshots are accurate, both crosshairs and fingers placed on edit: @poire-z it was incredible easy for me to replicate the dict lookup on the emulator. seriously |
|
I understood you had the problem with text results :/ wrongly apparently. |
|
to make it, even clearer. here it is happening on the emulator. Screen.Recording.2025-02-14.at.03.10.13.mov |
(Just a guess: MuPDF might be returning multiple rects - or table of rects - on the same line, when there are multiple wrapping HTML nodes on that line, a new rect/table when we get in/out of each node ?) |
In same cases MuPDF returns a visually single line of text as multiple lines. Merge such lines to ensure that getSelectedText works properly. See: koreader#13232 (comment)
|
I made a simple fix for the issue: #13276 |
In same cases MuPDF returns a visually single line of text as multiple lines. Merge such lines to ensure that getSelectedText works properly. See: #13232 (comment)
| self.html_dictionary_link_tapped_callback(self.dictionary, link) | ||
| end, | ||
| -- We need to override the widget's paintTo method to draw our indicator | ||
| paintTo = self.allow_key_text_selection and function(widget, bb, x, y) |
There was a problem hiding this comment.
Have you tested all this on a device without keys ?
Feels like paintTo = false and function will make paintTo be false and still ovveride the original method with false and may crash.
May be best to add a or nil at the end
There was a problem hiding this comment.
no i haven't. only tested on emu and k4. will add the or nil then
There was a problem hiding this comment.
follow up question. how can I make the emulator be touch only? or useDPadAsActionKeys?
currently doing ./kodev run -t somewhat replicates non-touch but still ignores useDPAd though
There was a problem hiding this comment.
Dunno if there are options. When I need that, I just hack into frontend/device/sdl/device.lua to change the yes/no I want to have.
poire-z
left a comment
There was a problem hiding this comment.
Not really fond of having DictQuickLookup grow 10% more fat, and adhoc menu items because you don't want the same cursor speed in reader vs dict - all this for obsolete non-touch devices where everything is unpractical....
But well, we don't choose our family.
In same cases MuPDF returns a visually single line of text as multiple lines. Merge such lines to ensure that getSelectedText works properly. See: koreader#13232 (comment)
…reader#13232) Initially, when you open the dictionary widget from the reader, FocusManager will function as usual. On Kindle 4 or devices with a keyboard, I have assigned the shortcuts <kbd>screenkb</kbd> + <kbd>up</kbd>/<kbd>down</kbd> or <kbd>shift</kbd> + <kbd>up</kbd>/<kbd>down</kbd> to initiate text selection (as a remainder, on kindle text selection in reader is initiated with <kbd>up</kbd>/<kbd>down</kbd> so it is fairly similar). At this point, FocusManager is disabled, allowing the cursor keys (and press) to control the now-visible crosshairs. Pressing <kbd>back</kbd> should stop text selection and restore FocusManager’s control of the widget.





what's new
This pull request is dedicated to enhancing the user experience for non-touch devices. At present, when reading a dictionary definition on said devices, users have limited options for performing a meta-search. While those with a keyboard (hasKeyboard) can effortlessly begin typing a new word to look up, the process is not as seamless on devices like the Kindle 4 or those with minimal keys (hasFewKeys). To address this challenge, and from the creator of Hotkeys, I am delighted to introduce to the [inter]national stage…
usage
Initially, when you open the dictionary widget from the reader, FocusManager will function as usual. On Kindle 4 or devices with a keyboard, I have assigned the shortcuts screenkb + up/down or shift + up/down to initiate text selection (as a remainder, on kindle text selection in reader is initiated with up/down so it is fairly similar). At this point, FocusManager is disabled, allowing the cursor keys (and press) to control the now-visible crosshairs. Pressing back should stop text selection and restore FocusManager’s control of the widget. (this is the case for the first three screenshots)
enhancements to
DictQuickLookup:onStartTextSelectorIndicator,onStopTextSelectorIndicator, andonMoveTextSelectorIndicatorto manage the indicator's behaviour.registerKeyEventsto clean up the initialisation process and consolidate key event handling logic.paintTomethod ofScrollHtmlWidgetandScrollTextWidgetto draw the text selector indicator on top of the content.others:
DoubleSpinWidgetfor adjusting crosshairs speed settings separately for reader and dictionary modes. This replaces the previous singleSpinWidget.FocusManagerto include akey_events_enabledflag, allowing temporary disabling of key events during text selection.screenshots
focus manager in control of widget vs text selection in charge
This change is