From 24a5ac890023544c85fd0b5f6b0e4848062bd79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 2 Sep 2016 16:18:13 -0400 Subject: [PATCH 1/4] mapbox: handle map move on 'moveend' only - no need map mapbox opts to layout.mapbox and fire events at each step during the move, handling this at the end works just fine for our needs. --- src/plots/mapbox/mapbox.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index 04bd1ba01f4..6dbc211071d 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -125,7 +125,7 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { }); // keep track of pan / zoom in user layout and emit relayout event - map.on('move', function() { + map.on('moveend', function() { var view = self.getView(); opts._input.center = opts.center = view.center; From 207bfe45b46dec7baa6d75659b8947ade401eb1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 2 Sep 2016 16:32:30 -0400 Subject: [PATCH 2/4] mapbox: fire plotly_relayout only when 'movend' is triggered by mouse - 'moveend' (and 'movestart', 'move') gets triggered by map.setCenter, map.setZoom, map.setBearing and map.setPitch. - make sure that 'plotly_relayout' is triggered here only when the 'moveend' originates from a mouse target. --- src/plots/mapbox/mapbox.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index 6dbc211071d..d13a04f3e70 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -125,7 +125,7 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { }); // keep track of pan / zoom in user layout and emit relayout event - map.on('moveend', function() { + map.on('moveend', function(eventData) { var view = self.getView(); opts._input.center = opts.center = view.center; @@ -133,9 +133,19 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) { opts._input.bearing = opts.bearing = view.bearing; opts._input.pitch = opts.pitch = view.pitch; - var update = {}; - update[self.id] = Lib.extendFlat({}, view); - gd.emit('plotly_relayout', update); + // 'moveend' gets triggered by map.setCenter, map.setZoom, + // map.setBearing and map.setPitch. + // + // Here, we make sure that 'plotly_relayout' is + // triggered here only when the 'moveend' originates from a + // mouse target (filtering out API calls) to not + // duplicate 'plotly_relayout' events. + + if(eventData.originalEvent) { + var update = {}; + update[self.id] = Lib.extendFlat({}, view); + gd.emit('plotly_relayout', update); + } }); map.on('mousemove', function(evt) { From 3807018d6d74b4568f94abef2c709a8886399852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 2 Sep 2016 16:33:02 -0400 Subject: [PATCH 3/4] test: add mapbox cases making sure event duplicated --- test/jasmine/tests/mapbox_test.js | 51 ++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index 29df105616f..00ac29b80f9 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -338,6 +338,17 @@ describe('mapbox plots', function() { }); it('should be able to restyle', function(done) { + var restyleCnt = 0, + relayoutCnt = 0; + + gd.on('plotly_restyle', function() { + restyleCnt++; + }); + + gd.on('plotly_relayout', function() { + relayoutCnt++; + }); + function assertMarkerColor(expectations) { return new Promise(function(resolve) { setTimeout(function() { @@ -360,6 +371,9 @@ describe('mapbox plots', function() { return Plotly.restyle(gd, 'marker.color', 'green'); }) .then(function() { + expect(restyleCnt).toEqual(1); + expect(relayoutCnt).toEqual(0); + return assertMarkerColor([ [0, 0.5019, 0, 1], [0, 0.5019, 0, 1] @@ -369,6 +383,9 @@ describe('mapbox plots', function() { return Plotly.restyle(gd, 'marker.color', 'red', [1]); }) .then(function() { + expect(restyleCnt).toEqual(2); + expect(relayoutCnt).toEqual(0); + return assertMarkerColor([ [0, 0.5019, 0, 1], [1, 0, 0, 1] @@ -378,6 +395,17 @@ describe('mapbox plots', function() { }); it('should be able to relayout', function(done) { + var restyleCnt = 0, + relayoutCnt = 0; + + gd.on('plotly_restyle', function() { + restyleCnt++; + }); + + gd.on('plotly_relayout', function() { + relayoutCnt++; + }); + function assertLayout(style, center, zoom, dims) { var mapInfo = getMapInfo(gd); @@ -397,22 +425,37 @@ describe('mapbox plots', function() { assertLayout('Mapbox Dark', [-4.710, 19.475], 1.234, [80, 100, 908, 270]); Plotly.relayout(gd, 'mapbox.center', { lon: 0, lat: 0 }).then(function() { + expect(restyleCnt).toEqual(0); + expect(relayoutCnt).toEqual(1); + assertLayout('Mapbox Dark', [0, 0], 1.234, [80, 100, 908, 270]); return Plotly.relayout(gd, 'mapbox.zoom', '6'); }).then(function() { + expect(restyleCnt).toEqual(0); + expect(relayoutCnt).toEqual(2); + assertLayout('Mapbox Dark', [0, 0], 6, [80, 100, 908, 270]); return Plotly.relayout(gd, 'mapbox.style', 'light'); }).then(function() { + expect(restyleCnt).toEqual(0); + expect(relayoutCnt).toEqual(3); + assertLayout('Mapbox Light', [0, 0], 6, [80, 100, 908, 270]); return Plotly.relayout(gd, 'mapbox.domain.x', [0, 0.5]); }).then(function() { + expect(restyleCnt).toEqual(0); + expect(relayoutCnt).toEqual(4); + assertLayout('Mapbox Light', [0, 0], 6, [80, 100, 454, 270]); return Plotly.relayout(gd, 'mapbox.domain.y[0]', 0.5); }).then(function() { + expect(restyleCnt).toEqual(0); + expect(relayoutCnt).toEqual(5); + assertLayout('Mapbox Light', [0, 0], 6, [80, 100, 454, 135]); done(); @@ -646,9 +689,11 @@ describe('mapbox plots', function() { }); it('should respond drag / scroll interactions', function(done) { - var updateData; + var relayoutCnt = 0, + updateData; gd.on('plotly_relayout', function(eventData) { + relayoutCnt++; updateData = eventData; }); @@ -657,6 +702,9 @@ describe('mapbox plots', function() { return _mouseEvent('mousedown', p0, noop); }).then(function() { return _mouseEvent('mousemove', p1, noop); + }).then(function() { + // repeat mousemove to simulate long dragging motion + return _mouseEvent('mousemove', p1, noop); }).then(function() { return _mouseEvent('mouseup', p1, noop); }).then(function() { @@ -689,6 +737,7 @@ describe('mapbox plots', function() { var p1 = [pointPos[0] + 50, pointPos[1] - 20]; _drag(pointPos, p1, function() { + expect(relayoutCnt).toEqual(1); assertLayout([-19.651, 13.751], 1.234, { withUpdateData: true }); }) From 34cf22e3b741fffa252c50363c2a36a6d52ca9ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 7 Sep 2016 17:24:57 -0400 Subject: [PATCH 4/4] fixup: copy mock before mutating it --- test/jasmine/tests/mapbox_test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index 6dd61fc4a18..043af340a14 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -295,9 +295,10 @@ describe('mapbox plots', function() { }).then(function() { expect(countVisibleTraces(gd, modes)).toEqual(2); - mock.data[0].visible = false; + var mockCopy = Lib.extendDeep({}, mock); + mockCopy.data[0].visible = false; - return Plotly.newPlot(gd, mock.data, mock.layout); + return Plotly.newPlot(gd, mockCopy.data, mockCopy.layout); }).then(function() { expect(countVisibleTraces(gd, modes)).toEqual(1);