From 4a25c43e4c796bf443395a8456b62f1ea84c1511 Mon Sep 17 00:00:00 2001 From: wimrijnders Date: Sun, 8 Oct 2017 18:06:50 +0200 Subject: [PATCH] Network: Retain constraint values in label font handling (#3520) * 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 --- lib/network/modules/components/Edge.js | 3 +- .../modules/components/shared/Label.js | 39 +++-- test/Label.test.js | 154 +++++++++++++++++- 3 files changed, 175 insertions(+), 21 deletions(-) diff --git a/lib/network/modules/components/Edge.js b/lib/network/modules/components/Edge.js index b1112e1d..653eb9f6 100644 --- a/lib/network/modules/components/Edge.js +++ b/lib/network/modules/components/Edge.js @@ -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. diff --git a/lib/network/modules/components/shared/Label.js b/lib/network/modules/components/shared/Label.js index cfc53a65..1e400212 100644 --- a/lib/network/modules/components/shared/Label.js +++ b/lib/network/modules/components/shared/Label.js @@ -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; } @@ -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); } diff --git a/test/Label.test.js b/test/Label.test.js index de5cb252..e92b43c7 100644 --- a/test/Label.test.js +++ b/test/Label.test.js @@ -32,7 +32,7 @@ class DummyContext { class DummyLayoutEngine { - positionInitially() {} + positionInitially() {} } /************************************************************** @@ -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()); @@ -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 with some multi tags", @@ -917,7 +917,7 @@ describe('Shorthand Font Options', function() { } } }); - } + } function dynamicAdd2(network, data) { @@ -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) { @@ -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 200'}, + { 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({});