TheLastProject
commented
2 days ago Thanks for your contribution!
From quick testing I can confirm it does seem to work. However, while reading through your code, I can't help but feel it is very complex for how simple what we're trying to do should be.
While I think the onSaveInstanceState and onRestoreInstanceState are the correct functions to go to deal with the state loss on rotation, I do quite worry how we went from one variable to store the current search filter in (mFilter) to three variables. I also have my doubts your onRestoreInstanceState code does more than set the variable because, from my testing, mSearchView seems to always be null when in onRestoreInstanceState, so the query is never executed.
I've been debugging with the original code (before your change) and I noticed that the value of mFilter does correctly stay set even when returning from a card being viewed, so we do actually know the correct search value in onResume when we return from the card. However, shortly after, for some reason the setQuery gets called on the mSearchView and sets mFilter to an empty string.
So I guess the real issue here is: why is the OnQueryTextListener of the mSearchView called with an empty string when we return from the card view activity? I sadly don't really have an answer to this but I think this is the only issue we have to really fix/work around to restore the search when coming back from the card view.
If we have the workaround for restoring the search state from mFilter when returning, saving and restoring mFilter on rotation (with onSaveInstanceState/onRestoreInstanceState) should be all we need to fix both issues.
One thing I think we may want to consider, given this post I found, is to try to replace the embedded androidx.appcompat.widget.SearchView which Catima currently uses with the new Material SearchView in the hope it does not have this OnQueryTextListener quirk that's causing all our pain here, which would hopefully allow us to restore it straight from mFilter. That would mean leaving this PR alone for a while and focusing on replacing the search view first, which is sadly quite a scope change (created a new issue for that in #2093) :(
So, to summarize: I think your change makes sense to fix the issue requested and the general design is in the right direction. They are however complicated due to bugs in the rest of Android. If we replace the SearchView for a more modern component, we may be able to make this PR a lot simpler and cleaner.
It's a pretty decent first PR :)
Hello, In this pull request, I implemented functionality to save and restore the user's search query during screen rotations and enhance the user experience when navigating back from a selected card, the way user will have the query entered before saved and retrived, based on Issue #1962. Unit tests were added and all four possible tests were run successfully without any error.
After hours of debugging I figured out the only way this Issue is solveble, where we need to implement 2 Strings to handle the changing text as input from user, because of the behaviour after coming back from a card regarding onQueryTextChange. (alternativelly we could use one string and one boolean for this very same behaviour) - changes in states were not effective on this Issue. This approach is a bit extraordinary but It solves both parts of problem of this Issue.
The changes are divided into 2 categories, the first are changes that are connected to screen rotation:
Saving Instance State:
I overridden the onSaveInstanceState method. When a user clicks on a card from the list, the current search query (currentQuery) is saved to a variable called finalQuery. This finalQuery is then stored in the outState bundle, allowing it to be retained even when the screen is rotated.
Restoring Instance State:
The onRestoreInstanceState method is also overridden to retrieve the saved query when the screen is rotated. If the savedInstanceState is not null, the stored search query is fetched and assigned to finalQuery. Finally, if mSearchView is available, the restored query is set in the search view, allowing users to see their previous search term after rotation.
The second part regarding the retrieving the text after coming back from card picking:
New Logic for Search History:
Additional logic was implemented to ensure that the user sees the last search query after returning from a picked card. If the newText input is empty, the app checks if finalQuery is not empty. If so, it sets the query text to finalQuery for the user. If both finalQuery and currentQuery are empty, the app clears the current search (currentQuery = "") and resets the search view to show all cards. When the user changes the text in the search view, the input is stored in currentQuery to be used later for restoring search history.
Handling Navigation from a Picked Card:
Upon returning from a picked card, if finalQuery is not empty, the search view is expanded to show the previous query. The finalQuery is reset to an empty string to avoid unintended behavior caused by the onQueryTextChange method, which is called automatically after returning to the search view.
Tests are focused on both rotation of screen by user (testing same test case as in previous unit test but with rotation commands) and also on simulating steps of the core of the issue with coming back from a card.
Thank you for feedback. Have a nice day :)