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

fire sourcedata when source metadata and tiles are loaded #4347

Merged
merged 8 commits into from
Mar 8, 2017
Merged

fire sourcedata when source metadata and tiles are loaded #4347

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e58fecf
source sorcery :sparkles:
Mar 1, 2017
1973eb5
fix render tests :art:
Mar 1, 2017
17cda5c
fix unit tests :wrench:
Mar 2, 2017
799779c
refactor to make everything dependent on 'data' event with new proper…
Mar 6, 2017
89ae98b
change language, updates tests
Mar 6, 2017
3ccc7e0
use sourceDataType event property :calendar:
Mar 7, 2017
c19a117
fixy tests :white_check_mark:
Mar 7, 2017
94c35d6
'update' :point_right: 'content', update tests, docs :memo:
Mar 8, 2017
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
6 changes: 2 additions & 4 deletions src/source/geojson_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ class GeoJSONSource extends Evented {
this.fire('error', {error: err});
return;
}
this.fire('data', {dataType: 'source'});
this.fire('source.load');
this.fire('data', {dataType: 'source', metadata: true});
});
}

Expand All @@ -126,13 +125,12 @@ class GeoJSONSource extends Evented {
*/
setData(data) {
this._data = data;

this.fire('dataloading', {dataType: 'source'});
this._updateWorkerData((err) => {
if (err) {
return this.fire('error', { error: err });
}
this.fire('data', {dataType: 'source'});
this.fire('data', {dataType: 'source', update: true});
});

return this;
Expand Down
8 changes: 4 additions & 4 deletions src/source/image_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,9 @@ class ImageSource extends Evented {
}

_finishLoading() {
this.fire('source.load');

if (this.map) {
this.setCoordinates(this.coordinates);
this.fire('data', {dataType: 'source', metadata: true});
}
}

Expand Down Expand Up @@ -115,7 +114,8 @@ class ImageSource extends Evented {
centerCoord.column = Math.round(centerCoord.column);
centerCoord.row = Math.round(centerCoord.row);

this.minzoom = this.maxzoom = centerCoord.zoom;
// TODO this is making tests fail... what is it for?
// this.minzoom = this.maxzoom = centerCoord.zoom;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lbud would love to know what this line is for. for some reason it's behaving in unexpected ways with the new eventing system and making the canvas source tests fail 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

@lbud might be able to say more, but my recollection from a while back is that this had to do with 'tricking' the source cache into always requesting the 'right' tile from the ImageSource.

Copy link
Contributor

@lbud lbud Mar 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think @anandthakker is right — because of #3186 (👺) we have to trick the source cache into thinking there's only one appropriate tile to ever request for image + image-inheriting sources. 👀 …

this.coord = new TileCoord(centerCoord.zoom, centerCoord.column, centerCoord.row);
this._tileCoords = cornerZ0Coords.map((coord) => {
const zoomedCoord = coord.zoomTo(centerCoord.zoom);
Expand All @@ -124,7 +124,7 @@ class ImageSource extends Evented {
Math.round((zoomedCoord.row - centerCoord.row) * EXTENT));
});

this.fire('data', {dataType: 'source'});
this.fire('data', {dataType:'source', update: true});
return this;
}

Expand Down
3 changes: 1 addition & 2 deletions src/source/raster_tile_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ class RasterTileSource extends Evented {
return this.fire('error', err);
}
util.extend(this, tileJSON);
this.fire('data', {dataType: 'source'});
this.fire('source.load');
this.fire('data', {dataType: 'source', metadata: true});
});
}

Expand Down
35 changes: 19 additions & 16 deletions src/source/source_cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,26 @@ class SourceCache extends Evented {
this.id = id;
this.dispatcher = dispatcher;

this.on('source.load', function() {
this._sourceLoaded = true;
});

this.on('error', function() {
this._sourceErrored = true;
});

this.on('data', function(event) {
if (this._sourceLoaded && event.dataType === 'source') {
this.on('data', function(e) {
// this._sourceLoaded signifies that the TileJSON is loaded if applicable
// if the source type does not come with a TileJSON, the flag signifies the
// source data has loaded (i.e geojson has been tiled on the worker and is ready)
if (e.dataType === 'source' && e.metadata) this._sourceLoaded = true;

// for sources with mutable data, this event fires when the underlying data
// to a source is changed. (i.e. GeoJSONSource#setData and ImageSource#serCoordinates)
if (this._sourceLoaded && e.dataType==="source" && e.update) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great ❤️

Notes/questions:

  1. We should update the internal documentation here to reflect these changes to the Source interface contract.

  2. Is e.metadata too generic a name? I'm not even sure if I like this idea yet, but what about something like e.sourceDataType == 'metadata' and e.sourceDataType == 'update'? (Basically having sourceDataType be analogous to dataType, but specific to source data events.)

  3. Is there still a possibility of a race condition here for vector sources? We need SourceCache#update() to be called after the TileJSON has loaded. This currently happens either here (for events with e.update) or via Style#_updateSources(). I'm imagining this problematic timeline:

  • map.addSource('mysource', ...) (plus map.addLayer() using mysource) => requests TileJSON, marks map._sourceDirty.
  • Next render frame fires; map._sourcesDirty being true leads to style._updateSources()... but since the TileJSON is still in flight, we don't get a SourceCache#update() for mysource.
  • TileJSON lands => VectorTileSource fires data with e.metadata => mysource's SourceCache marks itself _sourceLoaded.
  • But now, the actual tiles for mysource are not loaded until something else causes _sourcesDirty flag to be set.

this.reload();
if (this.transform) {
this.update(this.transform);
}
}
});

this.on('error', function() {
this._sourceErrored = true;
});

this._source = Source.create(id, options, dispatcher, this);

this._tiles = {};
Expand Down Expand Up @@ -169,7 +172,7 @@ class SourceCache extends Evented {
tile.timeAdded = new Date().getTime();
if (previousState === 'expired') tile.refreshedUponExpiration = true;
this._setTileReloadTimer(id, tile);
this._source.fire('data', {tile: tile, coord: tile.coord, dataType: 'tile'});
this._source.fire('data', {dataType: 'source', tile: tile, coord: tile.coord});

// HACK this is necessary to fix https://github.com/mapbox/mapbox-gl-js/issues/2986
if (this.map) this.map.painter.tileExtentVAO.vao = null;
Expand Down Expand Up @@ -296,6 +299,7 @@ class SourceCache extends Evented {
* @private
*/
update(transform) {
this.transform = transform;
if (!this._sourceLoaded) { return; }
let i;
let coord;
Expand Down Expand Up @@ -394,8 +398,6 @@ class SourceCache extends Evented {
for (i = 0; i < remove.length; i++) {
this.removeTile(+remove[i]);
}

this.transform = transform;
}

/**
Expand Down Expand Up @@ -423,8 +425,8 @@ class SourceCache extends Evented {
}
}
}

if (!tile) {
const cached = Boolean(tile);
if (!cached) {
const zoom = coord.z;
const overscaling = zoom > this._source.maxzoom ? Math.pow(2, zoom - this._source.maxzoom) : 1;
tile = new Tile(wrapped, this._source.tileSize * overscaling, this._source.maxzoom);
Expand All @@ -433,7 +435,7 @@ class SourceCache extends Evented {

tile.uses++;
this._tiles[coord.id] = tile;
this._source.fire('dataloading', {tile: tile, coord: tile.coord, dataType: 'tile'});
if (!cached) this._source.fire('dataloading', {tile: tile, coord: tile.coord, dataType: 'source'});

return tile;
}
Expand Down Expand Up @@ -475,6 +477,7 @@ class SourceCache extends Evented {
clearTimeout(this._timers[id]);
this._timers[id] = undefined;
}
// TODO should we keep this??
this._source.fire('data', { tile: tile, coord: tile.coord, dataType: 'tile' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj @anandthakker do you think we need to fire an event when a tile is removed now that tiledata is ⚰️? I feel like this isn't useful, but let me know if you disagree.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this event isn't useful -- if we're not using it anywhere internally, then I'm all for removing it.

What might be useful someday in the future would be a way for users to listen for changes to the list of currently renderable/rendered tile coordinates -- but this doesn't really provide that, and anyway that's a super non-urgent nice-to-have.


if (tile.uses > 0)
Expand Down
3 changes: 1 addition & 2 deletions src/source/vector_tile_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ class VectorTileSource extends Evented {
return;
}
util.extend(this, tileJSON);
this.fire('data', {dataType: 'source'});
this.fire('source.load');
this.fire('data', {dataType: 'source', metadata: true});
});
}

Expand Down
16 changes: 9 additions & 7 deletions src/style/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,15 @@ class Style extends Evented {
browser.frame(stylesheetLoaded.bind(this, null, stylesheet));
}

this.on('source.load', (event) => {
const source = this.sourceCaches[event.sourceId].getSource();
if (source && source.vectorLayerIds) {
for (const layerId in this._layers) {
const layer = this._layers[layerId];
if (layer.source === source.id) {
this._validateLayer(layer);
this.on('data', (event) => {
if (event.dataType === 'source' && event.metadata) {
const source = this.sourceCaches[event.sourceId].getSource();
if (source && source.vectorLayerIds) {
for (const layerId in this._layers) {
const layer = this._layers[layerId];
if (layer.source === source.id) {
this._validateLayer(layer);
}
}
}
}
Expand Down
16 changes: 7 additions & 9 deletions src/ui/control/attribution_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class AttributionControl {
this._updateAttributions();
this._updateEditLink();

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

if (compact === undefined) {
Expand All @@ -58,7 +58,7 @@ class AttributionControl {
onRemove() {
this._container.parentNode.removeChild(this._container);

this._map.off('data', this._updateData);
this._map.off('sourcedata', this._updateData);
this._map.off('moveend', this._updateEditLink);
this._map.off('resize', this._updateCompact);

Expand All @@ -74,16 +74,14 @@ class AttributionControl {
}
}

_updateData(event) {
if (event.dataType === 'source') {
this._updateAttributions();
this._updateEditLink();
}
_updateData(e) {
this._updateAttributions(e);
this._updateEditLink();
}

_updateAttributions() {
_updateAttributions(e) {
if (!this._map.style) return;

if (e && !e.metadata) return;
let attributions = [];

const sourceCaches = this._map.style.sourceCaches;
Expand Down
8 changes: 4 additions & 4 deletions src/ui/control/logo_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,22 @@ class LogoControl {
this._map = map;
this._container = DOM.create('div', 'mapboxgl-ctrl');

this._map.on('data', this._updateLogo);
this._map.on('sourcedata', this._updateLogo);
this._updateLogo();
return this._container;
}

onRemove() {
this._container.parentNode.removeChild(this._container);
this._map.off('data', this._updateLogo);
this._map.off('sourcedata', this._updateLogo);
}

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

_updateLogo() {
if (this._logoRequired()) {
_updateLogo(e) {
if (e && e.metadata && this._logoRequired()) {
const anchor = DOM.create('a', 'mapboxgl-ctrl-logo');
anchor.target = "_blank";
anchor.href = "https://www.mapbox.com/";
Expand Down
25 changes: 14 additions & 11 deletions test/unit/source/canvas_source.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ function createSource(options) {
}, options);

const source = new CanvasSource('id', options, { send: function() {} }, options.eventedParent);

source.canvas = c;

return source;
Expand All @@ -47,13 +46,15 @@ test('CanvasSource', (t) => {
t.test('constructor', (t) => {
const source = createSource();

source.on('source.load', () => {
t.equal(source.minzoom, 0);
t.equal(source.maxzoom, 22);
t.equal(source.tileSize, 512);
t.equal(source.animate, true);
t.equal(typeof source.play, 'function');
t.end();
source.on('data', (e) => {
if (e.metadata && e.dataType === 'source') {
t.equal(source.minzoom, 0);
t.equal(source.maxzoom, 22);
t.equal(source.tileSize, 512);
t.equal(source.animate, true);
t.equal(typeof source.play, 'function');
t.end();
}
});

source.onAdd(new StubMap());
Expand Down Expand Up @@ -82,9 +83,11 @@ test('CanvasSource', (t) => {
t.end();
});

source.on('source.load', () => {
t.ok(true, 'fires load event without rerendering');
t.end();
source.on('data', (e) => {
if (e.metadata && e.dataType === 'source') {
t.ok(true, 'fires load event without rerendering');
t.end();
}
});

source.onAdd(map);
Expand Down
14 changes: 8 additions & 6 deletions test/unit/source/geojson_source.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ test('GeoJSONSource#update', (t) => {
}, mockDispatcher).load();
});

t.test('fires "source.load"', (t) => {
t.test('fires event when metadata loads', (t) => {
const mockDispatcher = {
send: function(message, args, callback) {
setTimeout(callback, 0);
Expand All @@ -151,8 +151,8 @@ test('GeoJSONSource#update', (t) => {

const source = new GeoJSONSource('id', {data: {}}, mockDispatcher);

source.on('source.load', () => {
t.end();
source.on('data', (e) => {
if (e.metadata) t.end();
});

source.load();
Expand Down Expand Up @@ -191,9 +191,11 @@ test('GeoJSONSource#update', (t) => {
transform: {}
};

source.on('source.load', () => {
source.setData({});
source.loadTile(new Tile(new TileCoord(0, 0, 0), 512), () => {});
source.on('data', (e) => {
if (e.metadata) {
source.setData({});
source.loadTile(new Tile(new TileCoord(0, 0, 0), 512), () => {});
}
});

source.load();
Expand Down
Loading