SimonHalvdansson / Harmonic-HN

Modern Android client for Hacker News
https://play.google.com/store/apps/details?id=com.simon.harmonichackernews
Apache License 2.0
620 stars 39 forks source link

Disable refresh while typing the search query #68

Closed flofriday closed 1 year ago

flofriday commented 1 year ago

There is no logical reason to refresh while you are typing. However, the refresh would have revealed an invalid cache and would start loading the results from the previous search.

You could reproduce this with:

  1. Search something
  2. Go to the main screen
  3. Go to the search
  4. Refresh by pulling from the top

Now the refresh gesture is disabled while you are in the search and no results have been loaded.

flofriday commented 1 year ago

The code is little bit more verbose than it seems it would be necessary because the adapter has to call code from the fragment. I modeled the solution to be similar to the RefreshListener which needs to do something similar.

SimonHalvdansson commented 1 year ago

I tested it out and it seems to work to just set swipeRefreshLayout.setEnabled(!adapter.searching); in updateSearchStatus() in StoriesFragment. This function should be called whenever the search status is updated and I tried it out and it seems to be working.

The RefreshListener approach is used because we want to pass information from the adapter to the fragment. For search, this is already done using exitSearch() in StoriesFragment (this should probably be a callback actually). As this is already implemented, I think this is the easiest approach.

However I agree with the UX sentiment that the refresh indicator should be disabled while searching. Before I close the PR, is there something that you see missing from the swipeRefreshLayout.setEnabled(!adapter.searching); approach?

flofriday commented 1 year ago

Wouldn't that also disable the refresh animation once the results are loaded?

flofriday commented 1 year ago

It did, but you were right that the RefreshEnabler abstraction was unnecessary and initially disabling the gesture is possible in updateSearchStatus().

I now removed that but overall I think that we should keep the refresh in the search once the results are loaded. Which this PR should now do.

SimonHalvdansson commented 1 year ago

Okay I see, tried it out and works, thanks :)