jazz-soft / JZZ-midi-SMF

Standard MIDI Files: read / write / play
MIT License
40 stars 5 forks source link

MTrk.note does not update length #5

Closed pearcemerritt closed 5 years ago

pearcemerritt commented 5 years ago
describe('MTrk.note', function() {
  it('should update the length property of the calling MTrk', function() {
    var trk = new JZZ.MIDI.SMF.MTrk();
    trk = trk.ch(0);
    assert.equal(trk.length, 1);

    trk = trk.note('C4', 127);
    assert.equal(trk.length, 2);
  });
});

The above test fails. It can be found in my branch.

I discovered this while trying to write unit tests for some code in an app I've created that uses MTrk. I don't fully understand how MTrk works and my app is working fine as far as I can see and hear, so perhaps this is expected behavior? But, even if it is, it would be nice to have the length properly reflected on the MTrk object so that my unit tests can validate that program behavior is still correct (e.g. midi "noteOn" events have been added to SMF's, etc.) without having to play audio and use human ears to judge :)

From the digging that I've done, part of the issue may be with this line:

MTrk.prototype.send = function(msg) { this._orig.add(this._tick, msg); };

As far as I can tell: because my code builds SMF's using MTrk.ch and then subsequent Mtrk.note and Mtrk.tick calls, it gets in a situation where the MTrk object is converted into a Chan object but this._orig points at the original MTrk object the Chan was converted from. Then any calls to Chan.note result in calls to Chan.send, which updates this._orig and the two objects' states get out of sync. Had the MTrk not been wrapped by the Chan, this would have been pointing at itself and it wouldn't have mattered if calls were made on this or this._orig.

jazz-soft commented 5 years ago

It works correctly. functions like ch() and tick() return a "view" of the original track to which all the writes go. You can change your test to the following:

describe('MTrk.note', function() {
  it.only('should update the length property of the calling MTrk', function() {
    var trk = new JZZ.MIDI.SMF.MTrk();
    var ch = trk.ch(0);
    assert.equal(trk.length, 1);

    ch.note('C4', 127);
    assert.equal(trk.length, 2);
  });
});
pearcemerritt commented 5 years ago

ah, that makes sense! Thanks for clarifying :)