search

TheWicklowWolf / Syncify

Download Spotify or YouTube playlists on a schedule via yt-dlp.
Apache License 2.0
25 stars 2 forks source link

add support to youtube playlist and fix thumbnail crop #2

Closed iamprem closed 3 months ago

iamprem commented 3 months ago

Have oppurtunities to tidy up this code, but wanted to sent this first revision to see if you are open to pull requests.

TheWicklowWolf commented 3 months ago

Yes, I'm open to pull requests, and this seems like a valuable addition. Thank you for contributing!

A few notes:

  1. This should ideally support playlists from both YouTube and YouTube Music.
  2. Regarding the thumbnails, could you provide a screenshot to clarify what you're referring to? As I haven't noticed this issue myself..
  3. The playlist name is used as the folder name, so while using a Playlist UUID is a useful idea, I'm not sure if its much of an improvement.
    • The current correct flow to change an playlist name is:
      • Hit Edit -> Rename "Playlist Name"
      • Save changes and Close the Popup
      • Then "Save Sync List" on the main UI
      • Manually rename folder

So I'm not entirely sure if this is worth perusing; if you change the playlist name in the UI, you would just need to manually update the folder name to match. Additionally, you would have to address existing playlists that don't have a UUID.

iamprem commented 3 months ago

Thanks for the feedback.

Do you see how it has the padding on the the either side of the album art. The post processor command basically eliminates it by cropping the longer dimension to match the shorter dimension and makes it square.

Heavydirtysoul - twenty one pilots

Supported URL formats: As long as the url has youtube in it and a list query params with a valid playlistId it should work. Tested on the two format that I could get from YTMusic and YT.

iamprem commented 3 months ago

Let me know if this looks good to you.

TheWicklowWolf commented 3 months ago

Thanks for the feedback.

Do you see how it has the padding on the the either side of the album art. The post processor command basically eliminates it by cropping the longer dimension to match the shorter dimension and makes it square.

So what does it do when the image doesn't have any padding?

* I'll revert the playlist ID change as I misunderstood the intented use.

* I'll also revert the audio format as it seems that 251 is the best quality from youtube. Maybe we can fallback to `bestaudio` instead of `best` because `best` looks for a format that has both audio and video and I'm not sure if it will always have the best audio quality.

Yes, fallback to bestaudio is a good idea.

Supported URL formats: As long as the url has youtube in it and a list query params with a valid playlistId it should work. Tested on the two format that I could get from YTMusic and YT.

* https://music.youtube.com/playlist?list=PLrs49TmHs0BtBvAnt7bvig5SqGNG7S3hT

* https://www.youtube.com/watch?v=w1Smzzw_w7Q&list=PLrs49TmHs0BtBvAnt7bvig5SqGNG7S3hT

Perfect.

TheWicklowWolf commented 3 months ago

Let me know if this looks good to you.

This looks good, thanks again for you effort.

Could you also make the cropping feature conditional based on an environment variable, with the default set to off? This is mainly because I’ll likely use this in other projects and want to add the feature without altering the default behavior.

iamprem commented 3 months ago

So what does it do when the image doesn't have any padding?

If the link is for youtube video (instead of a youtube music song), it will still crop the youtube thumbnail to square losing left and right side. You are right, I should make this optional as users may not like cropped thumbnails in some cases.

iamprem commented 3 months ago

To keep all configurations in one place, should we add "Force square album art" checkbox in the "Configuration" modal instead?

TheWicklowWolf commented 3 months ago

To keep all configurations in one place, should we add "Force square album art" checkbox in the "Configuration" modal instead?

No, the "Configuration" modal is kinda full already. I think just put it as an environmental variable and include the details in the readme.

iamprem commented 3 months ago

Moved the album art crop behind an env variable and updated readme. Let me know if this is good.

TheWicklowWolf commented 3 months ago

Two minor things:

I'll merge it then. Sorry for being a bit pedantic, and I appreciate your efforts here.

iamprem commented 3 months ago

No worries. Addressed both comments.

TheWicklowWolf commented 3 months ago

Thanks again for your efforts on this.

I apologize for the code quality; it was put together somewhat haphazardly without much planning, so it’s a bit rough.

If there's anything else you'd like to work on, feel free to submit pull requests. All the best!