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

Remove duplicates from playlist feature #9707

Merged
merged 9 commits into from
Feb 28, 2023

Conversation

Jared234
Copy link
Contributor

@Jared234 Jared234 commented Jan 20, 2023

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

  • Added a feature that allows users to remove duplicate streams from a local playlist.

Before/After Screenshots/Screen Record

Before After

Fixes the following issue(s)

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

@SameenAhnaf SameenAhnaf added feature request Issue is related to a feature in the app playlist Anything to do with playlists in the app labels Jan 20, 2023
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.

Thanks!

@Jared234 Jared234 requested a review from Stypox January 21, 2023 13:58
@Jared234
Copy link
Contributor Author

Jared234 commented Feb 9, 2023

Thanks for the review. I have implemented all your suggestions.

Comment on lines 648 to 649
final List<PlaylistStreamEntry> itemsToKeep = playlistManager
.getDistinctPlaylistStreams(playlistId).blockingFirst();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this run in a separate thread, just like removeWatchedStreams? Also, while duplicates are being removed, the loading screen should be shown.

You could rename isRemovingWatched to isRewritingPlaylist and use it to prevent concurrent calls to removeDuplicates and removeWatched. Unfortunately this has never been the best solution (since there are other functions that change the playlist that are not guarded by that boolean, and so you might still get concurrent changes to the playlist), but there's no need to rework state handling in this PR (a viewmodel would be needed), so keep it as it was before.

@Stypox
Copy link
Member

Stypox commented Feb 28, 2023

See this sonar warning: https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&pullRequest=9707&id=TeamNewPipe_NewPipe&open=AYXO4S5J5QjBB_bdSBLT

I think you can use our own R.string.ok (or R.string.yes). Same goes for cancel iirc.

@sonarqubecloud
Copy link

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

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.

Thank you, I tested it a bit, even with my database, and everything seems to work fine. Thanks! Sorry for the repeated comments, I had to make changes from my phone. I hope I have merged this in time, and good luck again with the exam ;-)

@Stypox Stypox merged commit 23a2071 into TeamNewPipe:dev Feb 28, 2023
@Jared234
Copy link
Contributor Author

Yes, all my projects are now finished by the deadline. Thank you very much for all the support. I really enjoyed working on NewPipe.

@Stypox Stypox mentioned this pull request Mar 1, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app playlist Anything to do with playlists in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add the ability to remove duplicates in a playlist
3 participants