DD3Boh / OuterTune

A Material 3 Music Player with YouTube Music support for Android. Forked from InnerTune
GNU General Public License v3.0
365 stars 23 forks source link

Singleton sync and chip loader #63

Closed mattcarter11 closed 3 weeks ago

mattcarter11 commented 1 month ago

Feature / Changes

Sync spinner in chips and liked playlist

New Download chip (show only playlists/arts with any downloaded song)

N/M songs

Fixes

mattcarter11 commented 1 month ago

@mikooomich this one has some big files 😅 if anything has to change, I'm more than happy to oblige

mikooomich commented 1 month ago

As in a "this is ready for merge" or suggestions in general? I haven't looked into it too deeply, but looks great so far! A few things I noticed

mattcarter11 commented 1 month ago

@mikooomich db migration removed and fixed the ytmSync being ignored in the playlist tab

mattcarter11 commented 1 month ago

@mikooomich also, what do you think about adding this? A n/m download count on playlists/artists/albums with any downloaded song.

Maybe add an option to toggle this?

mikooomich commented 1 month ago

You cannot modify old db schema like that. You still need to increment db version and specify a database auto migration, see here -> https://github.com/DD3Boh/OuterTune/commit/c3e2ea7dc259209b10ab6666093a0a9f3f9c92c5#diff-841cb2099b74da4d913f1cf1d60c554d9e6ae0dc6f8c3a22ad3b95a4b5b39568R107

A n/m download count on playlists/artists/albums with any downloaded song. Maybe add an option to toggle this?

Yeah, nice touch

If the pull request is wip could you mark it as such (with the selector), then mark it ready when its ready to be reviewed/merged

mattcarter11 commented 1 month ago

@mikooomich change done and added N/M songs

mattcarter11 commented 1 month ago

@mikooomich

mikooomich commented 1 month ago

All database schema should be contained within one commit -> should i squash commits? what if they are mixed with other stuff?

"migrate get download logic to SQL (+performance) and add isDownload to song table" For this, i'd seperate the rest of the sql stuff (another commit) and add is downloaded (one commit) . When you add the isdownloaded field, it comes it side effects: you need to handle migration and room will generate the json for db V16. that should all be contained in that commit.

This also means squash the following since into that commit since that will be handled in the commit you add isdownloaded

Remove any rebase fixup commits (those should be applied within commits themselves) -> what do you mean applied within commits themselves? the commits from the branch used to rebas (dev)?

"add missing import after rebase" should not be its own commit (unless it's alot of fixing and makes sense to otherwise), instead, add it into the commit that needs it. (if you don't know where it came from, honestly you can just leave it as it)

mattcarter11 commented 3 weeks ago

@mikooomich changes done