Skip to content
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

Trim search string and remove duplicate records from the database #8242

Merged
merged 6 commits into from Mar 28, 2024

Conversation

dtcxzyw
Copy link
Contributor

@dtcxzyw dtcxzyw commented Apr 17, 2022

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Strip leading and trailing white spaces of search string before passing it into suggestionPublisher after typing or pasting text
  • Trim the search string of the search bar when the user presses search
  • Add new database migration to remove duplicate records in search history
  • Add a test for the database migration

Before/After Screenshots/Screen Record

  • Before:
before.mp4
  • After:
after.mp4

Fixes the following issue(s)

APK testing

Debug APK

Due diligence

@tsiflimagas
Copy link
Contributor

Migration worked great, and trimming also works as expected. Thank you!

@litetex
Copy link
Member

litetex commented Apr 17, 2022

@TiA4f8R
Do we really need a database migration for this?
The users can simply delete their search history and the problems should be gone.
Looks a little bit overkill to me, also regarding the fact that it doesn't affect that many users.

Without the database migration, only a single class is affected, now we have 6...

@AudricV
Copy link
Member

AudricV commented Apr 17, 2022

@TiA4f8R
Do we really need a database migration for this?

Disclaimer: the idea is not from me but from @TobiGr:

the dev might want to add a migration finding duplicate entries in the search history and delete them, leaving just the trimmed one

@TobiGr
Copy link
Member

TobiGr commented Apr 17, 2022

@litetex I personally do not like to leave it up to the user to clean up the database to contain expected values.
It's better to know exactly how the data is structured. It's just the search history, but we should keep the db clean. I've learned that the hard way... (not in this project though)

@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented Apr 18, 2022

I don't think it's a good idea to have the user manually clear the search history for this feature. Actually, NewPipe works fine with or without database migrations because the PR does not add a unique constraint to the search fields.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the database migration should be done, to keep data consistent

@sonarcloud
Copy link

sonarcloud bot commented May 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@triallax triallax added feature request Issue is related to a feature in the app database Issue is related to database operations labels May 22, 2022
@TobiGr TobiGr self-assigned this Aug 16, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TobiGr TobiGr dismissed litetex’s stale review August 17, 2023 16:02

outdated / applied

@TobiGr TobiGr requested a review from Stypox August 17, 2023 16:02
Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the initial work!

@Stypox Stypox merged commit e687eb5 into TeamNewPipe:dev Mar 28, 2024
7 checks passed
@Stypox Stypox mentioned this pull request Apr 1, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issue is related to database operations feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trim trailing/preceding whitespaces from search inputs
7 participants