Skip to content

Conversation

@adammw
Copy link
Collaborator

@adammw adammw commented Jun 19, 2013

Adds a couple of fields, most notably (to me at least) the listing of 30 second song previews.

@TooTallNate
Copy link
Owner

Out of curiosity, where/how are you getting these updates? I wrote a quick little script the other day that supposedly dumps the .proto files from spotify, but they seemed incomplete and missing the import headers...

TooTallNate added a commit that referenced this pull request Jun 19, 2013
Update metadata protobuf schema
@TooTallNate TooTallNate merged commit 3ff8c2f into TooTallNate:master Jun 19, 2013
@TooTallNate
Copy link
Owner

Thanks!

@adammw
Copy link
Collaborator Author

adammw commented Jun 19, 2013

These changes are from the data.xml file that it loads, https://play.spotify.edgekey.net/client/c46d547/proto/data.xml
I've also found some others (with comments even) for some of the plugin modules that get loaded as they are used, e.g. https://spapps.spotify.edgekey.net/radio/2.0.15/proto/radio.proto

I expect these changes are because they have changed the protocol to support more features, not that you missed anything. Previews for example have only just launched.

@TooTallNate
Copy link
Owner

I see. And do you have example code of playing one of these preview files?

P.S. I gave you commit access to this repo today, since you've been rocking it. Let's still do PR's when merging in code, but I'd love the extra set of eyes on other PR's and issues that get opened. Cheers dude!

@adammw
Copy link
Collaborator Author

adammw commented Jun 20, 2013

Trivial example is at https://github.com/adammw/node-spotify-web/blob/preview/example/playPreview.js
I didn't make a PR for that because I didn't think the example really did enough that was non-trivial, besides providing the base url. I was thinking it might be nicer to include either a .previewUri getter or something like .playPreview() method on Track so that the URL base (and also perhaps the agent request/response in the case of .playPreview()) is included in the library and consumers don't have to worry about what the base is, or having to rewrite code if the scheme for accessing the previews changes, but I wanted to get your take on it.

Thanks for the commit access, I'm not sure how much I'll use it, I'm hesitant to make any changes without consulting you in case I stuff something up or missing something in a PR. While I'm on the topic, I think I have asked you already but not sure if you saw it, do you have a tool or style guide your working off when you do those lint commits? Or is that just you going through the code by-hand?

@TooTallNate
Copy link
Owner

I was thinking it might be nicer to include either a .previewUri getter or something like .playPreview() method on Track so that the URL base (and also perhaps the agent request/response in the case of .playPreview()) is included in the library and consumers don't have to worry about what the base is, or having to rewrite code if the scheme for accessing the previews changes, but I wanted to get your take on it.

That sounds great to me and I was thinking basically the same thing. Side question: Do you know if the other file types are accessible via this same technique? i.e. OGG_VORBIS_160, MP3_320, etc.

Thanks for the commit access, I'm not sure how much I'll use it, I'm hesitant to make any changes without consulting you in case I stuff something up or missing something in a PR.

Ya definitely, let's do PRs still. I think it's more just to "auto-watch" the project for you ;) And if there's some PRs from other people that are simple bug fixes and straightforward, fell free to merge as well. API changes let's discuss first.

While I'm on the topic, I think I have asked you already but not sure if you saw it, do you have a tool or style guide your working off when you do those lint commits? Or is that just you going through the code by-hand?

I just run jshint. I guess I should commit a .jshintrc file to the repo for there to be common settings.

Thanks again for the help!

@adammw
Copy link
Collaborator Author

adammw commented Jul 7, 2013

@TooTallNate

I was thinking it might be nicer to include either a .previewUri getter or something like .playPreview() method on Track so that the URL base (and also perhaps the agent request/response in the case of .playPreview()) is included in the library and consumers don't have to worry about what the base is, or having to rewrite code if the scheme for accessing the previews changes, but I wanted to get your take on it.

That sounds great to me and I was thinking basically the same thing.

Ok. I apologise for the delay, I was working on some other projects.
ea9f372 (which is on the preview branch of my local repo) adds both the getter and the method. I decided that I may as well separate the two as I actually only wanted to get the URLs sometimes rather than only being able to play them. Obviously though, the url method is not as flexible and any consumers should be using the playPreview method unless there is a good reason they can't. I also updated the example so it matches the existing play example. The good thing about previews in general, is that at the moment, anyone who wants to try out the library can listen to previews with a free account, whereas full songs require a subscription, which sounds fair.

Did you want me to submit a separate PR for this or can you review/merge it directly?

Side question: Do you know if the other file types are accessible via this same technique? i.e. OGG_VORBIS_160, MP3_320, etc.

I don't think so. The previews are returned directly within the Track object, so while it's possible you will get that information as it is an array and each object does specify the file format, I haven't seen anything besides MP3_96. In fact, this patch essentially assumes that as it doesn't check the file type, and goes just for the first one.

Thanks for the commit access, I'm not sure how much I'll use it, I'm hesitant to make any changes without consulting you in case I stuff something up or missing something in a PR.

Ya definitely, let's do PRs still. I think it's more just to "auto-watch" the project for you ;) And if there's some PRs from other people that are simple bug fixes and straightforward, fell free to merge as well. API changes let's discuss first.

Ok, will continue to do PRs. If you get around to it, can you review the existing PRs (#20, #22, #24)?

#20 by me, refactors a lot of the code around Mercury request to make calls easier, and is loosely modelled around how the Web Client makes the calls.

#22 by me, builds upon #20 (ie it is essentially a dependency - it uses the new calling methods) to provide support for working with playlists including a new Playlist object (ala Track and Artist objects, although slightly different in terms of the constructor), adding items to playlists/rootlist, removing items from a playlist/rootlist, creating playlists, modifying playlist attributes, and "deleting" playlists.

#24 by @sashman-jaspin, also does playlists, creation and adding items.

My very biased opinion is to use my PR instead of @sashman-jaspin, but I'll leave the decision to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants