Skip to content

Fix scattermapbox update bug #6387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
archmoj merged 15 commits into from
Dec 7, 2022
Merged

Fix scattermapbox update bug #6387

Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
88678db
bring back restyle in scattermapbox plot update
archmoj Dec 1, 2022
b60d347
correct add & remove orders depending on old & new cluster options
archmoj Dec 2, 2022
0a41ffa
refactor - move sets to the end
archmoj Dec 2, 2022
14cbf6a
revise adding and removing cluster and non-cluster layers
archmoj Dec 2, 2022
282caad
revise draw and restyle logic
archmoj Dec 2, 2022
6987bc6
improve functions
archmoj Dec 2, 2022
845d990
avoid mapbox-gl errors
archmoj Dec 2, 2022
eb630b5
avoid jumps in zoom interactions
archmoj Dec 2, 2022
257b32c
fix visiblity tweaks
archmoj Dec 2, 2022
697db13
refactor functions
archmoj Dec 2, 2022
70213e1
reconsider below during updates
archmoj Dec 2, 2022
d4d0d6c
do not pass trace.cluster in case of nonCluster
archmoj Dec 2, 2022
7b59109
avoid jumps on zoom
archmoj Dec 6, 2022
0c6079c
fix hide & unhide
archmoj Dec 6, 2022
ebbeb8a
revise order of ifs to better handle hidden traces
archmoj Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 87 additions & 38 deletions src/traces/scattermapbox/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ var ORDER = {
nonCluster: ['fill', 'line', 'circle', 'symbol'],
};

function ScatterMapbox(subplot, uid, clusterEnabled) {
function ScatterMapbox(subplot, uid, clusterEnabled, isHidden) {
this.type = 'scattermapbox';
this.subplot = subplot;
this.uid = uid;
this.clusterEnabled = clusterEnabled;
this.isHidden = isHidden;

this.sourceIds = {
fill: 'source-' + uid + '-fill',
Expand Down Expand Up @@ -85,51 +86,96 @@ proto.update = function update(calcTrace) {
var map = subplot.map;
var optsAll = convert(subplot.gd, calcTrace);
var below = subplot.belowLookup['trace-' + this.uid];
var i, k, opts;
var hasCluster = !!(trace.cluster && trace.cluster.enabled);
var hadCluster = !!this.clusterEnabled;

if(below !== this.below) {
var order = ORDER.nonCluster;

for(i = order.length - 1; i >= 0; i--) {
k = order[i];
map.removeLayer(this.layerIds[k]);
var lThis = this;

function addCluster(noSource) {
if(!noSource) lThis.addSource('circle', optsAll.circle, trace.cluster);
var order = ORDER.cluster;
for(var i = 0; i < order.length; i++) {
var k = order[i];
var opts = optsAll[k];
lThis.addLayer(k, opts, below);
}
for(i = 0; i < order.length; i++) {
k = order[i];
opts = optsAll[k];
this.addLayer(k, opts, below);
}
this.below = below;
} else if(hasCluster && !hadCluster) {
for(i = ORDER.nonCluster.length - 1; i >= 0; i--) {
k = ORDER.nonCluster[i];
map.removeLayer(this.layerIds[k]);
map.removeSource(this.sourceIds[k]);
}

function removeCluster(noSource) {
var order = ORDER.cluster;
for(var i = order.length - 1; i >= 0; i--) {
var k = order[i];
map.removeLayer(lThis.layerIds[k]);
}
this.addSource('circle', optsAll.circle, trace.cluster);
for(i = 0; i < ORDER.cluster.length; i++) {
k = ORDER.cluster[i];
opts = optsAll[k];
this.addLayer(k, opts, below);
if(!noSource) map.removeSource(lThis.sourceIds.circle);
}

function addNonCluster(noSource) {
var order = ORDER.nonCluster;
for(var i = 0; i < order.length; i++) {
var k = order[i];
var opts = optsAll[k];
if(!noSource) lThis.addSource(k, opts);
lThis.addLayer(k, opts, below);
}
this.clusterEnabled = hasCluster;
} else if(!hasCluster && hadCluster) {
for(i = 0; i < ORDER.cluster.length; i++) {
k = ORDER.cluster[i];
map.removeLayer(this.layerIds[k]);
}

function removeNonCluster(noSource) {
var order = ORDER.nonCluster;
for(var i = order.length - 1; i >= 0; i--) {
var k = order[i];
map.removeLayer(lThis.layerIds[k]);
if(!noSource) map.removeSource(lThis.sourceIds[k]);
}
map.removeSource(this.sourceIds.circle);
for(i = 0; i < ORDER.nonCluster.length; i++) {
k = ORDER.nonCluster[i];
opts = optsAll[k];
this.addSource(k, opts, trace.cluster);
this.addLayer(k, opts, below);
}

function remove(noSource) {
if(hadCluster) removeCluster(noSource); else removeNonCluster(noSource);
Copy link
Collaborator

@alexcjohnson alexcjohnson Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a big deal, but if you really want this to be a one-liner you can collapse it more; otherwise I'd write it as multiple lines. (same in add below)

Suggested change
if(hadCluster) removeCluster(noSource); else removeNonCluster(noSource);
(hadCluster ? removeCluster : removeNonCluster)(noSource);

}

function add(noSource) {
if(hasCluster) addCluster(noSource); else addNonCluster(noSource);
}

function repaint() {
var order = hasCluster ? ORDER.cluster : ORDER.nonCluster;
for(var i = 0; i < order.length; i++) {
var k = order[i];
var opts = optsAll[k];
if(!opts) continue;

subplot.setOptions(lThis.layerIds[k], 'setLayoutProperty', opts.layout);

if(opts.layout.visibility === 'visible') {
if(k !== 'cluster') {
lThis.setSourceData(k, opts);
}
subplot.setOptions(lThis.layerIds[k], 'setPaintProperty', opts.paint);
}
}
this.clusterEnabled = hasCluster;
}

var wasHidden = this.isHidden;
var isHidden = trace.visible !== true;

if(isHidden) {
if(!wasHidden) remove();
} else if(wasHidden) {
if(!isHidden) add();
} else if(hadCluster !== hasCluster) {
remove();
add();
} else if(this.below !== below) {
remove(true);
add(true);
repaint();
} else {
repaint();
}

this.clusterEnabled = hasCluster;
this.isHidden = isHidden;
this.below = below;

// link ref for quick update during selections
calcTrace[0].trace._glTrace = this;
};
Expand All @@ -147,10 +193,13 @@ proto.dispose = function dispose() {
module.exports = function createScatterMapbox(subplot, calcTrace) {
var trace = calcTrace[0].trace;
var hasCluster = trace.cluster && trace.cluster.enabled;
var isHidden = trace.visible !== true;

var scatterMapbox = new ScatterMapbox(
subplot,
trace.uid,
hasCluster
hasCluster,
isHidden
);

var optsAll = convert(subplot.gd, calcTrace);
Expand Down