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

add completness information to playlist title #30

Merged

Conversation

AlexLArmstrong
Copy link
Contributor

fixes #29

@Viperinius
Copy link
Owner

I think it would be better to display the progress in the playlist description instead of the title:

  1. We then could keep the current check to find an existing playlist by name (StartsWith could lead to problems if you have multiple playlists like Cool Playlist and Cool)
  2. Jellyfin keeps a playlist.xml for each playlist in a directory named after their initial name, so any playlist with a progress suffix would always keep a directory with that name
  3. We would be more flexible how to format the info as we have more space available

What do you think?

@AlexLArmstrong AlexLArmstrong force-pushed the feature/completeness_information branch from 8b46ae5 to 12bb731 Compare March 30, 2024 23:00
@AlexLArmstrong
Copy link
Contributor Author

AlexLArmstrong commented Mar 30, 2024

  1. true, the starts with check is flawed...
  2. are you suggesting parsing the xml? If we always check against the "original" name, what happens it the Spotify playlist is renamed? (what happens now?)
  3. totally.. but does a playlist description exist? I couldn't find it during tinkering... edit: never mind, found it, in the UI they call it "Overview"

@Viperinius
Copy link
Owner

Viperinius commented Mar 30, 2024

are you suggesting parsing the xml?

No, just an FYI that might confuse someone if they ever need to look at jellyfin's directories e.g. when migrating.
For the plugin to work, this doesn't matter.

totally.. but does a playlist description exist? I couldn't find it during tinkering...

Yes, it currently gets set to the Spotify playlist description if that exists:

playlist.Overview = providerPlaylist.Description;

@AlexLArmstrong
Copy link
Contributor Author

AlexLArmstrong commented Mar 30, 2024

okay, sounds reasonable.
Lets prepend to the description something like:

This playlist is being kept in sync with Spotify playlist at {URL}.
Last synchronization was at {date time}.
Currently 13 out of 20 songs are synchronized.
Changes to the playlist will be lost during synchronization.

{Spotify.Description}

... maybe this is too verbose... what do you think?
Also... would we need to start providing translations?

@Viperinius
Copy link
Owner

What about:

Spotify Sync Info:
* Id: {id}
* Last sync at: {date time}.
* Progress: 13 / 20 tracks.

{Spotify.Description}

Translations won't really be feasible. As far as I know, Jellyfin's localisation is not accessible / extendable by plugins.

@Chaphasilor
Copy link

Can we use a Spotify link instead of an ID in the description? The ID is unnecessarily technical I think.
Also, "Progress" doesn't sound right, what about "found tracks" or "matched tracks". If there are tracks missing on the server, it's not like the "progress" will ever change...

@Viperinius
Copy link
Owner

Hm, how about like this:

Spotify Sync Info:
* Origin: {url}
* Last Sync At: {date time}
* Completion: {Jellyfin count} / {Spotify count} tracks

{Spotify.Description}

The only reason I would prefer the ID is that I don't think we can use any formatting or HTML inside the overview, so it would always show the full url (and without testing I'm not sure if it would actually render as a clickable link).
But I guess the "full" Spotify url is not that long so either or would be fine for me.

@Viperinius
Copy link
Owner

@AlexLArmstrong do you have any time soon to continue with this PR or would you like me to take over to finish it?

I will prepare a new release soon and it would be nice to include this feature if possible :)

@Viperinius
Copy link
Owner

Viperinius commented May 17, 2024

Ok, due to the fact that Jellyfin 10.9 was released in the meantime, the last proposed version on how to display the info in the "description" won't really work anymore 😕
Previously, only the overview field stripped all newlines from the text, but the tagline field did not. This would thus have been the place to put the block:

Spotify Sync Info:
* Origin: {url}
* Last Sync At: {date time}
* Completion: {Jellyfin count} / {Spotify count} tracks

With 10.9, the tagline is also compacted to

Spotify Sync Info: * Origin: {url} * Last Sync At: {date time} * Completion: {Jellyfin count} / {Spotify count} tracks

Edit: Oh, apparently line breaks in the overview field work with 10.9 if multiple '\n' are given. So the formatting should still be possible

@Viperinius
Copy link
Owner

I did the changes to show the completeness info via the description instead of renaming the playlist.
This is what it would look like currently:

image
A few notes:

  • Origin shows the ID and not the URL because as mentioned previously, the URL would be displayed without formatting, which looks kind of meh
  • As said in the last comment, with 10.9 line breaks in the tagline no longer work, but it is still used here instead of the overview field. This is just because it's a lot easier to update (the info block and original description are saved in separate fields)

If anyone has any preferences or suggestions regarding the solution shown in the screenshot, please let me know :)

@Chaphasilor
Copy link

Nice. I think the formatting could be condensed a bit if line breaks aren't used, and I feel like the origin ID isn't all that useful to see, unless the ID isn't shown anywhere else.

Maybe something along the lines of:

Synced 9 out of 14 tracks from Spotify playlist <ID> (at 2024-06-02 15:47:33)

That's a bit less technical but still contains all of the relevant info.

@Viperinius
Copy link
Owner

Viperinius commented Jun 2, 2024

Synced 9 out of 14 tracks from Spotify playlist ID (at 2024-06-02 15:47:33)

Looks good.
The ID is otherwise only "shown" in the plugin settings, so at least non-admins have no other way to know what playlist on Spotify is referenced.

@Viperinius Viperinius marked this pull request as ready for review June 2, 2024 18:11
@Chaphasilor
Copy link

I think the playlist name should make that clear enough. You could add the original name in case the playlist was renamed?

Also, I think if a whole user is synced (and not individual playlists) no playlist ID is shown even on the dashboard, even on the dashboard. But again, the playlist name should be sufficient for that.

@Viperinius Viperinius merged commit 6ea3b42 into Viperinius:master Aug 12, 2024
4 checks passed
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.

[Feature Request] Add sync state to playlist
3 participants