TextViewer: make find result bold #11427
Conversation
| -- @treturn table Text char list | ||
| -- @treturn table Search string char list | ||
| function util.stringSearch(txt, str, case_sensitive, start_pos) |
There was a problem hiding this comment.
Feels a little odd that this function in util would return some of its internal stuff.
I know it costs nothing (even if the caller does not use and store them, they will soon be GC'ed), and it's a good optimisation for TextViewer so it doesn't have to re-do it again, but it still feels a little odd and ad-hoc.
(Alternatives that don't feel better: duplicate it with this extended return values in TextViewer. Or add a param return_charlists=true so it still looks plain as before when not provided.)
frontend/ui/widget/textviewer.lua
Outdated
| end | ||
| self.text = table.concat(charlist) | ||
| self:reinit() | ||
| self.text = text |
There was a problem hiding this comment.
I guess this strange double self.text = assignment is what makes it work multiple times, so might be worth a comment on the last line: -- restore original text.
Does it really work when doing multiple searches, when no match, when matching, when moving, when again no match, when matching some pattern overlapping a previous match ?
In no case we would re-use the self.charlist with the PTF_BOLD_START/END in it which would prevent a match of some pattern because they are there in the middle ?
(It also feels it would be better to make bold ALL matches, so one may not need to use Find next - but then, it is quite some different code, and you would need a util.stringSearchAll() :))
There was a problem hiding this comment.
Does it really work
Yes it works good.
|
We could also make bold the selected text (not while selecting, but after copying) to see the copied result, if we get actual boundaries in koreader/frontend/ui/widget/textboxwidget.lua Line 2084 in f836f6a |
Well, it's not really something cheap this rebuilding of charlist and rerendering - and it doesn't really feel necessary (people who copied may soon go do other things with it). Also, I get there is no feedback of what has been selected/copied, but making it bold in the original text feels like no other app does that (they may do some kind of reverse video/color like our text selection in a book). Also, I think this kind of feedback has been more requested for in the dictlookup result - but there, half of our results could be from html dicts and use htmlboxwidget, where it would not happen, so we would get this kind of highlighting only in half of cases, which makes it too inconsistent, and it feels it's better to just not have it :) |
There was a problem hiding this comment.
Not super happy with the odd extra returned values of util.stringSearch(), but it's so simple that I can ignore my feeling :) (maybe add a comment just before the return -- TextViewer:findCallback() will benefit from getting these extra charlists we have just computed or something.
The rest of the magic is quite localized in TextViewer, so fine with me.
This change is