From 77434bd0bdb623809c85afaa3da432ebe01e993f Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Tue, 15 Nov 2016 17:55:43 -0800 Subject: [PATCH] Remove/re-add layers when source-related properties change (#571) * Remove/re-add layers when source-related properties change Two cases: - Fix #570 - remove/re-add layer when `source`, `source-layer`, or `type` properties are changed. - Remove all dependent layers before removing a source; if the source is being removed and re-added, then the corresponding layers are also added again afterward. (See https://github.com/mapbox/mapbox-gl-js/pull/3621#issuecomment-260786790) * Add explainer comment * Fix lint * Slightly better readability * One more comment --- lib/diff.js | 48 ++++++++++++++++++++++++++++++++++--- test/diff.js | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/lib/diff.js b/lib/diff.js index 16cf900..9715ba1 100644 --- a/lib/diff.js +++ b/lib/diff.js @@ -97,7 +97,7 @@ var operations = { }; -function diffSources(before, after, commands) { +function diffSources(before, after, commands, sourcesRemoved) { before = before || {}; after = after || {}; @@ -108,6 +108,7 @@ function diffSources(before, after, commands) { if (!before.hasOwnProperty(sourceId)) continue; if (!after.hasOwnProperty(sourceId)) { commands.push({ command: operations.removeSource, args: [sourceId] }); + sourcesRemoved[sourceId] = true; } } @@ -120,6 +121,7 @@ function diffSources(before, after, commands) { // no update command, must remove then add commands.push({ command: operations.removeSource, args: [sourceId] }); commands.push({ command: operations.addSource, args: [sourceId, after[sourceId]] }); + sourcesRemoved[sourceId] = true; } } } @@ -216,6 +218,17 @@ function diffLayers(before, after, commands) { // no need to update if previously added (new or moved) if (clean[layerId] || isEqual(beforeLayer, afterLayer)) continue; + // If source, source-layer, or type have changes, then remove the layer + // and add it back 'from scratch'. + if (!isEqual(beforeLayer.source, afterLayer.source) || !isEqual(beforeLayer['source-layer'], afterLayer['source-layer']) || !isEqual(beforeLayer.type, afterLayer.type)) { + commands.push({ command: operations.removeLayer, args: [layerId] }); + // we add the layer back at the same position it was already in, so + // there's no need to update the `tracker` + insertBeforeLayerId = tracker[tracker.lastIndexOf(layerId) + 1]; + commands.push({ command: operations.addLayer, args: [afterLayer, insertBeforeLayerId] }); + continue; + } + // layout, paint, filter, minzoom, maxzoom diffLayerPropertyChanges(beforeLayer.layout, afterLayer.layout, commands, layerId, null, operations.setLayoutProperty); diffLayerPropertyChanges(beforeLayer.paint, afterLayer.paint, commands, layerId, null, operations.setPaintProperty); @@ -273,6 +286,7 @@ function diffStyles(before, after) { var commands = []; try { + // Handle changes to top-level properties if (!isEqual(before.version, after.version)) { return [{ command: operations.setStyle, args: [after] }]; } @@ -300,8 +314,36 @@ function diffStyles(before, after) { if (!isEqual(before.light, after.light)) { commands.push({ command: operations.setLight, args: [after.light] }); } - diffSources(before.sources, after.sources, commands); - diffLayers(before.layers, after.layers, commands); + + // Handle changes to `sources` + // If a source is to be removed, we also--before the removeSource + // command--need to remove all the style layers that depend on it. + var sourcesRemoved = {}; + + // First collect the {add,remove}Source commands + var removeOrAddSourceCommands = []; + diffSources(before.sources, after.sources, removeOrAddSourceCommands, sourcesRemoved); + + // Push a removeLayer command for each style layer that depends on a + // source that's being removed. + // Also, exclude any such layers them from the input to `diffLayers` + // below, so that diffLayers produces the appropriate `addLayers` + // command + var beforeLayers = []; + if (before.layers) { + before.layers.forEach(function (layer) { + if (sourcesRemoved[layer.source]) { + commands.push({ command: operations.removeLayer, args: [layer.id] }); + } else { + beforeLayers.push(layer); + } + }); + } + commands = commands.concat(removeOrAddSourceCommands); + + // Handle changes to `layers` + diffLayers(beforeLayers, after.layers, commands); + } catch (e) { // fall back to setStyle console.warn('Unable to compute style diff:', e); diff --git a/test/diff.js b/test/diff.js index 1950f08..46dcdf5 100644 --- a/test/diff.js +++ b/test/diff.js @@ -225,5 +225,72 @@ t('diff', function (t) { }] } ], 'multiple light properties change'); + t.deepEqual(diffStyles({ + layers: [ { id: 'a', source: 'source-one' } ] + }, { + layers: [ { id: 'a', source: 'source-two' } ] + }), [ + { command: 'removeLayer', args: ['a'] }, + { command: 'addLayer', args: [{ id: 'a', source: 'source-two' }, undefined] } + ], 'updating a layer\'s source removes/re-adds the layer'); + + t.deepEqual(diffStyles({ + layers: [{ id: 'a', type: 'fill' }] + }, { + layers: [{ id: 'a', type: 'line' }] + }), [ + { command: 'removeLayer', args: ['a'] }, + { command: 'addLayer', args: [{ id: 'a', type: 'line' }, undefined] } + ], 'updating a layer\'s type removes/re-adds the layer'); + + t.deepEqual(diffStyles({ + layers: [{ id: 'a', source: 'a', 'source-layer': 'layer-one' }] + }, { + layers: [{ id: 'a', source: 'a', 'source-layer': 'layer-two' }] + }), [ + { command: 'removeLayer', args: ['a'] }, + { command: 'addLayer', args: [{ id: 'a', source: 'a', 'source-layer': 'layer-two' }, undefined] } + ], 'updating a layer\'s source-layer removes/re-adds the layer'); + + t.deepEqual(diffStyles({ + layers: [ + { id: 'b' }, + { id: 'c' }, + { id: 'a', type: 'fill' } + ] + }, { + layers: [ + { id: 'c' }, + { id: 'a', type: 'line' }, + { id: 'b' } + ] + }), [ + { command: 'removeLayer', args: ['b'] }, + { command: 'addLayer', args: [{id: 'b'}, undefined] }, + { command: 'removeLayer', args: ['a'] }, + { command: 'addLayer', args: [{ id: 'a', type: 'line' }, 'b'] } + ], 'pair respects layer reordering'); + + t.deepEqual(diffStyles({ + sources: { foo: { data: 1 }, bar: {} }, + layers: [ + { id: 'a', source: 'bar' }, + { id: 'b', source: 'foo' }, + { id: 'c', source: 'bar' } + ] + }, { + sources: { foo: { data: 2 }, bar: {} }, + layers: [ + { id: 'a', source: 'bar' }, + { id: 'b', source: 'foo' }, + { id: 'c', source: 'bar' } + ] + }), [ + { command: 'removeLayer', args: ['b'] }, + { command: 'removeSource', args: ['foo'] }, + { command: 'addSource', args: ['foo', { data: 2 }] }, + { command: 'addLayer', args: [{id: 'b', source: 'foo'}, 'c'] } + ], 'changing a source removes and re-adds dependent layers'); + t.end(); });