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

Implement playlists #60

Draft
wants to merge 71 commits into
base: master
Choose a base branch
from
Draft

Implement playlists #60

wants to merge 71 commits into from

Conversation

D0m1nos
Copy link
Contributor

@D0m1nos D0m1nos commented Oct 11, 2024

This pr implements playlists.

TODO:

  • Create/delete a playlist
  • Add/remove a song from a playlist
  • Play songs from a playlist
  • Add playlist to queue
  • "Create playlist" screen
  • Automatically refresh after making a change
  • "Edit playlist" screen
  • Remove a specific song through the ui
  • Tailwind it
  • Edit playlist name
  • Show notices
  • Add context menu when clicking on the three dots (right now it deletes the playlist lol)
  • Add a song to a specified playlist using the button in the song controls
  • Add a song to a specified playlist using the context menu in the songs tab
  • Visual indicator when a song is already in a playlist
  • Button to create a playlist in the context menu

Other features (probably in separate prs):

  • Support dragging items in a playlist (it works but the queue gets messed up, also the order doesn't get saved when exiting the playlist)
  • Fix queue not updating when removing a song from a playlist
  • Playlist search
  • Add specified collections as a playlist

D0m1nos added 30 commits October 9, 2024 20:31
@D0m1nos
Copy link
Contributor Author

D0m1nos commented Nov 3, 2024

Update

This is a small update since this pr is getting a bit big.

The main features work: creating/deleting a playlist, adding/removing songs and playing songs from a playlist.

On the code side here's some changes i made:

  • The playlists table has the name of the playlist as the key and a Playlist as the value
  • Playlist is an object with the name of the playlist, number of songs, duration of the playlist and the songs themselves (originally all of this info was shown on the playlists view, i don't know if it's still needed)
  • The Song type now has an optional playlists property that contains an array of playlists that song is in (the array contains only the name of the playlists)

I have some doubts on the design/functionality so i would like to know your thoughts on these:

  • How should the queue work? Right now it gets created with the songs inside the playlist, but with how the queue system is implemented right now there's a couple of things that don't work. For example you can't add to the queue songs from other playlists (more precisely, you can't add songs that aren't already in the queue because they don't really get added, their position just gets shifted)
  • Should i add a 'Create playlist' item in the context menu when adding a song to a playlist? If so, should the song get instantly added after creating it?
  • Should the user be able to add a song more than once in a playlist?
  • Should notices only show when an error occurs? Right now they're shown when adding a song, creating/deleting a playlist and when an error occurs
  • What is 'multi playlist playback' here? Should it get implemented in this pr?

Sorry for the long comment :essaying: but i wanted to ask instead of doing things randomly >.<

I will mark this pr as ready after this discussion and after the sidebar branch (#88) gets merged as that will cause a couple of conflicts.

@hrfarmer
Copy link
Collaborator

hrfarmer commented Nov 24, 2024

  1. Unless we want to also fix the queue in the pr, it's probably fine to leave it as is for now and do it in a separate one to reduce complexity. (Although it does need to happen soon, it's not in a good state right now)
  2. Yes to both
  3. Yes
  4. I personally think it's fine for notices to show up when creating playlists and stuff, but others maybe have different opinions about it.
  5. Not sure myself, probably need to ask @CaptSiro about that

@duduBTW
Copy link
Collaborator

duduBTW commented Nov 29, 2024

@D0m1nos

I wanted to suggest some UI changes to the PR, but the review was getting way too big, to avoid making our life's hard I created a PR with all the UI changes I was going to request, could you please take a look at it? You can merge it into your master if everything looks good and we can proceed with this one.

D0m1nos#1

Amazing job with the playlist!

@D0m1nos
Copy link
Contributor Author

D0m1nos commented Nov 29, 2024

That's awesome! Thank you so much!! I'll take a look at it in the following days.

@D0m1nos
Copy link
Contributor Author

D0m1nos commented Dec 2, 2024

@duduBTW I left some comments on your PR, please have a look when you have the time.

@duduBTW
Copy link
Collaborator

duduBTW commented Dec 12, 2024

@D0m1nos Fixed and replied to all the comments left there.

@D0m1nos
Copy link
Contributor Author

D0m1nos commented Dec 12, 2024

@duduBTW Thanks! I left a small note, after that i think it's good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants