feat(reader): add current chapter search - #1877
Conversation
|
Accidentally marked this as ready for review. Converted back to draft; this is still WIP. (sorry) |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Search works with bionic reading now, but single-character searches can be expensive because bionic reading splits words into additional inline DOM nodes. I’m considering either blocking 1-character searches when bionic reading is enabled, or changing the search/indexing path to use a plain-text representation and only map rendered highlights back to the DOM. I'd welcome any feedback on which behavior is preferable. |
|
I would even block search up to 3 characters and maybe make an exception for special characters. |
I don't want to impose but would you be okay with blocking only 1-2 character plain alphanumeric queries, while allowing 3+? I’m a little worried that blocking 3-character searches would reject legitimate terms like “cat”, “dog”, short names, acronyms, etc. |
|
This is also fine. I don't really now the limitation, so just do what works for now. Something like this can be easily added later. |
|
I re-ran the full Jest test suite locally on both the latest upstream Environment:
Result: The failing CI check appears to be from |
|
I have... a question. What is the point of a search for just... the current chapter? At first I assumed it was for.. all downloaded chapters |
I assumed a current chapter implementation because of the issue name stating "search words/characters in a novel chapter". While I see it as a building block for novel search as a whole; I'd still argue that the feature can stand on its own as it is now too, it's something that you'd find commonly in readers, document/text viewers as a whole and it's something I'd say many users would expect. Nice for finding your way back to specific points in a chapter, when you've forgotten a definition of something that was explained some time ago (e.g. you're reading a novel, characters are fighting, an ability definition is dropped, later in the chapter the mc counters said ability but you don't remember specifics). |
|
Iw as wondering why not shoot directly for the All Chapters Search feature since it magnifies this pr goal by several orders of magnitude |
As this is my first PR, I asked under #133 whether limiting the first implementation to the currently open chapter would be acceptable, with all-chapter search left for a later PR. CD-Z replied that smaller PRs are generally better because they are easier to review, so I scoped this PR around that. I agree that all-chapter search would be more powerful, but I see it as a larger follow-up feature with a different search path and UX: searching downloaded/cached chapter text, grouping results by chapter, opening chapters from results, and handling performance across many chapters. This PR focuses on the in-reader search behavior for the currently rendered chapter: highlighting matches, showing the match count, next/previous navigation, and clearing search state when closing search or changing chapters. I think that matches the current issue scope while keeping the first implementation reviewable. |
|
So I tested it on my less modern device (very good to benchnark if the app code is performant) O5DF6qBe.mp4A bit slow, is possible to optimize it? |
The slowdown is there due to the search activating only 300ms after the user stops typing to prevent it from activating on every change of text, I can delete the wait if you want. |
|
Isnt it better if the user triggers the search manually? |
Oh... Well... Yes. I'll change it tomorrow or the day after as I'm not entirely available now. |
I'd prefer to keep the 300ms debounce. |
oh no, I was just wondering. Looking at my browser Find feature it is indeed more comfortable with the automatic trigger vid.mp4Tho the 300ms is a bit too much. And the browser video above doesnt have any delay, clean. |
Well, I'll keep the debounce time then
I understand it being too slow for some people - I set it at 300ms to accommodate people who type slower, I can lower it to 200ms if maintainers'd prefer so too though. |
|
Why is the "bounce" a bad thing? Wanting to search means the users wants to "bounce" at something anyway. That delay seems useless |
|
No delay, very smooth, clean. Ydndn.mp4Browsers "Find" features is exactly the same feature of this pr, and operates on html too. They didnt put the debounce delay |
The debounce is there to avoid running the search/highlight update on every single keystroke, since that can be expensive on longer chapters or slower devices. I agree that too much delay can make it feel less responsive, so I can lower it from 300ms to 200ms or lower if maintainers prefer. For now I’ll keep the debounced live-search behavior since that was the maintainer preference.
Browser find is native and heavily optimized, while this implementation needs to run through the WebView content, update highlights, update the active match, and send the result state back to React Native. So I think a small debounce is a reasonable tradeoff here, especially for long chapters and slower devices. |
It’s to avoid re-running a search on every keystroke. On a mobile WebView, that extra work can make typing feel laggy, and the results can flicker or jump around while the user is still typing. A ~300 ms debounce typically provides the same perceived responsiveness while keeping typing smooth and the search experience more stable. |
|
I edited the delay to 5ms Not really an issue here, on my device: fff.mp4This chapter contains 275,000 characters (html tags excluded). Looks like every type aborts the previous search judging by the speed, idk tho. No freeze, no crash, no slowness(the search is slow in a legit way, the chapter is gigantic) Edit: the final search for "and" gives 1500+ results, the final search for "andr" gives 14 results. |
Summary
1500+.Closes #133
Testing
pnpm run lintpnpm run type-checkpnpm exec jest --selectProjects rn --runInBandnode --check android/app/src/main/assets/js/search.jsnode --check android/app/src/main/assets/js/core.jsManually tested on a Pixel 8a phone:
Notes
I left the commit history as-is to avoid an unnecessary force-push. I can squash it if preferred.