Skip to content

Commit

Permalink
Add StaveNote and Accidental to Builder API.
Browse files Browse the repository at this point in the history
  • Loading branch information
0xfe committed Aug 13, 2016
1 parent a833ee4 commit 7d0fce6
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 4 deletions.
28 changes: 24 additions & 4 deletions src/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
// *This API is currently DRAFT*

import { Vex } from './vex';
import { Accidental } from './accidental';
import { Renderer } from './renderer';
import { Stave } from './stave';
import { StaveNote } from './stavenote';

// To enable logging for this class. Set `Vex.Flow.Builder.DEBUG` to `true`.
function L(...args) { if (Builder.DEBUG) Vex.L('Vex.Flow.Builder', args); }
Expand Down Expand Up @@ -64,13 +66,12 @@ export class Builder {
}

initRenderer() {
if (this.options.renderer.el === '') {
const o = this.options.renderer;

This comment has been minimized.

Copy link
@Silverwolf90

Silverwolf90 Aug 13, 2016

Contributor

This is a personal style thing, so feel free to ignore. But I find single-letter/abbreviated variable names to be very difficult to read (unless in a mathematical context).

How about using destructuring:

const { el, backend, width, height, background } = this.options.renderer

This comment has been minimized.

Copy link
@0xfe

0xfe Aug 13, 2016

Author Owner

+1

This comment has been minimized.

Copy link
@0xfe

0xfe Aug 13, 2016

Author Owner

I'll address all your comments in the next commit, and also rename this to Factory. (Keep 'em coming.)

if (o.el === '') {
throw new X('HTML DOM element not set in Builder');
}

this.renderer = new Renderer(this.options.renderer.el, this.options.renderer.backend);
this.renderer.resize(this.options.renderer.width, this.options.renderer.height);
this.ctx = this.renderer.getContext();
this.ctx = Renderer.buildContext(o.el, o.backend, o.width, o.height, o.background);
}

getContext() { return this.ctx; }
Expand All @@ -96,6 +97,25 @@ export class Builder {
return stave;
}

StaveNote(noteStruct) {
const note = new StaveNote(noteStruct);
note.setStave(this.stave);
note.setContext(this.ctx);
this.renderQ.push(note);
return note;
}

Accidental(params) {
params = setDefaults(params, {
type: null,
options: {},
});

const acc = new Accidental(params.type);

This comment has been minimized.

Copy link
@Silverwolf90

Silverwolf90 Aug 15, 2016

Contributor

I can understand shortening accidental, but I'm really not a a fan of acc -- I always read it as "ack!".

What if we moved to using accid instead of acc and similarly artic for articulation. These abbreviations are what what MEI use and I think it hits a good balance between readability and brevity.

// acc.render_options.stroke_px = this.px(0.3);
return acc;
}

draw() {
this.renderQ.forEach(i => i.setContext(this.ctx).draw());
}
Expand Down
67 changes: 67 additions & 0 deletions tests/accidental_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Vex.Flow.Test.Accidental = (function() {
Vex.Flow.Test.runTests("Automatic Accidentals - No Accidentals Necsesary", Vex.Flow.Test.Accidental.automaticAccidentals2);
Vex.Flow.Test.runTests("Automatic Accidentals - Multi Voice Inline", Vex.Flow.Test.Accidental.automaticAccidentalsMultiVoiceInline);
Vex.Flow.Test.runTests("Automatic Accidentals - Multi Voice Offset", Vex.Flow.Test.Accidental.automaticAccidentalsMultiVoiceOffset);
Vex.Flow.Test.runTests("Builder API", Vex.Flow.Test.Accidental.builderAPI);
},

showNote: function(note, stave, ctx, x) {
Expand Down Expand Up @@ -674,6 +675,72 @@ Vex.Flow.Test.Accidental = (function() {
equal(hasAccidental(notes[8]), false, "Double sharp rememberd");
equal(hasAccidental(notes[9]), true, "Added natural");
equal(hasAccidental(notes[10]), false, "Natural remembered");
},

builderAPI: function(options) {
var vf = VF.Test.makeBuilder(options, 700, 240);
var assert = options.assert;

var stave = vf.Stave({x: 10, y: 10, width: 550});

function newNote(note_struct) { return vf.StaveNote(note_struct); }
function newAcc(type) { return vf.Accidental({type: type}); }

var notes = [
newNote({ keys: ["c/4", "e/4", "a/4"], duration: "w"}).
addAccidental(0, newAcc("b")).
addAccidental(1, newAcc("#")),

newNote({ keys: ["d/4", "e/4", "f/4", "a/4", "c/5", "e/5", "g/5"],
duration: "h"}).
addAccidental(0, newAcc("##")).
addAccidental(1, newAcc("n")).
addAccidental(2, newAcc("bb")).
addAccidental(3, newAcc("b")).
addAccidental(4, newAcc("#")).
addAccidental(5, newAcc("n")).
addAccidental(6, newAcc("bb")),

newNote({ keys: ["f/4", "g/4", "a/4", "b/4", "c/5", "e/5", "g/5"],
duration: "16"}).
addAccidental(0, newAcc("n")).
addAccidental(1, newAcc("#")).
addAccidental(2, newAcc("#")).
addAccidental(3, newAcc("b")).
addAccidental(4, newAcc("bb")).
addAccidental(5, newAcc("##")).
addAccidental(6, newAcc("#")),

newNote({ keys: ["a/3", "c/4", "e/4", "b/4", "d/5", "g/5"], duration: "w"}).
addAccidental(0, newAcc("#")).
addAccidental(1, newAcc("##").setAsCautionary()).
addAccidental(2, newAcc("#").setAsCautionary()).
addAccidental(3, newAcc("b")).
addAccidental(4, newAcc("bb").setAsCautionary()).
addAccidental(5, newAcc("b").setAsCautionary()),
];

var init = function(note, x) {
var mc = new Vex.Flow.ModifierContext();
note.addToModifierContext(mc);

var tickContext = new Vex.Flow.TickContext();
tickContext.addTickable(note).preFormat().setX(x).setPixelsUsed(65);
return note;
}

for (var i = 0; i < notes.length; ++i) {

This comment has been minimized.

Copy link
@Silverwolf90

Silverwolf90 Aug 13, 2016

Contributor

As you probably saw in the StaveNote tests, since this is where you grabbed the code, the following pattern is duplicated in the tests quite often. But, in my opinion, this is really "unidiomatic VexFlow." I think it's more evidence that VexFlow is missing a "Simple Formatter" (see #409) which positions objects by simply advancing an x position by some arbitrary value.

This comment has been minimized.

Copy link
@0xfe

0xfe Aug 13, 2016

Author Owner

Yeah agreed. Although I wonder if we just need better testing abstractions and functions to help with this. i.e., instead of a public Simple Formatter, just a lightweight Test Formatter. The intent here is to isolate the parts of the API to unit test.

init(notes[i], 30 + (i * 125));
var accidentals = notes[i].getAccidentals();
assert.ok(accidentals.length > 0, "Note " + i + " has accidentals");

for (var j = 0; j < accidentals.length; ++j) {
assert.ok(accidentals[j].width > 0, "Accidental " + j + " has set width");
}
}

vf.draw();
assert.ok(true, "Builder API");
}
};

Expand Down

0 comments on commit 7d0fce6

Please sign in to comment.