-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fix order of local search results #7491
Conversation
284f616
to
b9fb39a
Compare
app/src/main/java/org/schabi/newpipe/database/history/dao/SearchHistoryDAO.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java
Show resolved
Hide resolved
b9fb39a
to
2b93e64
Compare
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.
LGTM;
Tested it - seems to work as expected.
👍 for the UnitTests
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.
I haven't looked at the tests themselves.
app/src/androidTest/java/org/schabi/newpipe/testUtil/DatabaseMocker.kt
Outdated
Show resolved
Hide resolved
2b93e64
to
2963cd5
Compare
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.
Tests look good 👍
// For some reason the Flowable returned by getAll() never completes, so we can't assert | ||
// that the number of Lists it returns is exactly 1, we can only check if the first List is | ||
// correct. Why on earth has a Flowable been used instead of a Single for getAll()?!? |
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.
😆
What is it?
Description of the changes in your PR
Results from the local database were being fetched from the database in the correct order, but then the
DISTINCT
part of the query was not done by SQL but by Java using aSet
, which then caused all items to be ordered alphabetically and not by creation date. This PR fixes this by having the related queries just return a list of strings that are already the correct ones and in the correct order. I just addedDISTINCT
to the queries.Fixes the following issue(s)
Fixes #7133
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