cgravolet / scroblr

A lightweight browser extension that scrobbles the music you listen to on the web.
http://scroblr.fm
Other
229 stars 62 forks source link

Don't send update if content is empty #6

Closed mahemoff closed 12 years ago

mahemoff commented 12 years ago

Hi, I'm the creator of Player FM, which a Scroblr contributor recently patched in support for. The way my player works is it's always present, but invisible, so Scroblr is sending an empty track on startup, and I suspect this may happen with other audio apps. My suggestion is to update pollSongInfo where it checks if it should send an update:

if (currentsong.name != song.name || currentsong.artist != song.artist)

I would suggest adding a check that $.trim(currentsong.name) isn't empty here. Alternatively, you may wish to default song name and artists to be '' (empty string) instead of 'undefined', though I think the aforementioned check is probably more robust (in case a value includes space characters for some reason).

cgravolet commented 12 years ago

Hey Michael,

Thanks for the suggestion! I've got a few open source contributions I need to test before pushing out an update, I'll be sure this change gets in there.

Charles

On Aug 17, 2012, at 9:19 PM, Michael Mahemoff notifications@github.com wrote:

Hi, I'm the creator of Player FM, which a Scroblr contributor recently patched in support for. The way my player works is it's always present, but invisible, so Scroblr is sending an empty track on startup, and I suspect this may happen with other audio apps. My suggestion is to update pollSongInfo where it checks if it should send an update:

if (currentsong.name != song.name || currentsong.artist != song.artist) I would suggest adding a check that $.trim(currentsong.name) isn't empty here. Alternatively, you may wish to default song name and artists to be '' (empty string) instead of 'undefined', though I think the aforementioned check is probably more robust (in case a value includes space characters for some reason).

— Reply to this email directly or view it on GitHub.

cgravolet commented 12 years ago

Looking into this a little further, pollSongInfo uses the following selector to grab the artist:

artist = $('.permaplayer .meta .track-wrapper .title :first-child').text();

If you enter that in the console on the Player.fm homepage without playing a track, it returns the character "»". Furthermore , if you inspect the request that get's sent to Last.fm you'll notice those characters showing up as the track name / artist. I guess for some reason it's picking up the title/source seperator (») as the first-child when the anchor tag is empty. I think the workaround would be to check first if the player was visible or perhaps if the title/source had a length greater than 1, otherwise return empty strings.

mahemoff commented 12 years ago

I see. Actually it might be due to some update I made, but I think it's probably selecting too much.

Would you mind trying this:

artist: $('.permaplayer .meta .track-wrapper .current-series-link').text(),
name: $('.permaplayer .meta .track-wrapper .current-episode-link').text(),

for the Player FM entry? Combined with the check for zero-length string, it should work.

(I'd try myself, but I haven't set up the project for development. If this quick fix doesn't work, I'll set it up and work it out.)

cgravolet commented 12 years ago

Hey, that's pretty much exactly what I did, then I noticed your reply :)

mahemoff commented 12 years ago

Excellent, does that mean it worked? :)

cgravolet commented 12 years ago

Yea, it seems to be working so far. I'd like to test it some more over the weekend and hopefully push out all these fixes on Sunday. Thanks for your help!

mahemoff commented 12 years ago

Great, thanks for your help too. Looking forward to the update!