From 7ca62a0056f35645e74c3bc643ca22c471212295 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 14:26:10 -0400 Subject: [PATCH 1/2] add Plots.didMarginChange helpers - called to determine if old/new margins are different, which should give better result than JSON.strignify() --- src/plot_api/plot_api.js | 4 +-- src/plots/plots.js | 25 ++++++++++---- test/jasmine/tests/plots_test.js | 56 ++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 5e537b4c48c..81d227918cb 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -198,7 +198,7 @@ function plot(gd, data, layout, config) { * start async-friendly code - now we're actually drawing things */ - var oldmargins = JSON.stringify(fullLayout._size); + var oldMargins = Lib.extendFlat({}, fullLayout._size); // draw framework first so that margin-pushing // components can position themselves correctly @@ -311,7 +311,7 @@ function plot(gd, data, layout, config) { // in case the margins changed, draw margin pushers again function marginPushersAgain() { - if(JSON.stringify(fullLayout._size) === oldmargins) return; + if(!Plots.didMarginChange(oldMargins, fullLayout._size)) return; return Lib.syncOrAsync([ marginPushers, diff --git a/src/plots/plots.js b/src/plots/plots.js index 3ac177d31bc..8a94588b61b 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1802,7 +1802,9 @@ plots.autoMargin = function(gd, id, o) { pushMarginIds[id] = 1; } - if(!fullLayout._replotting) plots.doAutoMargin(gd); + if(!fullLayout._replotting) { + plots.doAutoMargin(gd); + } } }; @@ -1812,8 +1814,8 @@ plots.doAutoMargin = function(gd) { initMargins(fullLayout); var gs = fullLayout._size; - var oldmargins = JSON.stringify(gs); var margin = fullLayout.margin; + var oldMargins = Lib.extendFlat({}, gs); // adjust margins for outside components // fullLayout.margin is the requested margin, @@ -1892,10 +1894,7 @@ plots.doAutoMargin = function(gd) { gs.h = Math.round(height) - gs.t - gs.b; // if things changed and we're not already redrawing, trigger a redraw - if(!fullLayout._replotting && - oldmargins !== '{}' && - oldmargins !== JSON.stringify(fullLayout._size) - ) { + if(!fullLayout._replotting && plots.didMarginChange(oldMargins, gs)) { if('_redrawFromAutoMarginCount' in fullLayout) { fullLayout._redrawFromAutoMarginCount++; } else { @@ -1905,6 +1904,20 @@ plots.doAutoMargin = function(gd) { } }; +var marginKeys = ['l', 'r', 't', 'b', 'p', 'w', 'h']; + +plots.didMarginChange = function(margin0, margin1) { + for(var i = 0; i < marginKeys.length; i++) { + var k = marginKeys[i]; + var m0 = margin0[k]; + var m1 = margin1[k]; + if(!isNumeric(m0) || Math.abs(m1 - m0) > 0) { + return true; + } + } + return false; +}; + /** * JSONify the graph data and layout * diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js index 95c74b6c214..1ec7958cf28 100644 --- a/test/jasmine/tests/plots_test.js +++ b/test/jasmine/tests/plots_test.js @@ -1,6 +1,7 @@ var Plotly = require('@lib/index'); var Plots = require('@src/plots/plots'); var Lib = require('@src/lib'); +var Registry = require('@src/registry'); var d3 = require('d3'); var createGraphDiv = require('../assets/create_graph_div'); @@ -936,6 +937,61 @@ describe('Test Plots', function() { .then(done); }); }); + + describe('Test Plots.doAutoMargin', function() { + afterEach(destroyGraphDiv); + + it('should trigger a replot when necessary', function(done) { + var gd = createGraphDiv(); + var r0; + var w0; + + function _assert(msg, exp) { + var fullLayout = gd._fullLayout; + + expect(fullLayout._size.r).toBe(exp.r); + expect(fullLayout._size.w).toBe(exp.w); + + expect(Registry.call).toHaveBeenCalledTimes(exp.plotCallCnt); + Registry.call.calls.reset(); + } + + Plotly.newPlot(gd, [{ + y: [1, 2, 1], + name: 'A trace name long enough to push the right margin' + }], { + showlegend: true + }) + .then(function() { + r0 = gd._fullLayout._size.r; + w0 = gd._fullLayout._size.w; + spyOn(Registry, 'call'); + }) + .then(function() { + return Plots.doAutoMargin(gd); + }) + .then(function() { + _assert('after doAutoMargin() with identical margins', { + r: r0, + w: w0, + plotCallCnt: 0 + }); + }) + .then(function() { + gd._fullLayout._pushmargin.legend.r.size += 2; + return Plots.doAutoMargin(gd); + }) + .then(function() { + _assert('after doAutoMargin() with bigger margins', { + r: r0 + 2, + w: w0 - 2, + plotCallCnt: 1 + }); + }) + .catch(failTest) + .then(done); + }); + }); }); describe('grids', function() { From d5595d7f628a3c6225bbc9879517f0a6fd84bf16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 26 Apr 2019 14:28:20 -0400 Subject: [PATCH 2/2] add 1px tolerence in Plots.didMarginChange ... to avoid potential infinite loops when rounding errors make auto-margins fail to converge. See report (that @etpinard was never able to reproduce) in https://github.com/plotly/plotly.js/issues/3561#issuecomment-485587432 --- src/plots/plots.js | 4 +++- test/jasmine/tests/plots_test.js | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 8a94588b61b..e3081aff948 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1911,7 +1911,9 @@ plots.didMarginChange = function(margin0, margin1) { var k = marginKeys[i]; var m0 = margin0[k]; var m1 = margin1[k]; - if(!isNumeric(m0) || Math.abs(m1 - m0) > 0) { + // use 1px tolerance in case we old/new differ only + // by rounding errors, which can lead to infinite loops + if(!isNumeric(m0) || Math.abs(m1 - m0) > 1) { return true; } } diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js index 1ec7958cf28..2b6e777ae62 100644 --- a/test/jasmine/tests/plots_test.js +++ b/test/jasmine/tests/plots_test.js @@ -988,6 +988,19 @@ describe('Test Plots', function() { plotCallCnt: 1 }); }) + .then(function() { + gd._fullLayout._pushmargin.legend.r.size += 1; + return Plots.doAutoMargin(gd); + }) + .then(function() { + // see https://github.com/plotly/plotly.js/issues/3561#issuecomment-485953778 + // for more info + _assert('after doAutoMargin() with bigger margins under tolerance', { + r: r0 + 3, + w: w0 - 3, + plotCallCnt: 0 + }); + }) .catch(failTest) .then(done); });