-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add historical searches list to the Reader initial search screen #11293
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @renanferrari , thanks for this PR 🙇♂️! I agree it was not easy to follow all the conditions in Reader fragment and we will need (more sooner than later) to start a progressive refactor project for that, in any case your solution seems to work well, good job 👍! I tested it both with zalpha and vanilla flavor and didn't find issues. I feel the presence of both the suggestion popup and the history recycler view seems a bit sort of a duplication but I see from your PR description that you already discussed it with @osullivanchris, and also using it is actually smooth so I'm fine with it.
I left only a small inline comment for your consideration. Let me know wdyt 😊.
public interface OnSuggestionClickListener { | ||
void onSuggestionClicked(String query); | ||
} | ||
|
||
public interface OnSuggestionDeleteClickListener { | ||
void onDeleteClicked(String query); | ||
} | ||
|
||
public interface OnSuggestionClearClickListener { | ||
void onClearClicked(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can consider to move this interfaces into a dedicated kotlin file. In this way we could avoid to duplicate some of them in the ReaderSearchSuggestionAdapter
; wdyt?
NOTE: usually we try to create new classes (like this one) in kotlin where possible; but this is just to share a general comment (so no action required actually 😊).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! 😊 I have now extracted those interfaces to a single Kotlin file.
Just a side note: I have also updated the Reader settings screen title from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @renanferrari 😊! Thanks for the changes (and taking care of the settings screen title renaming), all works well 👍! I left just a minor comment about an interface that it seems we are not actually using ad maybe worth cleaning it up (sorry for missing it in my previous pass review 🙇♂️). But a part this it's looking good, good job 👍!
interface OnSuggestionClickListener { | ||
fun onSuggestionClicked(query: String?) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU it seems we are not currently using this interface, maybe worth considering removing it? wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually used by ReaderSearchSuggestionRecyclerAdapter
. It's just not used by the existing ReaderSearchSuggestionAdapter
, which uses SearchView
's own OnSuggestionListener
for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I see! Sorry I was missing it 😊!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @renanferrari ! LGTM
Great work @renanferrari ! I've had a look at the build and everything seems to be working. Sorry that something we thought would be low hanging fruit turned out to be so complex :) I do see the duplication which @develric raised. Perhaps something to improve in future. Its not 'broken' and I think we have invested enough time here on something which is already a bit of a side-track from our iA work :) |
Fixes #11168
This PR introduces a list of historical searches to the initial search screen of the Reader, as shown below.
The implementation here turned out to be quite rudimentary, but it took several rounds of trial and error to reach the minimum code needed to make it work well.
The main challenge here is to keep track of the search screen state, given it is tightly coupled with
ReaderPostListFragment
. For example, here are some of the conditions we need to keep track of to know when to show the historical searches list:I considered the possibility of extracting part of this logic to another fragment or to the existing
ViewModel
, and while I certainly think it can be done, I also think these solutions seemed to require a disproportionate amount of effort when compared to the potential payoffs for this particular feature. In other words, it didn't seem like the right time to invest more in this, especially considering that this is such a small feature.To test
For each of the tests below, you first need to make sure you're using the build variant thas has enabled IA flags.
1. Basic scenario
2. Repeat search
3. Delete and clear
Notes
PR submission checklist:
RELEASE-NOTES.txt
if necessary.