Skip to content
This repository has been archived by the owner on Mar 30, 2021. It is now read-only.

Use gmusic.js internally #520

Merged
merged 1 commit into from
Mar 30, 2016
Merged

Use gmusic.js internally #520

merged 1 commit into from
Mar 30, 2016

Conversation

jacobwgillespie
Copy link
Member

This supersedes work done at #402. It was easier to re-implement than to deal with the many merge conflicts. 😄

Still a work in progress

@jacobwgillespie
Copy link
Member Author

This appears to be working except for an issue encountered with gmusic.js: gmusic-utils/gmusic.js#33

@chrismou
Copy link
Member

Ha! Yeah, probably a good shout.

Will need to re-add the last.fm scrobbling stuff. It's gone against since you ported it to a new branch

@chrismou
Copy link
Member

I can't remember what else there was that was broken. I seem to remember some weirdness with the mini player? It's a bit of a haze. I'll check out the branch tomorrow and check it

@jacobwgillespie
Copy link
Member Author

Okay, @radiant-player/radiant-player-mac, this is ready for review. gmusic-utils/gmusic.js#35 was merged and that resolved all remaining issues I've encountered or that existed in #402. Everything looks like it's working to me.

I decided that it's probably not a good idea to check the entire node_modules dir into source control, so I gitignore'd it and created scripts/update-javascript-dependencies.sh. There's a note in the README about it.

@jacobwgillespie
Copy link
Member Author

Oh and @chrismou I added the LastFM fix back as well. 👍

@chrismou
Copy link
Member

Ah yeah, the seconds/milliseconds last.fm fix. I knew I'd changed something but couldn't remember what it was ;-)

I've built the branch and I'll keep using it for the rest of the day to see if I notice anything. Will post back here later 👍

Only thing I've noticed so far is it's spitting a lot of debug out to console. Probably worth switching some of that before we merge.

@jacobwgillespie
Copy link
Member Author

Good catch - disabled debug messages :)

@chrismou
Copy link
Member

Looks like the miniplayer is still being funny. All songs are showing as thumbed up for me (but only in the miniplayer). Clicking the icon does what it's expected to do (loves, unloves) but the icon state never changes

screenshot 2016-03-30 14 17 50

@jacobwgillespie
Copy link
Member Author

Any more details on how to reproduce? It's working for me, but this is what I'm doing:

  • Start a playlist that has a liked track down the list
  • The mini player is showing no rating
  • Skip forward to that track
  • Miniplayer is showing thumbs up
  • Skip to next song
  • Miniplayer shows no rating

I've tried forward and backwards, stopping and starting, controls in the miniplayer / keyboard / app and it seems to work. You may be doing something different though.

@chrismou
Copy link
Member

OK, I've not been able to reproduce this since. I'll assume it was cache until I can prove otherwise

@ebramanti
Copy link
Contributor

@jacobwgillespie I would encourage you to also clean up the diff. A lot of the whitespace you removed have made this diff harder to read. Might be useful to do some linting like this in a separate commit.

@chrismou
Copy link
Member

All the whitespace changes are in one file, so it's not as bad as it looks.

All looks good to me. Not had any issues in the last 5 hours. last.fm stuff all seems to still be working. Mini player working as expected and interacting with the web view OK

💪:shipit:👉

@ebramanti
Copy link
Contributor

@chrismou @jacobwgillespie 👍 to shipping this :shipit:

@jacobwgillespie
Copy link
Member Author

Sweet - I may do a separate PR sometime for whitespace cleanup. :)

@jacobwgillespie jacobwgillespie merged commit 9d57ecb into master Mar 30, 2016
@jacobwgillespie jacobwgillespie deleted the gmusic-utils branch March 30, 2016 20:00
@ebramanti ebramanti mentioned this pull request Mar 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants