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

Sort bookmarked playlists #8221

Merged
merged 26 commits into from
Mar 30, 2024
Merged

Sort bookmarked playlists #8221

merged 26 commits into from
Mar 30, 2024

Conversation

GGAutomaton
Copy link
Contributor

@GGAutomaton GGAutomaton commented Apr 15, 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

  • Database migration
    Add a new column display_index (INTEGER NOT NULL DEFAULT 0) in playlists and remote_playlists.
  • A playlists merge algorithm
    Implement an algorithm to merge local playlists and remote playlists by index. If they have the same index, sort them by name.
  • Changing index
    If the index saved in the database doesn't match the actual display index for some reason (e.g., setting to all zero after database migration, or a new playlist is created with index -1 (thus being on the top), or the index is not continuous after deletion, or the user changes the index), save the new index to the database.
  • Debounced saver
    Save the list 10 seconds after the last change occurred. The logic is almost the same as the one in LocalPlaylistFragment.
    Update: now the code is reused.
  • UI and Item holder
    • Create list_playlist_bookmark_item.xml. It's just list_playlist_mini_item.xml adding an handle on the right.
    • Create LocalBookmarkPlaylistItemHolder and RemoteBookmarkPlaylistItemHolder.
    • LocalItemListAdapter will choose the item holder with/without the handle.

Before/After Screenshots/Screen Record

Before:

Before

After:

After.mp4

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

@GGAutomaton
Copy link
Contributor Author

GGAutomaton commented Apr 16, 2022

demo.mp4

The data storage logic has been modified, so more testing is required to avoid data loss.
I'm not familiar with rxjava, therefore some code needs extra checking.

@GGAutomaton GGAutomaton marked this pull request as ready for review April 16, 2022 04:58
@GGAutomaton
Copy link
Contributor Author

GGAutomaton commented Apr 17, 2022

Two of the three code smells are about high complexity methods. But their logic is continuous, I don't think refactoring them is a necessity.
Another warning is about the duplications of item touch callback. It might be ok if not reusing the code.

Please keep me informed if there's anything else to do.

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 have a few comments

@GGAutomaton GGAutomaton marked this pull request as draft May 5, 2022 06:05
@sonarcloud

This comment was marked as outdated.

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
3.7% 3.7% Duplication

@GGAutomaton GGAutomaton marked this pull request as ready for review June 23, 2022 15:43
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.

Almost good, thank you! If you want me to take care of some of these changes, feel free to ask me ;-)

@opusforlife2 opusforlife2 added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface playlist Anything to do with playlists in the app labels Oct 24, 2022
@github-actions github-actions bot added the size/giant PRs with more than 750 changed lines label Mar 29, 2024
Complete removal of unneeded index, and remove default value for `remote_playlists.display_index`.
Remove checkDisplayIndexModified because it was causing more problems than it solved. Now when adding new playlists they won't necessarily appear at the top, but will get sorted alphabetically along with the other playlists with index -1. This will be the case until any playlist is sorted, at which point all indices are assigned and newly added playlists will appear at the top again.
@Stypox
Copy link
Member

Stypox commented Mar 30, 2024

The 5 sonartype issues are due to:

  • some code duplication due to both BookmarkFragment and LocalPlaylistFragment allowing to move items around (but it would make things just more complicated to try to deduplicate this, it's not so much after all)
  • an empty onComplete() method (there's no action to do, and there isn't a specific reason for this, so no need to add a comment to the method)
  • some "commented out code" which is actually a legitimate comment explaining what methods should be used for future reference
  • constructor with 8 parameters required by the Room database

I tested, fixed some things and made the implementation simpler. I tested again and everything seems to work well. I deleted, moved, renamed remote and local playlists and there were no inconsistencies.

Copy link

sonarcloud bot commented Mar 30, 2024

@Stypox Stypox merged commit 4c82388 into TeamNewPipe:dev Mar 30, 2024
6 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
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface playlist Anything to do with playlists in the app size/giant PRs with more than 750 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort bookmarked playlists in main page
3 participants