Feature: Save contents of the search box between searches

[podzolelements]

While doing some testing on my EPUB fork, I found if you close out of the search box, the searched text does not get preserved; behavior inconsistent from most applications I've used. This means if you want to search for the same string of text multiple times (either between closing the search box or across different documents), you must retype out the search every time you open the search box.

This PR introduces this search-saving functionality, which can be disabled in the settings if desired.

Additionally, I changed it so pressing the enter button in the search box performs the same action as hitting the > next search button, as currently it doesn't do anything other than close the keyboard.

Before

before.webm

After

after.webm

Setting toggle is under Application Settings > Controls and Behavior > Default Behavior, and is enabled by default:

Some notes:

• The search is common across all documents: I think this is simpler (both for UI/programmatically) and more useful than per-document searches
• There is an edge case that I don't particularly care about fixing: A search is performed, then search save setting is disabled, then no other searches are performed (if another is, this issue doesn't apply), then the save setting is re-enabled, the original saved search still shows up. I think the cost to readability from the extra logic required to track the state of the preference change isn't worth it to fix this fringe case.

[max-kammerer]

Why StringBuffer? Please check differences with StringBuilder

[podzolelements]

As far as I can tell the StringBuffer is a thread-safe StringBuilder (at the cost of a bit of performance; for practical purposes I don't think it matters if this test is to be believed) and I wasn't sure how this would interplay with the callbacks and wanted to play it safe. I've since changed it to a StringBuilder

[max-kammerer]

@podzolelements some suggestions from AI:

1. static final StringBuffer previousSearch and the out-parameter

SearchDialog.java:65, plus doSearch(..., StringBuffer searchText) at :234.

The StringBuffer only exists so that doSearch can write back through a parameter, but the parameter is always the very same static field that doSearch can
already see. A plain field assignment does the same thing:

private static String previousSearch = "";
...
previousSearch = newSearch;   // instead of setLength(0) + append(...)

That removes the extra parameter from all three call sites (:128, :136, :140) and the mutable shared buffer along with it. StringBuffer is also the
synchronized legacy class; nothing here runs off the UI thread.

More importantly, static means the value lives for the whole process: the search text from one book shows up when you open the search dialog in a completely different book, and it stays in memory after the document is closed. If the text should be scoped to the opened document, TemporaryOptions (prefs/TemporaryOptions.java) is the natural home — it is recreated per book in OrionApplication.onNewBook (:165-167) and is reachable from the activity.
On maybe even keep it in book options so these feature would work after app restart.

2. The list of accepted IME actions is too broad

SearchDialog.java:114-132.

IME_ACTION_NEXT means "move to the next field" and IME_ACTION_SEND means "send"; consuming them as "search" is not correct, and NEXT in particular breaks keyboard navigation out of the field.

The reason so many action ids had to be enumerated is that the EditText doesn't declare what its action is. Adding android:imeOptions="actionSearch" to @id/searchText in layout/search_dialog_int.xml fixes that at the source — the IME then shows a search key, and the listener can be reduced to IME_ACTION_SEARCH plus the hardware-keyboard branch. Please also accept KeyEvent.KEYCODE_NUMPAD_ENTER alongside KEYCODE_ENTER there.

3. Restored text leaves the caret at position 0

searchField.setText(previousSearch) at :109 puts the caret before the restored text, and onActivityCreated (:212) immediately focuses the field. So a user who opens search intending to type a new term types in front of the old one. Either searchField.setSelection(searchField.getText().length()), or selectAll() so that typing replaces the previous query — the second is the more common behaviour for a search box.

4. The text is recorded even when the option is disabled

doSearch writes to previousSearch unconditionally (:279-280); the preference is only consulted when the dialog is opened, where the disabled branch clears the buffer (:111). So with the option off the query is still kept in memory until the next time the dialog happens to be opened. Checking the preference at the point of writing (or simply not writing when it's off) is both simpler and matches what the setting promises.

5. lastSearch vs previousSearch

Two fields with near-identical names and quite different meanings in the same class: lastSearch (:55) drives the iterate-vs-real-search logic and is reset to null, while previousSearch is the persisted box contents. Please rename the new one to something unambiguous, e.g. savedSearchText.

6. Default value

GlobalOptions.kt:244 and the XML both default to true, so this changes behaviour for every existing user. I'm not against it, but I'd like it to be a deliberate decision rather than a side effect — do you have a reason to prefer true over false here?

7. Nits

• Trailing whitespace on SearchDialog.java:113, :243, :278.
• Boolean.TRUE.equals(...getValue()) at :107 — Preference.getValue() is non-null by construction (prefs/Preference.kt:11-13), so a plain .getValue() reads better.
• doSearch(..., 1, ...) at :128 vs +1 at :136 — worth keeping consistent with the neighbouring calls.
• pref_preserve_search_text_desc ("Searched text is saved between searches") suggests the text survives an app restart, which it doesn't. Something like "Keep the previous query in the search box" would set the right expectation.

[max-kammerer]

@podzolelements please also use https://github.com/max-kammerer/orion-viewer/blob/master/docs/architecture.md. Feel free to ask any questions

[podzolelements]

1. I've moved the saved text out of the static and into the TemporaryOptions system.
2. I've added the IME search restriction to the search box. However, not all keyboards respect this restriction, and some allow you to send different ones intentionally, regardless of the IME restrictions placed on the text box. I'm ok removing NEXT and SEND, as those shouldn't be present unless you've explicitly configured your keyboard to override the restriction. However I'm pretty firm on keeping GOand DONE though based on some of the keyboards I've used.
3. Entire search now selected on reentry.
4. I'm not sure I agree with this. I think it makes it nicer, but it makes the preference TOCTOU bug much worse. If you already have a saved search, then you disable the search saving setting, this approach will make it look like the setting doesn't work at all (as the saved search will still be there until you enter a new search, which at that point it would be cleared). Therefore, the check needs to be performed at the dialog creation time anyway, which is what we were cleaning up. I've decided to have it at both locations.
5. Changed to savedSearchText
6. Yes, enabling this by default is a deliberate decision, I should have explained initially. I am not aware of any other application with a search/Ctrl+f functionality that does not save the search between closing the search box and reopening it. I was surprised by this not being the case, and that's the basis on why I made the PR ;). Additionally, with the saved text now being selected on open, it requires no extra effort to remove the old search. The first key of the new search clears the old, so existing users will not be effected; a sequence of keystrokes before and after the change perform identically (sans reopening the search box and hitting enter immediately, which would previously trigger an error).

[max-kammerer]

@podzolelements Thank you for changes!

I've made second round of review, new field in SearchDialog looks redundant - it could be simply read from temp options, and writed back on changes. Property type could be simple String not StringBuilder

[podzolelements]

I had the field to reduce the verbosity of orionViewerActivity.getOrionApplication().getTempOptions().savedSearchText to just a local savedSearchText, and in order to have the changes propagate the changes a StringBuilder was required. I've updated it to directly use a String, let me know which you prefer.

[max-kammerer]

@podzolelements Cool!

What about adding simple UI test for new feature? You can use ZoomTest as example or other espresso tests in same folder.