Move search suggestion logic into a view model. #8746
Conversation
Isira-Seneviratne
changed the title
triallax
added
the
codequality
Isira-Seneviratne
force-pushed
the
Use_search_view_model
branch
3 times, most recently
from
19539ca to
5f7d9eb
Compare
|
Kudos, SonarCloud Quality Gate passed! |
Isira-Seneviratne
force-pushed
the
Use_search_view_model
branch
from
5f7d9eb to
f9b6a9a
Compare
Isira-Seneviratne
force-pushed
the
Use_search_view_model
branch
from
f9b6a9a to
43c8467
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
Also here. As much as I like this change, the following description is not enough: "Move the search suggestion retrieval logic into a view model". You should also include information about which code you moved, whether you had any trouble and/or fixed some bugs in the process, how you tested to make sure everything works as expected, whether you expect any difference from before ... E.g. see #7981 or #7491, or my player-related PRs for an example |
|
@Stypox Sorry for the late reply, I was ill for much of this week. Thanks, I'll improve the description of this PR tomorrow. |
Isira-Seneviratne
force-pushed
the
Use_search_view_model
branch
from
43c8467 to
afe49a0
Compare
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
In the PR description I would have expected right away answers to these example questions. I might find such answers by analyzing the code logically and by reading online resources, but that's not always the case, and is in any case a huge wast of time on my end. Since you wrote this code and looked up docs, you already know the answers and it would take you just some five minutes to write them down, but that would reduce my reviewing time from half an hour to five minutes, since I would be able to get your line of reasoning right away. When you create a PR you want to convince reviewers that it is worth merging. Oh, and if you don't have the answer to these kinds of questions right away, it might be a clear indication that the PR still requires some work.
- why you have changed the signature of
search() - what the parts of code you moved did
- why you removed things from
onResume - how the view model is able to resume after the app is killed in the background
- how you tested that the code actually works
(btw, these are just example questions: obviously in other PRs the questions would be different, but the kind of questions is usually similar)
Here is a bug, steps to reproduce:
- in Android developer settings, set the maximum number of background processes to 0
- open NewPipe and start typing something in the search bar (without searching)
- switch away from NewPipe and go to another app, e.g. settings
- switch back to NewPipe and you will get the crash below
Crash log
java.lang.IllegalArgumentException: serviceId is NO_SERVICE_ID
at org.schabi.newpipe.util.ExtractorHelper.checkServiceId(ExtractorHelper.java:73)
at org.schabi.newpipe.util.ExtractorHelper.getSuggestionsFor(ExtractorHelper.java:104)
at org.schabi.newpipe.fragments.list.search.SearchViewModel.getRemoteSuggestionsObservable(SearchViewModel.kt:73)
at org.schabi.newpipe.fragments.list.search.SearchViewModel.suggestionDisposable$lambda-1(SearchViewModel.kt:42)
at org.schabi.newpipe.fragments.list.search.SearchViewModel.$r8$lambda$ZNdtqSfaiCa4_8WvIQkYZMkCU8o(Unknown Source:0)
at org.schabi.newpipe.fragments.list.search.SearchViewModel$$ExternalSyntheticLambda3.apply(Unknown Source:4)
at io.reactivex.rxjava3.internal.operators.observable.ObservableSwitchMap$SwitchMapObserver.onNext(ObservableSwitchMap.java:111)
at io.reactivex.rxjava3.observers.SerializedObserver.onNext(SerializedObserver.java:114)
at io.reactivex.rxjava3.internal.operators.observable.ObservableDebounceTimed$DebounceTimedObserver.emit(ObservableDebounceTimed.java:143)
at io.reactivex.rxjava3.internal.operators.observable.ObservableDebounceTimed$DebounceEmitter.run(ObservableDebounceTimed.java:168)
at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:65)
at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:56)
at java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
at java.lang.Thread.run(Thread.java:1012)
| } else if (listNotification.isOnError() | ||
| && listNotification.getError() != null | ||
| && !ExceptionUtils.isInterruptedCaused(listNotification.getError())) { | ||
| showSnackBarError(new ErrorInfo(listNotification.getError(), | ||
| UserAction.GET_SUGGESTIONS, searchString, serviceId)); | ||
| } | ||
| } else if (response instanceof SuggestionItemError) { | ||
| final var throwable = ((SuggestionItemError) response).getThrowable(); | ||
| showSnackBarError(new ErrorInfo(throwable, UserAction.GET_SUGGESTIONS, searchString, | ||
| serviceId)); | ||
| } |
There was a problem hiding this comment.
Why are there two ways of getting errors? Can't they be merged, e.g. removing Yes they can, I pushed a commit myselfSuggestionItemError and always using the listNotification.getError()?
What is it?
Description of the changes in your PR
suggestionsFormethod togetSuggestionsFor.Fixes the following issue(s)
Relies on the following changes
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence