Skip to content

Beta 25#16

Merged
viestat merged 19 commits intomasterfrom
beta-25
Dec 5, 2017
Merged

Beta 25#16
viestat merged 19 commits intomasterfrom
beta-25

Conversation

@viestat
Copy link
Copy Markdown
Owner

@viestat viestat commented Sep 8, 2017

Implementation for the newest version of Spotify SDK

TODO:

  • Update to RN 48
  • Implement new Methods
  • Remove deprecated Methods
  • Update README.md

@digitaldavenyc
Copy link
Copy Markdown

digitaldavenyc commented Sep 8, 2017

@viestat omg dude you rock! I will test this against our project over the weekend.

Comment thread README.md
* `SpotifyLoginViewController.m`
* `SpotifyLoginViewController.h`
* `SpotifyAuth.m`
* `SpotifyAuth.h`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to add that it's required to add these references to AppDelegate.m

#import <SpotifyMetadata/SpotifyMetadata.h>
#import <SpotifyAudioPlayback/SpotifyAudioPlayback.h>

Comment thread README.md

//Assign our module from NativeModules and assign it to a variable
var SpotifyAuth = NativeModules.SpotifyAuth;
var SpotifyModule = NativeModules.SpotifyModule;
Copy link
Copy Markdown

@digitaldavenyc digitaldavenyc Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is correct, cannot import SpotifyModule, file is still called SpotifyAuth. Trying to load Native Module SpotifyModule returns null. There are references to this everywhere in the README.....

Comment thread README.md Outdated
someMethod(){
//You need this to Auth a user, without it you cant use any method!
SpotifyAuth.setClientID('Your ClientId','Your redirectURL', ['streaming'], (error)=>{
SpotifyModule.setClientID('Your ClientId','Your redirectURL', ['streaming'], (error)=>{
Copy link
Copy Markdown

@digitaldavenyc digitaldavenyc Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should include (error, message)

Comment thread README.md
@@ -87,7 +91,7 @@ ___

### Auth:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing startAuthenticationFlow

@digitaldavenyc
Copy link
Copy Markdown

digitaldavenyc commented Sep 14, 2017

@viestat I have been testing and came across an error on startAuthenticationFlow, I submitted a PR against the beta-25 branch that seems to help with the problem.

screen shot 2017-09-14 at 4 36 53 pm

This message also shows in the console in Xcode: Warning: Attempt to present <UINavigationController: 0x7f8fe15c0c00> on <UIViewController: 0x7f8fe0e14660> while a presentation is in progress!

It might make sense to use the suggestion on issue #10 and use dispatch_async

I feel like this type of authentication is somewhat problematic and frankly not ideal since users have to constantly re-auth, it might solve a lot of problems to use as a second option Authorization Code Flow

}
[sharedIn playURIs:urisArr withOptions:playOptions callback:^(NSError *error) {
// SPTAudioStreamingController *sharedIn = [SPTAudioStreamingController sharedInstance];
[self.player playSpotifyURI:spotifyUri startingWithIndex:index startingWithPosition:position callback:^(NSError *error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callback fails here, should uncomment SPTAudioStreamingController

  SPTAudioStreamingController *sharedIn = [SPTAudioStreamingController sharedInstance];
  [sharedIn playSpotifyURI:spotifyUri startingWithIndex:index startingWithPosition:position callback:^(NSError *error) {

@digitaldavenyc
Copy link
Copy Markdown

digitaldavenyc commented Sep 15, 2017

@viestat Having issues with almost all the NativeEventEmitter events, the only one that is firing is audioStreamingDidLogin, everything else does not fire. Here is an example of our implementation

const SpotifyAuth = require('NativeModules').SpotifyAuth;
const NativeEventEmitter = require('NativeEventEmitter');

class Player extends Component {
	componentWillMount() {
		this.spotify = new NativeEventEmitter(SpotifyAuth);
	}

	componentDidMount() {
		this.spotify.addListener('didStartPlayingTrack', this.onPlaying);
		this.spotify.addListener('didChangePlaybackStatus', (isPlaying) => console.log('didChangePlaybackStatus.isPlaying:', isPlaying));
		this.spotify.addListener('didChangePosition', this.onChangePosition);
		this.spotify.addListener('didReceiveError', (error) => console.log('didReceiveError:', error))
		this.spotify.addListener('audioStreamingDidLogin', (error) => console.log('audioStreamingDidLogin:', error))
		this.spotify.addListener('didReceivePlaybackEvent', (event) => console.log('didReceivePlaybackEvent:', event))
		this.spotify.addListener('didSeekToPosition', (position) => console.log('didSeekToPosition:', position))
		this.spotify.addListener('didChangeMetadata', (metadata) => console.log('didChangeMetadata:', metadata))
		this.spotify.addListener('didStopPlayingTrack', (trackUri) => console.log('didStopPlayingTrack:', trackUri))
		this.spotify.addListener('audioStreamingDidSkipToNextTrack', () => console.log('audioStreamingDidSkipToNextTrack'))
		this.spotify.addListener('audioStreamingDidSkipToPreviousTrack', () => console.log('audioStreamingDidSkipToPreviousTrack'))
		this.spotify.addListener('audioStreamingDidBecomeActivePlaybackDevice', () => console.log('audioStreamingDidBecomeActivePlaybackDevice'))
		this.spotify.addListener('audioStreamingDidBecomeInactivePlaybackDevice', () => console.log('audioStreamingDidBecomeInactivePlaybackDevice'))
		this.spotify.addListener('audioStreamingDidLosePermissionForPlayback', () => console.log('audioStreamingDidLosePermissionForPlayback'))
		this.spotify.addListener('audioStreamingDidPopQueue', () => console.log('audioStreamingDidPopQueue'))
	}
}

@viestat
Copy link
Copy Markdown
Owner Author

viestat commented Sep 19, 2017

@digitaldavenyc I will be working on a fix to the issues you highlighted thanks for your support and patience :)

@digitaldavenyc
Copy link
Copy Markdown

digitaldavenyc commented Sep 20, 2017

@viestat Looking forward to it!

I'd be happy to assist with any README updates or further code fixes. I tried looking into the issues with NativeEventEmitter and don't see anything wrong with the implementation, have you been able to test the NativeEventEmitter events firing successfully?

Do you think it makes sense to rename the SpotifyAuth class to SpotifyModule or update the README references to SpotifyAuth?

@viestat
Copy link
Copy Markdown
Owner Author

viestat commented Sep 30, 2017

@digitaldavenyc Regarding the the NativeEventEmitter events, they do not seem to fire even directly in the native side. So it might be a bug in the SDK itself. I will continue looking into it but you might find that info useful.

@digitaldavenyc
Copy link
Copy Markdown

@viestat are you sure it's an issue with the SDK? It's highly unlikely Spotify would release an SDK with that many broken events. I checked their GitHub issues and didn't see any issues with their events firing. It looks like these events have been part of the SDK since version 20. If it is in fact an issue with Spotify, we should open an issue.

I saw your changes to playbackState, these should be helpful in getting beta-25 version ready until we resolve this NativeEvent issue. Thanks!

@mross22
Copy link
Copy Markdown

mross22 commented Nov 29, 2017

@viestat I think the issue is you need to add the following line to SpotifyAuth.m:
self.player.playbackDelegate = self;

At this location in your PR:
https://github.com/viestat/react-native-spotify/pull/16/files#diff-3cb2dd92e545372fdf88500ec7772f2bR46

The reason you're not seeing events trigger is because you are registering SpotifyAuth as the SPTAudioStreamingDelegate but not the SPTAudioStreamingPlaybackDelegate (so you only get the audio streaming related events and not the playback related events)

@viestat
Copy link
Copy Markdown
Owner Author

viestat commented Nov 30, 2017

@mross22 You are Amazing!
that did it :) !!!!

@viestat viestat merged commit fc6a807 into master Dec 5, 2017
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.

3 participants