From b6f1145280d8d1b09cfe44b334fa84562c88cfff Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Mon, 9 Dec 2019 17:38:10 -0800 Subject: [PATCH 1/6] Move responsibility of triggering rtl text plugin load from VectorTileSource to TIle --- src/data/bucket/symbol_bucket.js | 4 +++- src/source/rtl_text_plugin.js | 9 +++++++++ src/source/tile.js | 2 ++ src/source/vector_tile_source.js | 10 ---------- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/data/bucket/symbol_bucket.js b/src/data/bucket/symbol_bucket.js index 728d6ee1056..a034b7ce431 100644 --- a/src/data/bucket/symbol_bucket.js +++ b/src/data/bucket/symbol_bucket.js @@ -521,7 +521,9 @@ class SymbolBucket implements Bucket { } isEmpty() { - return this.symbolInstances.length === 0; + // When the bucket encounters only rtl-text but the plugin isnt loaded, no symbol instances will be created. + // In order for the bucket to be serialized, and not discarded as an empty bucket both checks are necessary. + return this.symbolInstances.length === 0 && !this.hasRTLText; } uploadPending() { diff --git a/src/source/rtl_text_plugin.js b/src/source/rtl_text_plugin.js index 8681a98536f..e346222f6b5 100644 --- a/src/source/rtl_text_plugin.js +++ b/src/source/rtl_text_plugin.js @@ -141,3 +141,12 @@ export const plugin: { }; } }; + +export const lazyLoadRTLTextPlugin = function(){ + if (!plugin.isLoading() && + !plugin.isLoaded() && + getRTLTextPluginStatus() === 'deferred' + ) { + downloadRTLTextPlugin(); + } +} diff --git a/src/source/tile.js b/src/source/tile.js index 9602db15b43..d1d502cbf11 100644 --- a/src/source/tile.js +++ b/src/source/tile.js @@ -11,6 +11,7 @@ import Texture from '../render/texture'; import browser from '../util/browser'; import EvaluationParameters from '../style/evaluation_parameters'; import SourceFeatureState from '../source/source_state'; +import {lazyLoadRTLTextPlugin} from './rtl_text_plugin'; const CLOCK_SKEW_RETRY_TIMEOUT = 30000; @@ -183,6 +184,7 @@ class Tile { if (bucket instanceof SymbolBucket) { if (bucket.hasRTLText) { this.hasRTLText = true; + lazyLoadRTLTextPlugin(); break; } } diff --git a/src/source/vector_tile_source.js b/src/source/vector_tile_source.js index 5ec313a6264..1554b45bd1d 100644 --- a/src/source/vector_tile_source.js +++ b/src/source/vector_tile_source.js @@ -9,7 +9,6 @@ import TileBounds from './tile_bounds'; import {ResourceType} from '../util/ajax'; import browser from '../util/browser'; import {cacheEntryPossiblyAdded} from '../util/tile_request_cache'; -import {plugin as rtlTextPlugin, getRTLTextPluginStatus, downloadRTLTextPlugin} from './rtl_text_plugin'; import type {Source} from './source'; import type {OverscaledTileID} from './tile_id'; @@ -156,15 +155,6 @@ class VectorTileSource extends Evented implements Source { if (this.map._refreshExpiredTiles && data) tile.setExpiryData(data); tile.loadVectorData(data, this.map.painter); - if (tile.hasRTLText) { - const plugin = rtlTextPlugin; - if (!plugin.isLoading() && - !plugin.isLoaded() && - getRTLTextPluginStatus() === 'deferred' - ) { - downloadRTLTextPlugin(); - } - } cacheEntryPossiblyAdded(this.dispatcher); From ab225881a2045c0aed33d6b9ef943970f2cd3d49 Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Mon, 9 Dec 2019 17:40:07 -0800 Subject: [PATCH 2/6] Fix lint errors --- src/source/rtl_text_plugin.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/source/rtl_text_plugin.js b/src/source/rtl_text_plugin.js index e346222f6b5..fbd1b7fd3fa 100644 --- a/src/source/rtl_text_plugin.js +++ b/src/source/rtl_text_plugin.js @@ -142,11 +142,11 @@ export const plugin: { } }; -export const lazyLoadRTLTextPlugin = function(){ +export const lazyLoadRTLTextPlugin = function() { if (!plugin.isLoading() && !plugin.isLoaded() && getRTLTextPluginStatus() === 'deferred' ) { downloadRTLTextPlugin(); } -} +}; From bddc36b69b61dd7fb4cce292c7cc0df5e3628be0 Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Tue, 10 Dec 2019 17:30:17 -0800 Subject: [PATCH 3/6] Add tests for rtl text loading --- test/unit/data/symbol_bucket.test.js | 42 +++++++++++++++++----------- test/unit/source/tile.test.js | 27 ++++++++++++++++-- test/util/create_symbol_layer.js | 20 +++++++++++++ 3 files changed, 70 insertions(+), 19 deletions(-) create mode 100644 test/util/create_symbol_layer.js diff --git a/test/unit/data/symbol_bucket.test.js b/test/unit/data/symbol_bucket.test.js index 5a1355c969b..dc49b130f32 100644 --- a/test/unit/data/symbol_bucket.test.js +++ b/test/unit/data/symbol_bucket.test.js @@ -5,8 +5,6 @@ import Protobuf from 'pbf'; import {VectorTile} from '@mapbox/vector-tile'; import SymbolBucket from '../../../src/data/bucket/symbol_bucket'; import {CollisionBoxArray} from '../../../src/data/array_types'; -import SymbolStyleLayer from '../../../src/style/style_layer/symbol_style_layer'; -import featureFilter from '../../../src/style-spec/feature_filter'; import {performSymbolLayout} from '../../../src/symbol/symbol_layout'; import {Placement} from '../../../src/symbol/placement'; import Transform from '../../../src/geo/transform'; @@ -14,6 +12,7 @@ import {OverscaledTileID} from '../../../src/source/tile_id'; import Tile from '../../../src/source/tile'; import CrossTileSymbolIndex from '../../../src/symbol/cross_tile_symbol_index'; import FeatureIndex from '../../../src/data/feature_index'; +import {createSymbolBucket} from '../../util/create_symbol_layer'; // Load a point feature from fixture tile. const vt = new VectorTile(new Protobuf(fs.readFileSync(path.join(__dirname, '/../../fixtures/mbsv5-6-18-23.vector.pbf')))); @@ -29,21 +28,8 @@ transform.cameraToCenterDistance = 100; const stacks = {'Test': glyphs}; -function bucketSetup() { - const layer = new SymbolStyleLayer({ - id: 'test', - type: 'symbol', - layout: {'text-font': ['Test'], 'text-field': 'abcde'}, - filter: featureFilter() - }); - layer.recalculate({zoom: 0, zoomHistory: {}}); - - return new SymbolBucket({ - overscaling: 1, - zoom: 0, - collisionBoxArray, - layers: [layer] - }); +function bucketSetup(text = 'abcde') { + return createSymbolBucket('test', 'Test', text, collisionBoxArray); } test('SymbolBucket', (t) => { @@ -98,3 +84,25 @@ test('SymbolBucket integer overflow', (t) => { t.ok(console.warn.getCall(0).calledWithMatch(/Too many glyphs being rendered in a tile./)); t.end(); }); + +test('SymbolBucket detects rtl text', (t) => { + const rtlBucket = bucketSetup('مرحبا'); + const ltrBucket = bucketSetup('hello'); + const options = {iconDependencies: {}, glyphDependencies: {}}; + rtlBucket.populate([{feature}], options); + ltrBucket.populate([{feature}], options); + + t.ok(rtlBucket.hasRTLText); + t.notOk(ltrBucket.hasRTLText); + t.end(); +}); + +test('SymbolBucket detects rtl text mixed with ltr text', (t) => { + const mixedBucket = bucketSetup('مرحبا translates to hello'); + const options = {iconDependencies: {}, glyphDependencies: {}}; + mixedBucket.populate([{feature}], options); + + t.ok(mixedBucket.hasRTLText); + t.end(); +}); + diff --git a/test/unit/source/tile.test.js b/test/unit/source/tile.test.js index e36b2994892..906d3a82aef 100644 --- a/test/unit/source/tile.test.js +++ b/test/unit/source/tile.test.js @@ -1,4 +1,5 @@ import {test} from '../../util/test'; +import {createSymbolBucket} from '../../util/create_symbol_layer'; import Tile from '../../../src/source/tile'; import GeoJSONWrapper from '../../../src/source/geojson_wrapper'; import {OverscaledTileID} from '../../../src/source/tile_id'; @@ -8,6 +9,7 @@ import vtpbf from 'vt-pbf'; import FeatureIndex from '../../../src/data/feature_index'; import {CollisionBoxArray} from '../../../src/data/array_types'; import {extend} from '../../../src/util/util'; +import {lazyLoadRTLTextPlugin} from '../../../src/source/rtl_text_plugin'; import {serialize, deserialize} from '../../../src/util/web_worker_transfer'; test('querySourceFeatures', (t) => { @@ -263,6 +265,27 @@ test('expiring tiles', (t) => { t.end(); }); +test('rtl text detection', (t) => { + t.test('Tile#hasRTLText is true when a tile loads a symbol bucket with rtl text', (t) => { + const tile = new Tile(new OverscaledTileID(1, 0, 1, 1, 1)); + const symbolBucket = createSymbolBucket('test', 'Test', 'مرحبا', new CollisionBoxArray()); + symbolBucket.hasRTLText = true; + tile.loadVectorData( + createVectorData({rawTileData: createRawTileData(), buckets: [symbolBucket]}), + createPainter({ + getLayer() { + return symbolBucket.layers[0] + } + }) + ); + + t.ok(tile.hasRTLText); + t.end(); + }); + + t.end(); +}); + function createRawTileData() { return fs.readFileSync(path.join(__dirname, '/../../fixtures/mbsv5-6-18-23.vector.pbf')); } @@ -276,6 +299,6 @@ function createVectorData(options) { }, options); } -function createPainter() { - return {style: {}}; +function createPainter(styleStub = {}) { + return {style: styleStub}; } diff --git a/test/util/create_symbol_layer.js b/test/util/create_symbol_layer.js new file mode 100644 index 00000000000..181a648e8fb --- /dev/null +++ b/test/util/create_symbol_layer.js @@ -0,0 +1,20 @@ +import SymbolBucket from '../../src/data/bucket/symbol_bucket'; +import SymbolStyleLayer from '../../src/style/style_layer/symbol_style_layer'; +import featureFilter from '../../src/style-spec/feature_filter'; + +export function createSymbolBucket(layerId, font, text, collisionBoxArray) { + const layer = new SymbolStyleLayer({ + id: layerId, + type: 'symbol', + layout: {'text-font': [font], 'text-field': text}, + filter: featureFilter() + }); + layer.recalculate({zoom: 0, zoomHistory: {}}); + + return new SymbolBucket({ + overscaling: 1, + zoom: 0, + collisionBoxArray, + layers: [layer] + }); +} \ No newline at end of file From e01d0870775e410698ff4c9b4bd089c226e248ca Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Tue, 10 Dec 2019 17:50:18 -0800 Subject: [PATCH 4/6] Fix lint errors --- test/unit/source/tile.test.js | 3 +-- test/util/create_symbol_layer.js | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/unit/source/tile.test.js b/test/unit/source/tile.test.js index 906d3a82aef..6623b3db48c 100644 --- a/test/unit/source/tile.test.js +++ b/test/unit/source/tile.test.js @@ -9,7 +9,6 @@ import vtpbf from 'vt-pbf'; import FeatureIndex from '../../../src/data/feature_index'; import {CollisionBoxArray} from '../../../src/data/array_types'; import {extend} from '../../../src/util/util'; -import {lazyLoadRTLTextPlugin} from '../../../src/source/rtl_text_plugin'; import {serialize, deserialize} from '../../../src/util/web_worker_transfer'; test('querySourceFeatures', (t) => { @@ -274,7 +273,7 @@ test('rtl text detection', (t) => { createVectorData({rawTileData: createRawTileData(), buckets: [symbolBucket]}), createPainter({ getLayer() { - return symbolBucket.layers[0] + return symbolBucket.layers[0]; } }) ); diff --git a/test/util/create_symbol_layer.js b/test/util/create_symbol_layer.js index 181a648e8fb..7ddde7b7101 100644 --- a/test/util/create_symbol_layer.js +++ b/test/util/create_symbol_layer.js @@ -17,4 +17,4 @@ export function createSymbolBucket(layerId, font, text, collisionBoxArray) { collisionBoxArray, layers: [layer] }); -} \ No newline at end of file +} From 315742d49d9017da10073566a279664c11c61e63 Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Thu, 12 Dec 2019 13:45:05 -0800 Subject: [PATCH 5/6] Add test for SymbolBucket#isEmpty being false if bucket has rtl text --- test/unit/data/symbol_bucket.test.js | 12 ++++++++++++ test/unit/source/tile.test.js | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/test/unit/data/symbol_bucket.test.js b/test/unit/data/symbol_bucket.test.js index dc49b130f32..bb830a5b27c 100644 --- a/test/unit/data/symbol_bucket.test.js +++ b/test/unit/data/symbol_bucket.test.js @@ -97,6 +97,18 @@ test('SymbolBucket detects rtl text', (t) => { t.end(); }); +// Test to prevent symbol bucket with rtl from text being culled by worker serialization. +test('SymbolBucket with rtl text is NOT empty even though no symbol instances are created', (t) => { + const rtlBucket = bucketSetup('مرحبا'); + const options = {iconDependencies: {}, glyphDependencies: {}}; + rtlBucket.createArrays(); + rtlBucket.populate([{feature}], options); + + t.notOk(rtlBucket.isEmpty()); + t.equal(rtlBucket.symbolInstances.length, 0); + t.end(); +}); + test('SymbolBucket detects rtl text mixed with ltr text', (t) => { const mixedBucket = bucketSetup('مرحبا translates to hello'); const options = {iconDependencies: {}, glyphDependencies: {}}; diff --git a/test/unit/source/tile.test.js b/test/unit/source/tile.test.js index 6623b3db48c..a253a801793 100644 --- a/test/unit/source/tile.test.js +++ b/test/unit/source/tile.test.js @@ -267,7 +267,9 @@ test('expiring tiles', (t) => { test('rtl text detection', (t) => { t.test('Tile#hasRTLText is true when a tile loads a symbol bucket with rtl text', (t) => { const tile = new Tile(new OverscaledTileID(1, 0, 1, 1, 1)); - const symbolBucket = createSymbolBucket('test', 'Test', 'مرحبا', new CollisionBoxArray()); + // Create a stub symbol bucket + const symbolBucket = createSymbolBucket('test', 'Test', 'test', new CollisionBoxArray()); + // symbolBucket phasnt been populateed yet so we force override the value in the stub symbolBucket.hasRTLText = true; tile.loadVectorData( createVectorData({rawTileData: createRawTileData(), buckets: [symbolBucket]}), From 1a1ff6de87d0e6cb0c4bdc2ca40965acf2955db7 Mon Sep 17 00:00:00 2001 From: Arindam Bose Date: Fri, 13 Dec 2019 12:14:29 -0800 Subject: [PATCH 6/6] Fix comment typos --- test/unit/source/tile.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/source/tile.test.js b/test/unit/source/tile.test.js index a253a801793..11f6f3b8420 100644 --- a/test/unit/source/tile.test.js +++ b/test/unit/source/tile.test.js @@ -269,7 +269,7 @@ test('rtl text detection', (t) => { const tile = new Tile(new OverscaledTileID(1, 0, 1, 1, 1)); // Create a stub symbol bucket const symbolBucket = createSymbolBucket('test', 'Test', 'test', new CollisionBoxArray()); - // symbolBucket phasnt been populateed yet so we force override the value in the stub + // symbolBucket has not been populated yet so we force override the value in the stub symbolBucket.hasRTLText = true; tile.loadVectorData( createVectorData({rawTileData: createRawTileData(), buckets: [symbolBucket]}),