Fixes #3027 Category search does not work properly [current master] #3034
Conversation
* Clear previous categories search result only when the new search results are non-empty
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
Codecov Report
@@ Coverage Diff @@
## 2.11-release #3034 +/- ##
===============================================
+ Coverage 4.43% 4.45% +0.01%
===============================================
Files 259 259
Lines 12261 12264 +3
Branches 1051 1051
===============================================
+ Hits 544 546 +2
Misses 11678 11678
- Partials 39 40 +1
Continue to review full report at Codecov.
|
| @@ -20,6 +17,9 @@ | |||
| import javax.inject.Singleton; | |||
| import timber.log.Timber; | |||
|
|
|||
| import static fr.free.nrw.commons.di.CommonsApplicationModule.IO_THREAD; | |||
There was a problem hiding this comment.
Do you have a style guideline document, or stylizer tool, for this project? Different projects calls for different import orders.
| @@ -61,7 +61,6 @@ class CategoriesPresenterTest { | |||
| categoriesPresenter?.searchForCategories("test") | |||
| verify(view)?.showProgress(true) | |||
| verify(view)?.showError(null) | |||
| verify(view)?.setCategories(null) | |||
There was a problem hiding this comment.
Why should this test case be removed?
| @@ -20,6 +17,9 @@ | |||
| import javax.inject.Singleton; | |||
| import timber.log.Timber; | |||
|
|
|||
| import static fr.free.nrw.commons.di.CommonsApplicationModule.IO_THREAD; | |||
| import static fr.free.nrw.commons.di.CommonsApplicationModule.MAIN_THREAD; | |||
There was a problem hiding this comment.
This change as coded seems to be primarily about not clearing search results. But that's not the issue you're citing for the change, so it's unclear to me why you are making this change. And it doesn't seem like this change would fix that issue. Can you explain?
| @@ -92,10 +93,14 @@ public void searchForCategories(String query) { | |||
| s -> categoryItems.add(s), | |||
| Timber::e, | |||
| () -> { | |||
| view.setCategories(categoryItems); | |||
| if (null != categoryItems && categoryItems.size() > 0) { | |||
There was a problem hiding this comment.
Can categoryItems be null? (I don't see how it would be.) If so, then the isEmpty check on line 103 will crash. If not, then checking is unnecessary here.
| @@ -75,7 +77,6 @@ public void searchForCategories(String query) { | |||
| .doOnSubscribe(disposable -> { | |||
| view.showProgress(true); | |||
| view.showError(null); | |||
| view.setCategories(null); | |||
| @@ -39,6 +39,8 @@ | |||
|
|
|||
| private CompositeDisposable compositeDisposable; | |||
|
|
|||
| private List<CategoryItem> lastShownCategoryItems=new ArrayList<>(); | |||
There was a problem hiding this comment.
Are you planning to use the items in this list for anything beyond checking if the list is empty?
If not, then all you need is a boolean, which will also have lower memory impact.
Should this be cleared when the view is attached/detached?
Description (required)
Clear previous categories search result only when the new search results are non-empty
Fixes #3027 Category search does not work properly [current master]
What changes did you make and why?
Clear previous categories search result only when the new search results are non-empty
Tests performed (required)
Tested betaDebug on Pixel Api 27