Skip to content
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

Don't fire sourcedata event with sourceDataType: 'metadata' when a source is removed #5771

Closed
wants to merge 5 commits into from
Closed

Don't fire sourcedata event with sourceDataType: 'metadata' when a source is removed #5771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
31812da
Don't fire sourcedata event with sourceDataType: 'metadata' when a so…
jfirebaugh Nov 29, 2017
36fdac9
make relevant changes to attribution control, remove sourcedata updat…
Jan 11, 2018
a3a47b1
fix logo test
Jan 11, 2018
d8a166f
put sourcedata listener back in
Jan 11, 2018
e22c807
fix flow
Jan 11, 2018
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
1 change: 0 additions & 1 deletion src/style/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,6 @@ class Style extends Evented {
const sourceCache = this.sourceCaches[id];
delete this.sourceCaches[id];
delete this._updatedSources[id];
sourceCache.fire('data', {sourceDataType: 'metadata', dataType:'source', sourceId: id});
sourceCache.setEventedParent(null);
sourceCache.clearTiles();

Expand Down
8 changes: 5 additions & 3 deletions src/ui/control/attribution_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class AttributionControl {
this._updateAttributions();
this._updateEditLink();

this._map.on('styledata', this._updateData);
this._map.on('sourcedata', this._updateData);
this._map.on('moveend', this._updateEditLink);

Expand All @@ -67,6 +68,7 @@ class AttributionControl {
onRemove() {
DOM.remove(this._container);

this._map.off('styledata', this._updateData);
this._map.off('sourcedata', this._updateData);
this._map.off('moveend', this._updateEditLink);
this._map.off('resize', this._updateCompact);
Expand Down Expand Up @@ -97,15 +99,15 @@ class AttributionControl {
}
}

_updateData(e: any) {
if (e && e.sourceDataType === 'metadata') {
_updateData(e: ?Object) {
if (e && (e.type === "styledata" || e.sourceDataType === "metadata")) {
this._updateAttributions();
this._updateEditLink();
}
}

_updateAttributions() {
if (!this._map.style) return;
if (!this._map || !this._map.style) return;
let attributions: Array<string> = [];

if (this._map.style.stylesheet) {
Expand Down
10 changes: 4 additions & 6 deletions src/ui/control/logo_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,24 @@ class LogoControl {
this._container.appendChild(anchor);
this._container.style.display = 'none';

this._map.on('styledata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll also need to remove this event listener in the onRemove function

this._map.on('sourcedata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are safe to remove this listener all together. Keeping it here and removing the metadata condition causes _updateLogo to be called on every tile load 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might still be necessary to ensure that the control updates when TileJSON is loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, thank you! it was working on the debug page because the spritesheet was loading after the tilejson.

this._updateLogo();
return this._container;
}

onRemove() {
DOM.remove(this._container);
this._map.off('styledata', this._updateLogo);
this._map.off('sourcedata', this._updateLogo);
}

getDefaultPosition() {
return 'bottom-left';
}

_updateLogo(e: any) {
if (!e || e.sourceDataType === 'metadata') {
this._container.style.display = this._logoRequired() ? 'block' : 'none';
}
_updateLogo(e: ?Object) {
if (e && (e.type === 'styledata' || e.sourceDataType === "metadata")) this._container.style.display = this._logoRequired() ? 'block' : 'none';
}

_logoRequired() {
Expand All @@ -65,8 +65,6 @@ class LogoControl {

return false;
}

}


module.exports = LogoControl;
41 changes: 25 additions & 16 deletions test/unit/ui/control/attribution.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,34 @@ test('AttributionControl dedupes attributions that are substrings of others', (t
map.addControl(attribution);

map.on('load', () => {
map.on('styledata', () => {
t.equal(attribution._container.innerHTML, 'Hello World | Another Source');
t.end();
});
map.addSource('1', { type: 'vector', attribution: 'World' });
map.addSource('2', { type: 'vector', attribution: 'Hello World' });
map.addSource('3', { type: 'vector', attribution: 'Another Source' });
map.addSource('4', { type: 'vector', attribution: 'Hello' });
map.addSource('5', { type: 'vector', attribution: 'Hello World' });

});
});

test('AttributionControl is removed when source is removed', (t) => {
const map = createMap();
const attribution = new AttributionControl();
map.addControl(attribution);

map.on('load', () => {
map.on('styledata', () => {
t.equal(attribution._container.innerHTML, 'Hello World');
t.end();
});
map.addSource('1', { type: 'vector', attribution: 'Hello World' });
map.addSource('2', { type: 'vector', attribution: 'Another Source' });
map.removeSource('2');

let times = 0;
map.on('data', (e) => {
if (e.dataType === 'source' && e.sourceDataType === 'metadata') {
if (++times === 5) {
t.equal(attribution._container.innerHTML, 'Hello World | Another Source');
t.end();
}
}
});

});

test('AttributionControl has the correct edit map link', (t) => {
Expand All @@ -111,13 +122,11 @@ test('AttributionControl has the correct edit map link', (t) => {
map.addControl(attribution);
map.on('load', () => {
map.addSource('1', {type: 'vector', attribution: '<a class="mapbox-improve-map" href="https://www.mapbox.com/feedback/" target="_blank">Improve this map</a>'});
map.on('data', (e) => {
if (e.dataType === 'source' && e.sourceDataType === 'metadata') {
t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/0', 'edit link contains map location data');
map.setZoom(2);
t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/2', 'edit link updates on mapmove');
t.end();
}
map.on('styledata', () => {
t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/0', 'edit link contains map location data');
map.setZoom(2);
t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/2', 'edit link updates on mapmove');
t.end();
});
});
});
141 changes: 81 additions & 60 deletions test/unit/ui/control/logo.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,88 +1,109 @@
'use strict';

const test = require('mapbox-gl-js-test').test;
const VectorTileSource = require('../../../../src/source/vector_tile_source');
const window = require('../../../../src/util/window');
const Map = require('../../../../src/ui/map');

function createMap(logoPosition, logoRequired) {
test('LogoControl appears in bottom-left by default', (t) => {
const container = window.document.createElement('div');
const map = new Map({container});
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
});

test('LogoControl appears in the position specified by the position option', (t) => {
const container = window.document.createElement('div');
return new Map({
container: container,
const map = new Map({container, logoPosition: 'top-left'});
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-top-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
});

test('LogoControl is hidden when no sources with the mapbox_logo property exist', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
'composite': createSource({
minzoom: 1,
maxzoom: 10,
attribution: "Mapbox",
tiles: [
"http://example.com/{z}/{x}/{y}.png"
]
}, logoRequired)
},
sources: {},
layers: []
},
logoPosition: logoPosition || undefined
});
}

function createSource(options, logoRequired) {
const source = new VectorTileSource('id', options, { send: function () {} });
source.onAdd({
transform: { angle: 0, pitch: 0, showCollisionBoxes: false }
});
source.on('error', (e) => {
throw e.error;
});
const logoFlag = "mapbox_logo";
source[logoFlag] = logoRequired === undefined ? true : logoRequired;
return source;
}
test('LogoControl appears in bottom-left by default', (t) => {
const map = createMap();
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-logo'
).length, 1);
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'none');
t.end();
});
});

test('LogoControl appears in the position specified by the position option', (t) => {
const map = createMap('top-left');
test('LogoControl is shown when a source with the mapbox_logo property is added', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {},
layers: []
},
});
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-top-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
map.addSource('source', {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
});
map.once('styledata', () => {
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'block');
t.end();
});
});
});

test('LogoControl is not displayed when the mapbox_logo property is false', (t) => {
const map = createMap('top-left', false);
test('LogoControl is visible when a source with the mapbox_logo property exists', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
source: {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
}
},
layers: []
},
});
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-top-left > .mapboxgl-ctrl')[0].style.display, 'none');
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'block');
t.end();
});
});
test('LogoControl is not added more than once', (t)=>{
const map = createMap();
const source = createSource({
minzoom: 1,
maxzoom: 10,
attribution: "Mapbox",
tiles: [
"http://example.com/{z}/{x}/{y}.png"
]

test('LogoControl is hidden when the last source with the mapbox_logo property is removed', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
source: {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
}
},
layers: []
},
});
map.on('load', ()=>{
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-logo').length, 1, 'first LogoControl');
map.addSource('source2', source);
map.on('sourcedata', (e)=>{
if (e.isSourceLoaded && e.sourceId === 'source2' && e.sourceDataType === 'metadata') {
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-logo').length, 1, 'only one LogoControl is added with multiple sources');
t.end();
}
map.on('load', () => {
map.removeSource('source');
map.once('styledata', () => {
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'none');
t.end();
});
});
});