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.

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

Add option to load rtl-text-plugin lazily #8865

Merged
merged 14 commits into from
Oct 24, 2019
32 changes: 32 additions & 0 deletions debug/rtl-plugin-autoload.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<!DOCTYPE html>
<html>
<head>
<title>Mapbox GL JS debug page</title>
<meta charset='utf-8'>
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
<link rel='stylesheet' href='../dist/mapbox-gl.css' />
<style>
body { margin: 0; padding: 0; }
html, body, #map { height: 100%; }
</style>
</head>

<body>
<div id='map'></div>

<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>

var map = window.map = new mapboxgl.Map({
container: 'map',
zoom: 12.5,
center: [-77.01866, 38.888],
style: 'mapbox://styles/mapbox/streets-v10',
hash: true,
rtlTextPluginURL: 'https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js'
});

</script>
</body>
</html>
14 changes: 10 additions & 4 deletions src/data/bucket/symbol_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {register} from '../../util/web_worker_transfer';
import EvaluationParameters from '../../style/evaluation_parameters';
import Formatted from '../../style-spec/expression/types/formatted';
import ResolvedImage from '../../style-spec/expression/types/resolved_image';
import {plugin as globalRTLTextPlugin} from '../../source/rtl_text_plugin';

import type {
Bucket,
Expand Down Expand Up @@ -303,6 +304,7 @@ class SymbolBucket implements Bucket {
writingModes: Array<number>;
allowVerticalPlacement: boolean;
hasPaintOverrides: boolean;
hasRTLText: boolean;

constructor(options: BucketParameters<SymbolStyleLayer>) {
this.collisionBoxArray = options.collisionBoxArray;
Expand All @@ -315,6 +317,7 @@ class SymbolBucket implements Bucket {
this.sourceLayerIndex = options.sourceLayerIndex;
this.hasPattern = false;
this.hasPaintOverrides = false;
this.hasRTLText = false;

const layer = this.layers[0];
const unevaluatedLayoutValues = layer._unevaluatedLayout._values;
Expand Down Expand Up @@ -407,10 +410,13 @@ class SymbolBucket implements Bucket {
// but plain string token evaluation skips that pathway so do the
// conversion here.
const resolvedTokens = layer.getValueAndResolveTokens('text-field', feature, availableImages);
text = transformText(resolvedTokens instanceof Formatted ?
resolvedTokens :
Formatted.fromString(resolvedTokens),
layer, feature);
const formattedText = Formatted.factory(resolvedTokens);
if (formattedText.containsRTLText()) {
this.hasRTLText = true;
}
if (this.hasRTLText && globalRTLTextPlugin.isLoaded() || !this.hasRTLText) {
text = transformText(formattedText, layer, feature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a warning emitted here so that customers don't just see a map with no labels and wonder what the heck is going on?

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've changed the behavior now so that only when lazy loading, does it skip rendering the text.

}
}

let icon: ResolvedImage | void;
Expand Down
9 changes: 7 additions & 2 deletions src/source/rtl_text_plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const clearRTLTextPlugin = function() {
pluginURL = null;
};

export const setRTLTextPlugin = function(url: string, callback: ErrorCallback) {
export const setRTLTextPlugin = function(url: string, callback: ?ErrorCallback) {
if (pluginRequested) {
throw new Error('setRTLTextPlugin cannot be called multiple times.');
}
Expand All @@ -55,13 +55,18 @@ export const plugin: {
applyArabicShaping: ?Function,
processBidirectionalText: ?(string, Array<number>) => Array<string>,
processStyledBidirectionalText: ?(string, Array<number>, Array<number>) => Array<[string, Array<number>]>,
isLoaded: () => boolean
isLoaded: () => boolean,
isLoading: () => boolean
} = {
applyArabicShaping: null,
processBidirectionalText: null,
processStyledBidirectionalText: null,
isLoaded() {
return foregroundLoadComplete || // Foreground: loaded if the completion callback returned successfully
plugin.applyArabicShaping != null; // Background: loaded if the plugin functions have been compiled
},
isLoading() {
return pluginRequested &&
plugin.applyArabicShaping == null;
}
};
15 changes: 15 additions & 0 deletions src/source/tile.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class Tile {

symbolFadeHoldUntil: ?number;
hasSymbolBuckets: boolean;
hasRTLText: boolean;

/**
* @param {OverscaledTileID} tileID
Expand All @@ -110,6 +111,7 @@ class Tile {
this.expirationTime = null;
this.queryPadding = 0;
this.hasSymbolBuckets = false;
this.hasRTLText = false;

// Counts the number of times a response was already expired when
// received. We're using this to add a delay when making a new request
Expand Down Expand Up @@ -184,6 +186,19 @@ class Tile {
}
}

this.hasRTLText = false;
if (this.hasSymbolBuckets) {
for (const id in this.buckets) {
const bucket = this.buckets[id];
if (bucket instanceof SymbolBucket) {
if (bucket.hasRTLText) {
this.hasRTLText = true;
break;
}
}
}
}

this.queryPadding = 0;
for (const id in this.buckets) {
const bucket = this.buckets[id];
Expand Down
11 changes: 11 additions & 0 deletions src/source/vector_tile_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ 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, setRTLTextPlugin} from './rtl_text_plugin';

import type {Source} from './source';
import type {OverscaledTileID} from './tile_id';
Expand Down Expand Up @@ -153,6 +154,16 @@ 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() &&
this.map != null &&
this.map._rtlTextPluginURL
) {
setRTLTextPlugin(this.map._rtlTextPluginURL);
}
}

cacheEntryPossiblyAdded(this.dispatcher);

Expand Down
1 change: 1 addition & 0 deletions src/source/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export default class Worker {
this.workerSourceTypes[name] = WorkerSource;
};

// This is invoked by the RTL text plugin when the download via the `importScripts` call has finished, and the code has been parsed.
this.self.registerRTLTextPlugin = (rtlTextPlugin: {applyArabicShaping: Function, processBidirectionalText: Function, processStyledBidirectionalText?: Function}) => {
if (globalRTLTextPlugin.isLoaded()) {
throw new Error('RTL text plugin already registered.');
Expand Down
18 changes: 18 additions & 0 deletions src/style-spec/expression/types/formatted.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow

import {stringContainsRTLText} from "../../../util/script_detection";
import type Color from '../../util/color';

export class FormattedSection {
Expand Down Expand Up @@ -27,10 +28,27 @@ export default class Formatted {
return new Formatted([new FormattedSection(unformatted, null, null, null)]);
}

static factory(text: Formatted | string): Formatted {
if (text instanceof Formatted) {
return text;
} else {
return Formatted.fromString(text);
}
}

toString(): string {
return this.sections.map(section => section.text).join('');
}

containsRTLText(): boolean {
for (const section of this.sections) {
if (stringContainsRTLText(section.text)) {
return true;
}
}
return false;
}

serialize(): Array<mixed> {
const serialized = ["format"];
for (const section of this.sections) {
Expand Down
3 changes: 3 additions & 0 deletions src/ui/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ const defaultOptions = {
* @param {number} [options.fadeDuration=300] Controls the duration of the fade-in/fade-out animation for label collisions, in milliseconds. This setting affects all symbol layers. This setting does not affect the duration of runtime styling transitions or raster tile cross-fading.
* @param {boolean} [options.crossSourceCollisions=true] If `true`, symbols from multiple sources can collide with each other during collision detection. If `false`, collision detection is run separately for the symbols in each source.
* @param {string} [options.accessToken=null] If specified, map will use this token instead of the one defined in mapboxgl.accessToken.
* @param {string} [options.rtlTextPluginURL=null] If specified, map will use this url to load the RTL text plugin, the first time RTL text is encountered.

* @example
* var map = new mapboxgl.Map({
Expand Down Expand Up @@ -278,6 +279,7 @@ class Map extends Camera {
_mapId: number;
_localIdeographFontFamily: string;
_requestManager: RequestManager;
_rtlTextPluginURL: ?string;
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 no longer needed right?


/**
* The map's {@link ScrollZoomHandler}, which implements zooming in and out with a scroll wheel or trackpad.
Expand Down Expand Up @@ -344,6 +346,7 @@ class Map extends Camera {
this._crossSourceCollisions = options.crossSourceCollisions;
this._crossFadingFactor = 1;
this._collectResourceTiming = options.collectResourceTiming;
this._rtlTextPluginURL = options.rtlTextPluginURL;
this._renderTaskQueue = new TaskQueue();
this._controls = [];
this._mapId = uniqueId();
Expand Down
22 changes: 17 additions & 5 deletions src/util/script_detection.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,13 @@ export function charInComplexShapingScript(char: number) {
isChar['Arabic Presentation Forms-B'](char);
}

export function charInRTLScript(char: number) {
// Main blocks for Hebrew, Arabic, Thaana and other RTL scripts
return (char >= 0x0590 && char <= 0x08FF) ||
isChar['Arabic Presentation Forms-A'](char) ||
isChar['Arabic Presentation Forms-B'](char);
}

export function charInSupportedScript(char: number, canRenderRTL: boolean) {
// This is a rough heuristic: whether we "can render" a script
// actually depends on the properties of the font being used
Expand All @@ -285,11 +292,7 @@ export function charInSupportedScript(char: number, canRenderRTL: boolean) {

// Even in Latin script, we "can't render" combinations such as the fi
// ligature, but we don't consider that semantically significant.
if (!canRenderRTL &&
((char >= 0x0590 && char <= 0x08FF) ||
isChar['Arabic Presentation Forms-A'](char) ||
isChar['Arabic Presentation Forms-B'](char))) {
// Main blocks for Hebrew, Arabic, Thaana and other RTL scripts
if (!canRenderRTL && charInRTLScript(char)) {
return false;
}
if ((char >= 0x0900 && char <= 0x0DFF) ||
Expand All @@ -306,6 +309,15 @@ export function charInSupportedScript(char: number, canRenderRTL: boolean) {
return true;
}

export function stringContainsRTLText(chars: string): boolean {
for (const char of chars) {
if (charInRTLScript(char.charCodeAt(0))) {
ryanhamley marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
}
return false;
}

export function isStringInSupportedScript(chars: string, canRenderRTL: boolean) {
for (const char of chars) {
if (!charInSupportedScript(char.charCodeAt(0), canRenderRTL)) {
Expand Down