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

Account for TIDAL redesign. #1698

Merged
merged 1 commit into from
Oct 4, 2018
Merged

Account for TIDAL redesign. #1698

merged 1 commit into from
Oct 4, 2018

Conversation

jaccarmac
Copy link
Contributor

Redesign seemed to roll in late last week.

data-test-id attributes are probably less than ideal but until I absolutely
have to use string matches on class attributes I will not do so.

I tried to follow the old structure as much as possible; Not totally sure why
the duration and album art are not restricted to the player selector.

@jaccarmac
Copy link
Contributor Author

This implementation is bugged w.r.t. multiple artists at the moment. It no longer registers artists with commas properly between their names. I need to write either a wrapper function to do the splicing or a selector which only pulls the first artist. Any strong preference as to which?

@jaccarmac
Copy link
Contributor Author

Clearing the queue seems to do weird things to scrobble detection. Less badly-behaved than the last scrobbler at least but less than ideal.

@inverse inverse added the connector This issue or pull request is related to connectors label Sep 12, 2018
@inverse
Copy link
Member

inverse commented Sep 12, 2018

Thanks for working on this. I don't have a Tidal account so I am unable to test.

What do you mean with comm's between their name? Normally if there needs to be additional parsing on any getter we overload the function if it becomes too complex with a selector.

@jaccarmac
Copy link
Contributor Author

Yep, that's what I mean. The DOM looks like the snippet below, and I'm matching on the data-test-id

<span class="mediaArtists--2L7Wb">
  <a href="/artist/7628878" title="House of Feelings" data-test-id="grid-item-detail-text-title-artist" draggable="false">House of Feelings</a>
  , 
  <a href="/artist/6118403" title="Denitia" data-test-id="grid-item-detail-text-title-artist" draggable="false">Denitia</a>
</span>

@inverse
Copy link
Member

inverse commented Sep 12, 2018

So a track can have multiple artists attributed? how would last.fm handle those cases? We need to think about how that should be handled on the other side since they AFAIK don't support this.

@jaccarmac
Copy link
Contributor Author

Give it a think for sure, since it's not only a problem with Tidal. Weird artist combos are present on YouTube, Spotify, and elsewhere as well. IMO it's really a problem with Last.fm itself.

In any case, my thinking w.r.t. proper solutions is to make an option between two scrobbling styles.

  1. Last.fm style: Primary artist per track, additional artists in a feature list at the end of the track name.
  2. Musicbrainz style: Artists and features in artist field, track title clean.

Personally my preference is for 2, Last.fm works better with 1, and most scrobblers use a mix of both.

@inverse
Copy link
Member

inverse commented Sep 13, 2018

Totally - For example here's one that kinda goes the way you said with 1.

https://www.last.fm/music/Modeselektor/_/Berlin+feat.+Miss+Platnum

@jaccarmac
Copy link
Contributor Author

Alright, I cleaned this up and made it play a little nicer with the queue, should be easy to merge. There's one more issue that I think may be a wider bug since I also see it on Google Play Music, so I may open another ticket for that.

Play/pause state changes aren't getting picked up. It seems that the MutationObserver is not getting triggered by clicking the pause button, which doesn't actually add or remove anything from the DOM. It simply changes the value of some attributes. Not sure what the best way of dealing with that is given all the MutationObserving is being done outside of individual connectors.

@jaccarmac
Copy link
Contributor Author

Not going to ask why but the most recent change made that problem go away. I'll take it. This is ready to go now.

Redesign seemed to roll in late last week.

data-test-id attributes are probably less than ideal but until I absolutely
have to use string matches on class attributes I will not do so.

I tried to follow the old structure as much as possible; Not totally sure why
the duration and album art are not restricted to the player selector.
@inverse
Copy link
Member

inverse commented Oct 4, 2018

Changes look reasonable - I am unable to test though

@inverse inverse merged commit 32f02f7 into web-scrobbler:master Oct 4, 2018
@jaccarmac jaccarmac deleted the tidal-redesign branch October 5, 2018 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector This issue or pull request is related to connectors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants