Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Commit

Permalink
Network: Retain constraint values in label font handling (#3520)
Browse files Browse the repository at this point in the history 
* Network: Retain constraint values in label font handling

Fixes #3517.

Due to changed logic in the label font handling, the option values for `widthConstraint` and `heightConstraint` were overwritten.
The fix is in effect a reversal of two code lines: parsing constraint options should come *after* parsing (multi)font options.

Further changes:

- Additional 1-liner fix: constraint values were not copied for edge-instance specific options.
- Small refactoriing of `Label#constrain()` in order to separate concerns
- Added unit test for regression testing of this issue.

This leads to the curious observation that, while the actual change is two lines of source code, this resulted in a +-150 line regression test.

* Made unit test more linear, removed tabs

* Made 'enhanced subset' of unit test

* Removed TODO from comment

* Culled redundant nodes from unit test
  • Loading branch information
@wimrijnders @yotamberk
wimrijnders authored and yotamberk committed Oct 8, 2017
1 parent 6afc095 commit 4a25c43
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 21 deletions.
3 changes: 2 additions & 1 deletion lib/network/modules/components/Edge.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ class Edge {
'value',
'width',
'font',
'chosen'
'chosen',
'widthConstraint'
];

// only deep extend the items in the field array. These do not have shorthand.
Expand Down
39 changes: 24 additions & 15 deletions lib/network/modules/components/shared/Label.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,48 +144,57 @@ class Label {

/**
* Set the width and height constraints based on 'nearest' value
*
* @param {Array} pile array of option objects to consider
* @returns {object} the actual constraint values to use
* @private
*/
constrain(pile) {
this.fontOptions.constrainWidth = false;
this.fontOptions.maxWdt = -1;
this.fontOptions.minWdt = -1;
// NOTE: constrainWidth and constrainHeight never set!
// NOTE: for edge labels, only 'maxWdt' set
// Node labels can set all the fields
let fontOptions = {
constrainWidth: false,
maxWdt: -1,
minWdt: -1,
constrainHeight: false,
minHgt: -1,
valign: 'middle',
}

let widthConstraint = util.topMost(pile, 'widthConstraint');
if (typeof widthConstraint === 'number') {
this.fontOptions.maxWdt = Number(widthConstraint);
this.fontOptions.minWdt = Number(widthConstraint);
fontOptions.maxWdt = Number(widthConstraint);
fontOptions.minWdt = Number(widthConstraint);
} else if (typeof widthConstraint === 'object') {
let widthConstraintMaximum = util.topMost(pile, ['widthConstraint', 'maximum']);
if (typeof widthConstraintMaximum === 'number') {
this.fontOptions.maxWdt = Number(widthConstraintMaximum);
fontOptions.maxWdt = Number(widthConstraintMaximum);
}
let widthConstraintMinimum = util.topMost(pile, ['widthConstraint', 'minimum'])
if (typeof widthConstraintMinimum === 'number') {
this.fontOptions.minWdt = Number(widthConstraintMinimum);
fontOptions.minWdt = Number(widthConstraintMinimum);
}
}

this.fontOptions.constrainHeight = false;
this.fontOptions.minHgt = -1;
this.fontOptions.valign = 'middle';

let heightConstraint = util.topMost(pile, 'heightConstraint');
if (typeof heightConstraint === 'number') {
this.fontOptions.minHgt = Number(heightConstraint);
fontOptions.minHgt = Number(heightConstraint);
} else if (typeof heightConstraint === 'object') {
let heightConstraintMinimum = util.topMost(pile, ['heightConstraint', 'minimum']);
if (typeof heightConstraintMinimum === 'number') {
this.fontOptions.minHgt = Number(heightConstraintMinimum);
fontOptions.minHgt = Number(heightConstraintMinimum);
}
let heightConstraintValign = util.topMost(pile, ['heightConstraint', 'valign']);
if (typeof heightConstraintValign === 'string') {
if ((heightConstraintValign === 'top')||(heightConstraintValign === 'bottom')) {
this.fontOptions.valign = heightConstraintValign;
if ((heightConstraintValign === 'top')|| (heightConstraintValign === 'bottom')) {
fontOptions.valign = heightConstraintValign;
}
}
}

return fontOptions;
}


Expand All @@ -197,8 +206,8 @@ class Label {
*/
update(options, pile) {
this.setOptions(options, true);
this.constrain(pile);
this.propagateFonts(pile);
util.deepExtend(this.fontOptions, this.constrain(pile));
this.fontOptions.chooser = ComponentUtil.choosify('label', pile);
}

Expand Down
154 changes: 149 additions & 5 deletions test/Label.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class DummyContext {


class DummyLayoutEngine {
positionInitially() {}
positionInitially() {}
}

/**************************************************************
Expand Down Expand Up @@ -72,7 +72,7 @@ describe('Network Label', function() {
let showBlocks = () => {
return '\nreturned: ' + JSON.stringify(returned, null, 2) + '\n' +
'expected: ' + JSON.stringify(expected, null, 2);
}
}

assert.equal(expected.lines.length, returned.lines.length, 'Number of lines does not match, ' + showBlocks());

Expand Down Expand Up @@ -121,7 +121,7 @@ describe('Network Label', function() {
"OnereallylongwordthatshouldgooverwidthConstraint.maximumifdefined",
"One really long sentence that should go over widthConstraint.maximum if defined",
"Reallyoneenormouslylargelabel withtwobigwordsgoingoverwayovermax"
]
]

var html_text = [
"label <b>with</b> <code>some</code> <i>multi <b>tags</b></i>",
Expand Down Expand Up @@ -917,7 +917,7 @@ describe('Shorthand Font Options', function() {
}
}
});
}
}


function dynamicAdd2(network, data) {
Expand All @@ -926,7 +926,7 @@ describe('Shorthand Font Options', function() {
font: '7 Font7 #070707' // Note: this kills the font.multi, bold and ital settings!
}
});
}
}


it('deals with dynamic data and option updates for shorthand', function (done) {
Expand Down Expand Up @@ -1408,6 +1408,150 @@ describe('Shorthand Font Options', function() {
});


/**
*
* The test network is derived from example `network/nodeStyles/widthHeight.html`,
* where the associated issue (i.e. widthConstraint values not copied) was most poignant.
*
* NOTE: boolean shorthand values for widthConstraint and heightConstraint do nothing.
*/
it('Sets the width/height constraints in the font label options', function (done) {
var nodes = [
{ id: 100, label: 'node 100'},
{ id: 210, group: 'group1', label: 'node 210'},
{ id: 211, widthConstraint: { minimum: 120 }, label: 'node 211'},
{ id: 212, widthConstraint: { minimum: 120, maximum: 140 }, group: 'group1', label: 'node 212'}, // group override
{ id: 220, widthConstraint: { maximum: 170 }, label: 'node 220'},
{ id: 200, font: { multi: true }, widthConstraint: 150, label: 'node <b>200</b>'},
{ id: 201, widthConstraint: 150, label: 'node 201'},
{ id: 202, group: 'group2', label: 'node 202'},
{ id: 203, heightConstraint: { minimum: 75, valign: 'bottom'}, group: 'group2', label: 'node 203'}, // group override
{ id: 204, heightConstraint: 80, group: 'group2', label: 'node 204'}, // group override
{ id: 300, heightConstraint: { minimum: 70 }, label: 'node 300'},
{ id: 400, heightConstraint: { minimum: 100, valign: 'top' }, label: 'node 400'},
{ id: 401, heightConstraint: { minimum: 100, valign: 'middle' }, label: 'node 401'},
{ id: 402, heightConstraint: { minimum: 100, valign: 'bottom' }, label: 'node 402'}
];

var edges = [
{ id: 1, from: 100, to: 210, label: "edge 1"},
{ id: 2, widthConstraint: 80, from: 210, to: 211, label: "edge 2"},
{ id: 3, heightConstraint: 90, from: 100, to: 220, label: "edge 3"},
{ id: 4, from: 401, to: 402, widthConstraint: { maximum: 150 }, label: "edge 12"},
];

var container = document.getElementById('mynetwork');
var data = {
nodes: nodes,
edges: edges
};
var options = {
edges: {
font: {
size: 12
},
widthConstraint: {
maximum: 90
}
},
nodes: {
shape: 'box',
margin: 10,
widthConstraint: {
maximum: 200
}
},
groups: {
group1: {
shape: 'dot',
widthConstraint: {
maximum: 130
}
},
// Following group serves to test all font options
group2: {
shape: 'dot',
widthConstraint: {
minimum: 150,
maximum: 180,
},
heightConstraint: {
minimum: 210,
valign: 'top',
}
},
},
physics: {
enabled: false
}
};
var network = new vis.Network(container, data, options);

var nodes_expected = [
{ nodeId: 100, minWdt: -1, maxWdt: 200, minHgt: -1, valign: 'middle'},
{ nodeId: 210, minWdt: -1, maxWdt: 130, minHgt: -1, valign: 'middle'},
{ nodeId: 211, minWdt: 120, maxWdt: 200, minHgt: -1, valign: 'middle'},
{ nodeId: 212, minWdt: 120, maxWdt: 140, minHgt: -1, valign: 'middle'},
{ nodeId: 220, minWdt: -1, maxWdt: 170, minHgt: -1, valign: 'middle'},
{ nodeId: 200, minWdt: 150, maxWdt: 150, minHgt: -1, valign: 'middle'},
{ nodeId: 201, minWdt: 150, maxWdt: 150, minHgt: -1, valign: 'middle'},
{ nodeId: 202, minWdt: 150, maxWdt: 180, minHgt: 210, valign: 'top'},
{ nodeId: 203, minWdt: 150, maxWdt: 180, minHgt: 75, valign: 'bottom'},
{ nodeId: 204, minWdt: 150, maxWdt: 180, minHgt: 80, valign: 'middle'},
{ nodeId: 300, minWdt: -1, maxWdt: 200, minHgt: 70, valign: 'middle'},
{ nodeId: 400, minWdt: -1, maxWdt: 200, minHgt: 100, valign: 'top'},
{ nodeId: 401, minWdt: -1, maxWdt: 200, minHgt: 100, valign: 'middle'},
{ nodeId: 402, minWdt: -1, maxWdt: 200, minHgt: 100, valign: 'bottom'},
];


// For edge labels, only maxWdt is set. We check the rest anyway, be it for
// checking incorrect settings or for future code changes.
//
// There is a lot of repetitiveness here. Perhaps using a direct copy of the
// example should be let go.
var edges_expected = [
{ id: 1, minWdt: -1, maxWdt: 90, minHgt: -1, valign: 'middle'},
{ id: 2, minWdt: 80, maxWdt: 80, minHgt: -1, valign: 'middle'},
{ id: 3, minWdt: -1, maxWdt: 90, minHgt: 90, valign: 'middle'},
{ id: 4, minWdt: -1, maxWdt: 150, minHgt: -1, valign: 'middle'},
];


let assertConstraints = (expected, fontOptions, label) => {
assert.equal(expected.minWdt, fontOptions.minWdt, 'Incorrect min width' + label);
assert.equal(expected.maxWdt, fontOptions.maxWdt, 'Incorrect max width' + label);
assert.equal(expected.minHgt, fontOptions.minHgt, 'Incorrect min height' + label);
assert.equal(expected.valign, fontOptions.valign, 'Incorrect valign' + label);
}


// Check nodes
util.forEach(nodes_expected, function(expected) {
let networkNode = network.body.nodes[expected.nodeId];
assert(networkNode !== undefined && networkNode !== null, 'node not found for id: ' + expected.nodeId);
let fontOptions = networkNode.labelModule.fontOptions;

var label = ' for node id: ' + expected.nodeId;
assertConstraints(expected, fontOptions, label);
});


// Check edges
util.forEach(edges_expected, function(expected) {
let networkEdge = network.body.edges[expected.id];

var label = ' for edge id: ' + expected.id;
assert(networkEdge !== undefined, 'Edge not found' + label);

let fontOptions = networkEdge.labelModule.fontOptions;
assertConstraints(expected, fontOptions, label);
});

done();
});


it('deals with null labels and other awkward values', function (done) {
var ctx = new DummyContext();
var options = getOptions({});
Expand Down

0 comments on commit 4a25c43

Please sign in to comment.