From dc27c5b9665d4c9de5ec19944cd276b5f748d157 Mon Sep 17 00:00:00 2001 From: Wim Rijnders Date: Sun, 15 Oct 2017 09:44:40 +0200 Subject: [PATCH 1/4] Network: force array order when sorting hierarchical levels Fixes #340r34. If coordinates are not available to sort within a hierarchical level, sort to array order instead. The previous fix on this issue was not good enough to circumvent this quirk in the chromium sorting. This should bury it. --- .../modules/components/DirectionStrategy.js | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/network/modules/components/DirectionStrategy.js b/lib/network/modules/components/DirectionStrategy.js index b72e127eb..d700d9ad9 100644 --- a/lib/network/modules/components/DirectionStrategy.js +++ b/lib/network/modules/components/DirectionStrategy.js @@ -87,6 +87,11 @@ class DirectionInterface { /** * Sort array of nodes on the unfixed coordinates. * + * **Note:** chrome has a tendency to change the order of the + * array items anyway if the custom sort function returns 0. + * This sort forces the issue by falling back to the array order + * if the compare values can't be determined. + * * @param {Array.} nodeArray array of nodes to sort */ sort(nodeArray) { this.fake_use(nodeArray); this.abstract(); } @@ -108,6 +113,25 @@ class DirectionInterface { * @param {number} diff Offset to add to the unfixed coordinate */ shift(nodeId, diff) { this.fake_use(nodeId, diff); this.abstract(); } + + + /** + * Map node id's in array to their position within the array + * + * Sort changes the array inline, we need to keep track of id order separately. + * + * @param {Array.} nodeArray array of nodes + * @returns {object.} hash with node id's to array index + */ + idPerLevel(nodeArray) { + let idOrder = {}; + for (var n = 0; n < nodeArray.length; ++n) { + let item = nodeArray[n]; + idOrder[item.id] = n; + } + + return idOrder; + } } @@ -156,9 +180,12 @@ class VerticalStrategy extends DirectionInterface { /** @inheritdoc */ sort(nodeArray) { + let idOrder = this.idPerLevel(nodeArray); + nodeArray.sort(function(a, b) { - // Test on 'undefined' takes care of divergent behaviour in chrome - if (a.x === undefined || b.x === undefined) return 0; // THIS HAPPENS + if (a.x === undefined || b.x === undefined) { + return idOrder[a.id] - idOrder[b.id]; + } return a.x - b.x; }); } @@ -221,9 +248,12 @@ class HorizontalStrategy extends DirectionInterface { /** @inheritdoc */ sort(nodeArray) { + let idOrder = this.idPerLevel(nodeArray); + nodeArray.sort(function(a, b) { - // Test on 'undefined' takes care of divergent behaviour in chrome - if (a.y === undefined || b.y === undefined) return 0; // THIS HAPPENS + if (a.y === undefined || b.y === undefined) { + return idOrder[a.id] - idOrder[b.id]; + } return a.y - b.y; }); } From 3bbb21b7c18102cff1f4907429d84b1bcd03e447 Mon Sep 17 00:00:00 2001 From: Wim Rijnders Date: Tue, 17 Oct 2017 05:09:10 +0200 Subject: [PATCH 2/4] Added TimSort for sorting in DirectionStrategy --- .../modules/components/DirectionStrategy.js | 46 +++++-------------- package.json | 3 +- 2 files changed, 13 insertions(+), 36 deletions(-) diff --git a/lib/network/modules/components/DirectionStrategy.js b/lib/network/modules/components/DirectionStrategy.js index d700d9ad9..1c6812acb 100644 --- a/lib/network/modules/components/DirectionStrategy.js +++ b/lib/network/modules/components/DirectionStrategy.js @@ -3,6 +3,7 @@ * * Strategy pattern for usage of direction methods for hierarchical layouts. */ +var TimSort = require('timsort'); /** @@ -87,10 +88,14 @@ class DirectionInterface { /** * Sort array of nodes on the unfixed coordinates. * - * **Note:** chrome has a tendency to change the order of the - * array items anyway if the custom sort function returns 0. - * This sort forces the issue by falling back to the array order - * if the compare values can't be determined. + * **Note:** chrome has non-stable sorting implementation, which + * has a tendency to change the order of the array items, + * even if the custom sort function returns 0. + * + * For this reason, an external sort implementation is used, + * which has the added benefit of being faster of the standard + * platforms implementation. This has been verified on `node.js`, + * `firefox` and `chrome` (all linux). * * @param {Array.} nodeArray array of nodes to sort */ @@ -113,25 +118,6 @@ class DirectionInterface { * @param {number} diff Offset to add to the unfixed coordinate */ shift(nodeId, diff) { this.fake_use(nodeId, diff); this.abstract(); } - - - /** - * Map node id's in array to their position within the array - * - * Sort changes the array inline, we need to keep track of id order separately. - * - * @param {Array.} nodeArray array of nodes - * @returns {object.} hash with node id's to array index - */ - idPerLevel(nodeArray) { - let idOrder = {}; - for (var n = 0; n < nodeArray.length; ++n) { - let item = nodeArray[n]; - idOrder[item.id] = n; - } - - return idOrder; - } } @@ -180,12 +166,7 @@ class VerticalStrategy extends DirectionInterface { /** @inheritdoc */ sort(nodeArray) { - let idOrder = this.idPerLevel(nodeArray); - - nodeArray.sort(function(a, b) { - if (a.x === undefined || b.x === undefined) { - return idOrder[a.id] - idOrder[b.id]; - } + TimSort.sort(nodeArray, function(a, b) { return a.x - b.x; }); } @@ -248,12 +229,7 @@ class HorizontalStrategy extends DirectionInterface { /** @inheritdoc */ sort(nodeArray) { - let idOrder = this.idPerLevel(nodeArray); - - nodeArray.sort(function(a, b) { - if (a.y === undefined || b.y === undefined) { - return idOrder[a.id] - idOrder[b.id]; - } + TimSort.sort(nodeArray, function(a, b) { return a.y - b.y; }); } diff --git a/package.json b/package.json index dd09a2ed3..4b6de056b 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,8 @@ "hammerjs": "^2.0.8", "keycharm": "^0.2.0", "moment": "^2.18.1", - "propagating-hammerjs": "^1.4.6" + "propagating-hammerjs": "^1.4.6", + "timsort": "^0.3.0" }, "devDependencies": { "async": "^2.5.0", From 795d62cb6e6a16f967927e2f4fa4afd21eba5548 Mon Sep 17 00:00:00 2001 From: Wim Rijnders Date: Tue, 17 Oct 2017 05:20:27 +0200 Subject: [PATCH 3/4] Added TimSort to LayoutEngine --- lib/network/modules/LayoutEngine.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/network/modules/LayoutEngine.js b/lib/network/modules/LayoutEngine.js index ccee1a438..ad907ebd8 100644 --- a/lib/network/modules/LayoutEngine.js +++ b/lib/network/modules/LayoutEngine.js @@ -1,4 +1,3 @@ -'use strict'; /** * There's a mix-up with terms in the code. Following are the formal definitions: * @@ -30,6 +29,8 @@ * The layout is a way to arrange the nodes in the view; this can be done * on non-hierarchical networks as well. The converse is also possible. */ +'use strict'; +var TimSort = require('timsort'); let util = require('../../util'); var NetworkUtil = require('../NetworkUtil').default; var {HorizontalStrategy, VerticalStrategy} = require('./components/DirectionStrategy.js'); @@ -1422,7 +1423,7 @@ class LayoutEngine { result.push(Number(size)); }); - result.sort(function(a, b) { + TimSort.sort(result, function(a, b) { return b - a; }); From 6b5c58939aebf4cd573ad89f7b1dd0fbd9010081 Mon Sep 17 00:00:00 2001 From: Wim Rijnders Date: Tue, 17 Oct 2017 15:45:53 +0200 Subject: [PATCH 4/4] Fixed typo --- lib/network/modules/components/DirectionStrategy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/network/modules/components/DirectionStrategy.js b/lib/network/modules/components/DirectionStrategy.js index 1c6812acb..ac3151ce5 100644 --- a/lib/network/modules/components/DirectionStrategy.js +++ b/lib/network/modules/components/DirectionStrategy.js @@ -93,7 +93,7 @@ class DirectionInterface { * even if the custom sort function returns 0. * * For this reason, an external sort implementation is used, - * which has the added benefit of being faster of the standard + * which has the added benefit of being faster than the standard * platforms implementation. This has been verified on `node.js`, * `firefox` and `chrome` (all linux). *