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

Feat: Change current track's favorited state #13

Merged

Conversation

ducktordanny
Copy link
Contributor

With the included changes we'll be able to mark a track as favorited or undo this via the following examples:

  • Favorite: require('apple-music').set_current_track_favorited(true)
  • Undo favorite: require('apple-music').set_current_track_favorited(false)

This command will also handle if the OS version is below 14 (Sonoma) where the favorited property was called loved. This is done via the osascript that @p5quared provided in the related issue's comment.

If the command succeeds it will prompt the change and the name plus the author of the track it was applied for.

With these changes, we're now also able to get the current track with require('apple-music').get_current_track() and it will return the name and author of the track in the following format <name> - <author>.

@ducktordanny ducktordanny marked this pull request as ready for review July 24, 2024 20:08
@p5quared p5quared self-requested a review July 24, 2024 20:36
@p5quared p5quared self-assigned this Jul 24, 2024
@p5quared p5quared added the enhancement New feature or request label Jul 24, 2024
@p5quared p5quared linked an issue Jul 24, 2024 that may be closed by this pull request
Copy link
Owner

@p5quared p5quared left a comment

Choose a reason for hiding this comment

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

I love this feature!

I left some notes. Here's what I'd like to see to merge:

  1. get_current_track should just return the name, and leave it to users to decide what they want.
  2. Split up set_current_track_favorited to favorite_current_track and unfavorite_current_track.

Optional:
What do you think about naming the split functions just favorite() and unfavorite()?
I don't think you can favorite anything besides tracks, and I doubt anyone will be trying to favorite/unfavorite anything besides the currently playing track.
In the future we could add favorite_track_by_name(name) or something if it is desired.

I left some other small suggestions like get_current_artist, I actually think I'd prefer those in separate PRs.
I'm active pretty much every day and am a fan of many tiny easy PRs over smushing everything together.

lua/apple-music/init.lua Outdated Show resolved Hide resolved
@ducktordanny
Copy link
Contributor Author

As I've just checked you can also favorite playlists and albums as well. But I tried this in the app (iPhone) and it seems those don't show up in the built-in favorite songs playlist (I assume it only fetches the favorite tracks in the background), which is weird because then we can't see the favorited albums and playlists in a summary (at least in the app). Anyway, I think this should be included in another PR later on.

@p5quared
Copy link
Owner

As I've just checked you can also favorite playlists and albums as well.

Ha well I stand corrected on this part! I still think we start with assuming just songs and then we can add albums or playlists later.

lua/apple-music/init.lua Outdated Show resolved Hide resolved
lua/apple-music/init.lua Outdated Show resolved Hide resolved
@ducktordanny ducktordanny force-pushed the feat/set-current-track-loved-state branch from 8e34acc to 0c4126c Compare July 25, 2024 15:10
@p5quared
Copy link
Owner

@ducktordanny When you're all set just re-request review from me.

Better name for general function which handles the favorite state
changes. `set_favorite_track_by_name` -> `set_track_favorited_state`
Copy link
Owner

@p5quared p5quared left a comment

Choose a reason for hiding this comment

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

Looks great! ❤️

You are welcome to merge as is and I can do the minor refactor (moving the get_current_trackname() outside of M) if you want. Otherwise, if you do that refactor I'll also approve.

I also reworded some comments. Feel free to disagree if you feel strongly about them.

lua/apple-music/init.lua Outdated Show resolved Hide resolved
lua/apple-music/init.lua Outdated Show resolved Hide resolved
lua/apple-music/init.lua Outdated Show resolved Hide resolved
lua/apple-music/init.lua Outdated Show resolved Hide resolved
lua/apple-music/init.lua Outdated Show resolved Hide resolved
@ducktordanny
Copy link
Contributor Author

ducktordanny commented Jul 26, 2024

Thanks, I made the suggested changes I think those make it better. 👌

@p5quared
Copy link
Owner

Awesome!

I'm merging this as is. I haven't really tested it much so I'll do that tonight and will put it in the next release probably with something for #17 as well.

@p5quared p5quared merged commit 6cb40b6 into p5quared:main Jul 26, 2024
@ducktordanny ducktordanny deleted the feat/set-current-track-loved-state branch July 26, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea: Set current track to loved/favorited?
2 participants