Repository navigation
Conversation
Clearing the search field before applying filters kept the old query and filled the field with it again. Apply filters with the text that is currently in the field, so an empty field searches with filters only.
ego1s1
left a comment
There was a problem hiding this comment.
Nice catch, this fixes the right bug. Just normalise toolbarQuery alongside listing.query so blank doesn't split state, and confirm the repro source doesn't keep text in Filter.Text. Otherwise it's good to go.
There was a problem hiding this comment.
Ran into this bug myself last week so thanks for picking it up. Fix is on the right track, but I think it leaves the two query states split.
Right now you set listing.query from toolbarQuery but you never write toolbarQuery back. So if I clear the field to " " you end up with listing.query = null and toolbarQuery = " ". Field looks empty-ish, results are right, but isUserQuery and that back-press check in the Screen (toolbarQuery != null) now disagree. I hit a case where back still tried to clear a query that was already gone.
I'd just normalise both in the same update:
val query = it.toolbarQuery?.takeIf(String::isNotBlank)
it.copy(
listing = Listing.Search(query = query, filters = filters),
toolbarQuery = query
)
That way empty/blank both mean null everywhere. No extra call needed.
One more thing worth checking before merge is that some sources stash the text inside filters as Filter.Text. You pass filters through untouched, so those will still search the old text even after this change. I didn't verify which extension #4239 used. If yours doesn't use Filter.Text we're fine, but can you call it out in the PR? Don't want to wipe all Text filters blindly, that would kill author fields and stuff.
Keep the search field and the active listing query in sync so a blank field does not leave Back trying to clear a query that is already gone.
|
Thanks, good catch. Both are normalized in the same update now: val query = it.toolbarQuery?.takeIf(String::isNotBlank) That fixes the split state and the back-press case. On Filter.Text: I didn't touch filter values in this PR. The source from the report uses checkbox/select genre filters (Content Rating, Type, match mode, genre checkboxes), not a text field, so it doesn't keep the query there. I've added a note to the PR that sources storing the search text in a Filter.Text are out of scope here, since clearing those blindly would also wipe fields like author. Let me know if that looks right. |
ego1s1
left a comment
There was a problem hiding this comment.
LGTM. Normalisation of both toolbarQuery and listing.query looks right, blank now correctly means filters-only and the field stays empty. Thanks for updating the KDoc too.
Fixes #4239
If you search for something, clear the field, and then apply filters, the old search text comes back and is still applied to the results. The field is repopulated with it too.
This happens because the filter button calls
search()without a query, andsearch()falls back to the previous one. This adds a smallsearchWithFilters()that uses the current field text instead:The field text and the active listing query are normalized together, so a blank field leaves no query behind and Back behaves consistently.
Tested on a Pixel 6 (Android 16) emulator. Before: "One Piece" reappeared and the results were One Piece + genre. After: the search bar closes on a blank field and the results are genre-only.
Note: this only normalizes the query. It doesn't change filter values. Sources that keep the search text in a
Filter.Textare out of scope here, since clearing those blindly would also clear fields like author.Mihon is Android-only, so there is no other platform to change.
If this behavior is intentional, feel free to close it.