Permalink
Browse files

Temporarily removed album-reference in track (caused segfault) + some…

… renaming of variables
  • Loading branch information...
1 parent 58391f3 commit de5fe4a1f8cc6408e8342a6d25646f23820607d6 @liesen committed Jul 20, 2010
Showing with 90 additions and 48 deletions.
  1. +0 −4 src/playlist.cc
  2. +53 −35 src/track.cc
  3. +4 −2 src/track.h
  4. +25 −0 test/test-playlist-iterate-tracks.js
  5. +2 −0 test/test-search-basic.js
  6. +6 −7 wscript
View
@@ -152,10 +152,6 @@ Handle<Value> Playlist::TrackGetter(uint32_t index,
const AccessorInfo& info) {
HandleScope scope;
Playlist* p = Unwrap<Playlist>(info.This());
-
- if (!sp_playlist_is_loaded(p->playlist_))
- return Undefined();
-
@rsms

rsms Jul 21, 2010

Collaborator

Why was this guard removed?

sp_track* track = sp_playlist_track(p->playlist_, index);
if (track == NULL)
View
@@ -2,44 +2,44 @@
#include "album.h"
#include "artist.h"
-Persistent<FunctionTemplate> Track::constructor_template;
+Persistent<FunctionTemplate> Track::constructor_template_;
@rsms

rsms Jul 21, 2010

Collaborator

This should be named "constructor_template" IMHO as the whole of node is designed that way:

@liesen

liesen Jul 21, 2010

Owner

Will revert.

// -----------------------------------------------------------------------------
// Track implementation
-Track::Track(sp_track *track)
- : node::EventEmitter()
- , track_(track)
- , album_(NULL)
-{
+Track::Track(sp_track *track) : track_(track), album_(NULL) {
+ if (track_)
+ sp_track_add_ref(track_);
}
Track::~Track() {
if (track_)
sp_track_release(track_);
+
if (album_)
delete album_;
}
Handle<Value> Track::New(sp_track *track) {
HandleScope scope;
- Local<Object> instance =
- constructor_template->GetFunction()->NewInstance(0, NULL);
+ Local<Function> constructor = constructor_template_->GetFunction();
+ Local<Object> instance = constructor->NewInstance(0, NULL);
Track *p = new Track(track);
- (p)->Wrap(instance);
- if (p->track_) {
- sp_track_add_ref(p->track_);
- if (!p->SetupBackingTrack())
- return JS_THROW(Error, sp_error_message(sp_track_error(p->track_)));
- }
+ p->Wrap(instance);
+
+ if (p->track_ && !p->SetupBackingTrack())
+ return JS_THROW(Error, sp_error_message(sp_track_error(p->track_)));
+
return scope.Close(instance);
@rsms

rsms Jul 21, 2010

Collaborator

I think we're missing a sp_track_add_ref(p->track_); here...

@liesen

liesen Jul 21, 2010

Owner

Constructor does ..._add_ref

}
Handle<Value> Track::New(const Arguments& args) {
- HandleScope scope;
return args.This();
}
+/**
+ * Sets all permanent properties on a track.
+ */
bool Track::SetupBackingTrack() {
if (!track_) return true;
@@ -55,20 +55,24 @@ bool Track::SetupBackingTrack() {
// todo: symbolize keys
- handle_->Set(String::New("name"),
- String::New(sp_track_name(track_)));
+ handle_->Set(String::New("name"), String::New(sp_track_name(track_)));
handle_->Set(String::New("available"),
- Boolean::New(sp_track_is_available(track_)));
+ Boolean::New(sp_track_is_available(track_)));
handle_->Set(String::New("duration"),
- Integer::New(sp_track_duration(track_)));
+ Integer::New(sp_track_duration(track_)));
handle_->Set(String::New("popularity"),
- Integer::New(sp_track_popularity(track_)));
+ Integer::New(sp_track_popularity(track_)));
+
+ // Set properties only available for browse results
+ int track_index = sp_track_index(track_);
+
+ if (track_index)
+ handle_->Set(String::New("discIndex"), Integer::New(track_index));
- // only available for browse results:
- int d = sp_track_index(track_);
- if (d) handle_->Set(String::New("discIndex"), Integer::New(d));
- d = sp_track_disc(track_);
- if (d) handle_->Set(String::New("disc"), Integer::New(d));
+ int disc = sp_track_disc(track_);
+
+ if (disc)
+ handle_->Set(String::New("disc"), Integer::New(disc));
return true;
}
@@ -84,12 +88,16 @@ GETTER_C(Track::LoadedGetter) {
GETTER_C(Track::AlbumGetter) {
HandleScope scope;
Track *p = Unwrap<Track>(info.This());
+
if (!p->track_ || !sp_track_is_loaded(p->track_))
return Undefined();
+
if (!p->album_) {
Local<Object> instance = Album::New(sp_track_album(p->track_));
- p->album_ = ObjectWrap::Unwrap<Album>(instance);
+ // p->album_ = ObjectWrap::Unwrap<Album>(instance);
+ return scope.Close(instance);
}
+
return scope.Close(p->album_->handle_);
}
@@ -108,33 +116,43 @@ GETTER_C(Track::ArtistsGetter) {
scope.Close(array);
}
-GETTER_C(Track::URIGetter) {
+/**
+ * Gets the URI of a track. Replaces getter with the URI (string constant) once
@liesen

liesen Jul 21, 2010

Owner

This is obviously not true (yet?). What do you think of this, Rasmus? I couldn't get it to work during the few minutes I worked on it.

+ * it has been created.
+ */
+Handle<Value> Track::UriGetter(Local<String> property,
+ const AccessorInfo& info) {
HandleScope scope;
Track *p = Unwrap<Track>(info.This());
+
if (!p->track_ || !sp_track_is_loaded(p->track_))
return Undefined();
- char uri_buf[40]; // spotify:track:0EcVDS2dfZR5RytFhFOtYH
+
sp_link *link = sp_link_create_from_track(p->track_, 0);
+
if (!link)
return Undefined();
- sp_link_as_string(link, uri_buf, sizeof(uri_buf));
+
+ const int kUriBufferLen = 40; // spotify:track:0EcVDS2dfZR5RytFhFOtYH
+ char uri_buf[kUriBufferLen];
+ sp_link_as_string(link, uri_buf, kUriBufferLen);
sp_link_release(link);
return scope.Close(String::New(uri_buf));
}
void Track::Initialize(Handle<Object> target) {
HandleScope scope;
Local<FunctionTemplate> t = FunctionTemplate::New(New);
- constructor_template = Persistent<FunctionTemplate>::New(t);
- constructor_template->SetClassName(String::NewSymbol("Track"));
- constructor_template->Inherit(EventEmitter::constructor_template);
+ constructor_template_ = Persistent<FunctionTemplate>::New(t);
+ constructor_template_->SetClassName(String::NewSymbol("Track"));
+ constructor_template_->Inherit(EventEmitter::constructor_template);
- Local<ObjectTemplate> instance_t = constructor_template->InstanceTemplate();
+ Local<ObjectTemplate> instance_t = constructor_template_->InstanceTemplate();
instance_t->SetInternalFieldCount(1);
instance_t->SetAccessor(String::New("loaded"), LoadedGetter);
instance_t->SetAccessor(String::New("album"), AlbumGetter);
instance_t->SetAccessor(String::New("artists"), ArtistsGetter);
- instance_t->SetAccessor(String::New("uri"), URIGetter);
+ instance_t->SetAccessor(String::New("uri"), UriGetter);
- target->Set(String::New("Track"), constructor_template->GetFunction());
+ target->Set(String::New("Track"), constructor_template_->GetFunction());
}
View
@@ -7,19 +7,21 @@ class Album;
class Track : public EventEmitter {
public:
- static Persistent<FunctionTemplate> constructor_template;
static void Initialize(Handle<Object> target);
+
explicit Track(sp_track *track);
~Track();
+
static Handle<Value> New(const Arguments& args);
static Handle<Value> New(sp_track *track);
GETTER_H(LoadedGetter);
GETTER_H(AlbumGetter);
GETTER_H(ArtistsGetter);
- GETTER_H(URIGetter);
+ GETTER_H(UriGetter);
protected:
bool SetupBackingTrack();
+ static Persistent<FunctionTemplate> constructor_template_;
sp_track* track_;
Album *album_;
};
@@ -0,0 +1,25 @@
+require('./test');
+
+
+createSession(function (session) {
+ session.addListener('metadataUpdated', function () { sys.log('metadataUpdated'); });
+ session.playlists.addListener('load', function () {
+ if (this.length > 0) {
+ function f () {
+ this.removeAllListeners();
+
+ for (var i = 0; i < this.length; i++) {
+ assert.ok(this[i] instanceof spotify.Track);
+ }
+
+ if (this.length > 0) {
+ sys.puts(sys.inspect(this[0]));
+ }
+
+ session.logout();
+ }
+
+ this[0].addListener('updated', f);
+ }
+ });
+});
@@ -11,6 +11,8 @@ createSession(function (session) {
assert.strictEqual(typeof result.uri, "string");
assert.strictEqual(result.uri, "spotify:search:"+query);
assert.ok(Array.isArray(result.tracks));
+ sys.puts(result.tracks[0].uri);
+ sys.puts(result.tracks[0].uri);
assert.ok(Array.isArray(result.albums));
assert.ok(Array.isArray(result.artists));
View
13 wscript
@@ -37,13 +37,12 @@ def configure(conf):
def lint(ctx):
Utils.exec_command('python cpplint.py --verbose=0 --filter='+
- '-legal/copyright,'+ # in the future
- '-build/header_guard,'+ # not interesting
- '-whitespace/comments,'+ # not interesting
- '-build/include,'+ # lint is run from outside src
- '-build/namespaces,'+ # we are not building a C++ API
- '-whitespace/braces'+ # some places need { after LF
- ' src/*.cc'+
+ '-legal/copyright,' + # in the future
+ '-build/header_guard,' + # not interesting
+ '-build/include,' + # lint is run from outside src
+ '-build/namespaces,' + # we are not building a C++ API
+ '-whitespace/comments,' +
+ ' src/*.cc' +
' $(find src \! -name queue.h -name *.h)')
def build(ctx):

0 comments on commit de5fe4a

Please sign in to comment.