From 0d1db3faa9c735d41ceabc3bf7e6d0c0c6b1612a Mon Sep 17 00:00:00 2001 From: Jon Mease Date: Thu, 12 Nov 2020 12:13:57 -0500 Subject: [PATCH 1/4] Refactor to extract lookupBelow and findFollowingMapboxLayerId functions --- src/plots/mapbox/layers.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/plots/mapbox/layers.js b/src/plots/mapbox/layers.js index dd05b225667..a0261dc9e4e 100644 --- a/src/plots/mapbox/layers.js +++ b/src/plots/mapbox/layers.js @@ -85,6 +85,10 @@ proto.needsNewLayer = function(opts) { ); }; +proto.lookupBelow = function() { + return this.subplot.belowLookup['layout-' + this.index]; +}; + proto.updateImage = function(opts) { var map = this.subplot.map; map.getSource(this.idSource).updateImage({ @@ -107,15 +111,9 @@ proto.updateSource = function(opts) { map.addSource(this.idSource, sourceOpts); }; -proto.updateLayer = function(opts) { - var subplot = this.subplot; - var convertedOpts = convertOpts(opts); - - var below = this.subplot.belowLookup['layout-' + this.index]; - var _below; - +proto.findFollowingMapboxLayerId = function(below) { if(below === 'traces') { - var mapLayers = subplot.getMapLayers(); + var mapLayers = this.subplot.getMapLayers(); // find id of first plotly trace layer for(var i = 0; i < mapLayers.length; i++) { @@ -123,13 +121,19 @@ proto.updateLayer = function(opts) { if(typeof layerId === 'string' && layerId.indexOf(constants.traceLayerPrefix) === 0 ) { - _below = layerId; + below = layerId; break; } } - } else { - _below = below; } + return below; +}; + +proto.updateLayer = function(opts) { + var subplot = this.subplot; + var convertedOpts = convertOpts(opts); + var below = this.lookupBelow(); + var _below = this.findFollowingMapboxLayerId(below); this.removeLayer(); From 82fbdcdf31e119d71f8d54a957ea4490baf4bc8e Mon Sep 17 00:00:00 2001 From: Jon Mease Date: Thu, 12 Nov 2020 12:14:59 -0500 Subject: [PATCH 2/4] Reorder layer in updateImage to match what updateLayer would do --- src/plots/mapbox/layers.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/plots/mapbox/layers.js b/src/plots/mapbox/layers.js index a0261dc9e4e..45111e167d2 100644 --- a/src/plots/mapbox/layers.js +++ b/src/plots/mapbox/layers.js @@ -94,6 +94,14 @@ proto.updateImage = function(opts) { map.getSource(this.idSource).updateImage({ url: opts.source, coordinates: opts.coordinates }); + + // Since the `updateImage` control flow doesn't call updateLayer, + // We need to take care of moving the image layer to match the location + // where updateLayer would have placed it. + var _below = this.findFollowingMapboxLayerId(this.lookupBelow()); + if(_below !== null) { + this.subplot.map.moveLayer(this.idLayer, _below); + } }; proto.updateSource = function(opts) { From e240ce65c03bc2c00dda6488c701403d9c9e11c8 Mon Sep 17 00:00:00 2001 From: Jon Mease Date: Thu, 12 Nov 2020 12:15:57 -0500 Subject: [PATCH 3/4] Handle layer.source as arrays in equality check --- src/plots/mapbox/layers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/mapbox/layers.js b/src/plots/mapbox/layers.js index 45111e167d2..99669e1077a 100644 --- a/src/plots/mapbox/layers.js +++ b/src/plots/mapbox/layers.js @@ -73,7 +73,7 @@ proto.needsNewSource = function(opts) { // stay safe and make new source on type changes return ( this.sourceType !== opts.sourcetype || - this.source !== opts.source || + JSON.stringify(this.source) !== JSON.stringify(opts.source) || this.layerType !== opts.type ); }; From 405473543e58c62314129410b01499cc9e4d797c Mon Sep 17 00:00:00 2001 From: Jon Mease Date: Thu, 12 Nov 2020 17:35:46 -0500 Subject: [PATCH 4/4] Extend the updateImage mapbox test to check layer order --- test/jasmine/tests/mapbox_test.js | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index 538dd39f701..8bb86a575f6 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -948,10 +948,15 @@ describe('@noCI, mapbox plots', function() { layout: { mapbox: { layers: [{ + 'sourcetype': 'raster', + 'source': ['https://a.tile.openstreetmap.org/{z}/{x}/{y}.png'], + 'below': 'traces', + }, { 'sourcetype': 'image', 'coordinates': coords, - 'source': source - }] + 'source': source, + 'below': 'traces', + }], } } }; @@ -970,7 +975,7 @@ describe('@noCI, mapbox plots', function() { Plotly.react(gd, makeFigure(redImage)).then(function() { var mapbox = gd._fullLayout.mapbox._subplot; map = mapbox.map; - layerSource = map.getSource(mapbox.layerList[0].idSource); + layerSource = map.getSource(mapbox.layerList[1].idSource); spyOn(layerSource, 'updateImage').and.callThrough(); spyOn(map, 'removeSource').and.callThrough(); @@ -981,6 +986,23 @@ describe('@noCI, mapbox plots', function() { {url: greenImage, coordinates: coords} ); expect(map.removeSource).not.toHaveBeenCalled(); + + // Check order of layers + var mapbox = gd._fullLayout.mapbox._subplot; + var mapboxLayers = mapbox.getMapLayers(); + var plotlyjsLayers = mapbox.layerList; + + var indexLower = mapboxLayers.findIndex(function(layer) { + return layer.id === 'plotly-layout-layer-' + plotlyjsLayers[0].uid; + }); + + var indexUpper = mapboxLayers.findIndex(function(layer) { + return layer.id === 'plotly-layout-layer-' + plotlyjsLayers[1].uid; + }); + + expect(indexLower).toBeGreaterThan(-1); + expect(indexUpper).toBeGreaterThan(0); + expect(indexUpper).toBe(indexLower + 1); }) .catch(failTest) .then(done);