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 semantic class tags for currently playing song #1364

Closed
KucharczykL opened this issue Sep 24, 2021 · 7 comments
Closed

Add semantic class tags for currently playing song #1364

KucharczykL opened this issue Sep 24, 2021 · 7 comments

Comments

@KucharczykL
Copy link

KucharczykL commented Sep 24, 2021

Is your feature request related to a problem? Please describe.

I use the WebScrobbler extension (https://github.com/web-scrobbler/web-scrobbler) to scrobble songs I'm listening to in my browser to services other than Last.fm and it worked with Navidrome until recently.

I believe currently it's broken because the .songInfo class was removed in aa72d3d
Looking at their repository, I think WebScrobbler was relying on it: web-scrobbler/web-scrobbler@5e62c92

Describe the solution you'd like

It would be nice if the album and artist of the currently playing song were identified as such, for example using classes.
The reason why I ask this is that at the moment the information about artist, album, and year are a single string of text separated by dashes. This is possible to parse of course but if the order or position is ever changed, there would need to be another fix.
The parsing would also fail if any of them contained the - character.
Currently we have this:

<span class="jss407">Emika - Dva - 2013</span>

And I am asking it to be changed to something like this:

<span class="jss407">
 <span class="songArtist">Emika</span> - <span class="songAlbum">Dva</span> - 2013
</span>

(The year is not really important for this purpose but I suppose it might make sense to do the same with it.)

Describe alternative solutions that would also satisfy this problem

Change nothing in Navidrom and just fix the Navidrome connector in the Web Scrobbler extension.
This would be quite fragile as any changes to the layout or order would probably break it again.

Additional context

image

@deluan
Copy link
Member

deluan commented Sep 26, 2021

Yeah, this kind of issues may happens from time to time with solutions that don't use well supported APIs. I actually fixed WebScrobbler in the past due to a similar issue, but as we restructured the AudioTitle component, this came up again. It is hard to keep track of third-party integrations, and make sure it all works...

I may take a look at this later, but a better approach is just use Navidrome's own scrobble features, specially because it is buffered and retriable, meaning that if Last.fm goes down or connectivity is broken, ND keeps retrying until it can submit the scrobbles, even if you close the browser or restart the server.

@KucharczykL
Copy link
Author

KucharczykL commented Sep 30, 2021

I do appreciate Navidrome having its own Last.fm integration but I also use other services (LibreFM, ListenBrainz) and I use other services, including YouTube Music, Spotify, and Deezer. Having them all scrobble to the same services from one spot is a great!

That said, what would be a better solution to fix this long-term other than scraping and having the information in semantic tags like I described above? I assume the API requires authorization, right?

@deluan
Copy link
Member

deluan commented Oct 4, 2021

Ah yes, if you use other services, Navidrome does not support them (yet).

That said, what would be a better solution to fix this long-term other than scraping and having the information in semantic tags like I described above? I assume the API requires authorization, right?

Yeah, I was not saying there is a better solution for something like a multisource scraper, just that scraping (and other form of non-documented integration) are always susceptible to such breaks.

Unfortunately I don't have time to dig into this issue and fix it myself, but would be glad to accept a PR that address this.

@KucharczykL
Copy link
Author

See #1392

@deluan deluan closed this as completed in ee8943f Oct 7, 2021
@deluan
Copy link
Member

deluan commented Oct 7, 2021

Hey @KucharczykL, thanks for the PR. I checked it, but it broke some styles. And because the previous code also had some small issues, I decided to fix it myself. I didn't included the songYear as it is not really required for this use case and it would complicate things a bit, making the AudioTitle component even harder to read. Hope this is fine for you.

@KucharczykL
Copy link
Author

No problem at all, I'm just sorry I couldn't be of more help. I will take a look at how you fixed the issue and compare it to my naive solution to see how it's supposed to be done.

Thank you for fixing this!

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants