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>
mapboxgl.setRTLTextPlugin('https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js', null, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

So the expectation here is that the plugin won't be loaded right away? How would we see that? Can this be modified to fly to somewhere with RTL text label names and emit an alert when the popup is loaded so we can see it working? And can this be combined with the existing rtl.html debug page?

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 made the plugin loading interactive in the rtl.html page, so you can click a button and see the status live.


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
});

</script>
</body>
</html>
19 changes: 15 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,18 @@ 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() || // Use the rtlText plugin shape text
!this.hasRTLText || // non-rtl terxt so can proceed safely
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: terxt

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 minor, but it seems like the order here should be !this.hasRTLText || !globalRTLTextPlugin.isAvailableInWorker() || this.hasRTLText && globalRTLTextPlugin.isLoaded(). Start with the most generic check and work your way down to more complex ones.

!globalRTLTextPlugin.isAvailableInWorker() // We-doent intend to async-load the rtl text plugin, so proceed with incorrect shaping
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We-doent

) {
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
107 changes: 90 additions & 17 deletions src/source/rtl_text_plugin.js
Original file line number Diff line number Diff line change
@@ -1,50 +1,73 @@
// @flow

import {Event, Evented} from '../util/evented';
import {getArrayBuffer} from '../util/ajax';
import browser from '../util/browser';
import window from '../util/window';

const status = {
unavailable: 'unavailable',
loading: 'loading',
unavailable: 'unavailable', // Not loaded
available: 'available', // Host url specified, but we havent started loading yet
arindam1993 marked this conversation as resolved.
Show resolved Hide resolved
loading: 'loading', // request in-flight
downloaded: 'downloaded', //plugin loaded and cached on main-thread and pluginBlobURL for worker is generated
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between downloaded and loaded, and why would we want to expose that distinction?

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally don't think these statuses make intuitive sense. We shouldn't be asking end users to understand the intricacies of lazy loading the RTL plugin on the main thread and worker threads in order to get a status update. I hated unavailable when I included it but couldn't think of a better option. available meaning that the user has just specified a URL isn't intuitive. And I agree that downloaded probably shouldn't be exposed at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, I've changed the architecture as described in the main PR body.

loaded: 'loaded',
error: 'error'
};
let pluginStatus = status.unavailable;
let pluginURL = null;
let pluginBlobURL = null;
let lazy = null;

// store `pluginAvailable` that have occurred before the `registerPluginAvailability` bind
// so we can flush all the state updates to the workers
let eventQueue = [];
let _pluginAvailableCb = null;

let _workerAvailable = false;

export const evented = new Evented();
evented.on('pluginAvailable', (args) => {
if (!_pluginAvailableCb) {
eventQueue.push(args);
} else {
_pluginAvailableCb(args);
}
});

type CompletionCallback = (error?: Error) => void;
type ErrorCallback = (error: Error) => void;
type PluginAvailableCallback = (args: {pluginURL: ?string, lazy: ?boolean, completionCallback: CompletionCallback}) => void;

let _completionCallback;

export const getRTLTextPluginStatus = function () {
return pluginStatus;
};

export const registerForPluginAvailability = function(
callback: (args: {pluginURL: string, completionCallback: CompletionCallback}) => void
) {
if (pluginURL) {
callback({pluginURL, completionCallback: _completionCallback});
} else {
evented.once('pluginAvailable', callback);
export const registerForPluginAvailability = function(callback: PluginAvailableCallback) {
for (const event of eventQueue) {
callback(event);
}
eventQueue = [];

_pluginAvailableCb = callback;
return callback;
};

export const clearRTLTextPlugin = function() {
pluginStatus = status.unavailable;
pluginURL = null;
pluginBlobURL = null;
lazy = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

if lazy is a boolean, shouldn't it default to false?

};

export const setRTLTextPlugin = function(url: string, callback: ErrorCallback) {
if (pluginStatus === status.loading || pluginStatus === status.loaded) {
export const setRTLTextPlugin = function(url: string, callback: ?ErrorCallback, lazyLoad: ?boolean) {
if (pluginStatus === status.available || pluginStatus === status.loading || pluginStatus === status.loaded) {
throw new Error('setRTLTextPlugin cannot be called multiple times.');
}
pluginStatus = status.loading;
pluginURL = browser.resolveURL(url);
lazy = !!lazyLoad;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, if we just default to false and always treated it as a boolean, we wouldn't have to do the cast here.

pluginStatus = status.available;
_completionCallback = (error?: Error) => {
if (error) {
// Clear loaded state to allow retries
Expand All @@ -55,23 +78,73 @@ export const setRTLTextPlugin = function(url: string, callback: ErrorCallback) {
}
} else {
// Called once for each worker
pluginStatus = status.loaded;
if (!lazy) {
pluginStatus = status.loaded;
}
}
};
evented.fire(new Event('pluginAvailable', {pluginURL, completionCallback: _completionCallback}));

if (lazy) {
// Inform the worker-threads that we intend to load the plugin lazily later,
// This is so the workers can skip RTL text parsing.
evented.fire(new Event('pluginAvailable', {
pluginURL: null,
lazy,
completionCallback: _completionCallback
}));
} else {
downloadRTLTextPlugin();
}
};

export const downloadRTLTextPlugin = function() {
if (pluginStatus !== status.available) {
throw new Error('rtl-text-plugin cannot be downloaded unless a pluginURL is specified');
}
pluginStatus = status.loading;

if (pluginURL) {
getArrayBuffer({url: pluginURL}, (error, data) => {
if (error || !data) {
throw error;
} else {
const rtlBlob = new window.Blob([data], {type: 'application/javascript'});
const URL = window.URL || window.webkitURL;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need webkitURL here? i can't even really find much information on it but it seems to be deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

This may have been added in the past when Safari/Webkit didn't support window.URL. But that doesn't seem to be an issue any longer based on caniuse

pluginBlobURL = URL.createObjectURL(rtlBlob);
pluginStatus = status.downloaded;
evented.fire(new Event('pluginAvailable', {
pluginURL: pluginBlobURL,
lazy,
completionCallback: _completionCallback
}));
}
});
}
};

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,
markWorkerAvailable: () => void,
isAvailableInWorker: () => boolean
} = {
applyArabicShaping: null,
processBidirectionalText: null,
processStyledBidirectionalText: null,
isLoaded() {
return pluginStatus === status.loaded || // Foreground: loaded if the completion callback returned successfully
plugin.applyArabicShaping != null; // Background: loaded if the plugin functions have been compiled
return pluginStatus === status.loaded || // Main Thread: loaded if the completion callback returned successfully
plugin.applyArabicShaping != null; // Web-worker: loaded if the plugin functions have been compiled
},
isLoading() { // Main Thread Only: query the loading status, this function does not return the correct value in the worker context.
return pluginStatus === status.loading;
},
markWorkerAvailable() { // Worker thread only: this tells the worker threads that the plugin is available on the Main thread
_workerAvailable = true;
},
isAvailableInWorker() {
return !!_workerAvailable;
}
};
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
10 changes: 10 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, getRTLTextPluginStatus, downloadRTLTextPlugin} from './rtl_text_plugin';

import type {Source} from './source';
import type {OverscaledTileID} from './tile_id';
Expand Down Expand Up @@ -153,6 +154,15 @@ 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() === 'available'
) {
downloadRTLTextPlugin();
}
}

cacheEntryPossiblyAdded(this.dispatcher);

Expand Down
19 changes: 13 additions & 6 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 Expand Up @@ -151,13 +152,19 @@ export default class Worker {
}
}

loadRTLTextPlugin(map: string, pluginURL: string, callback: Callback<void>) {
loadRTLTextPlugin(map: string, pluginData: Object, callback: Callback<void>) {
try {
if (!globalRTLTextPlugin.isLoaded()) {
this.self.importScripts(pluginURL);
callback(globalRTLTextPlugin.isLoaded() ?
null :
new Error(`RTL Text Plugin failed to import scripts from ${pluginURL}`));
const {pluginURL, lazy} = pluginData;
if (pluginURL) {
if (!globalRTLTextPlugin.isLoaded()) {
this.self.importScripts(pluginURL);
callback(globalRTLTextPlugin.isLoaded() ?
null :
new Error(`RTL Text Plugin failed to import scripts from ${pluginURL}`));
}
} else if (lazy) {
// Set the state of the rtl text plugin in worker scope, to load the plugin if necessary.
globalRTLTextPlugin.markWorkerAvailable();
}
} catch (e) {
callback(e.toString());
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
8 changes: 5 additions & 3 deletions src/style/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,11 @@ class Style extends Evented {

const self = this;
this._rtlTextPluginCallback = Style.registerForPluginAvailability((args) => {
self.dispatcher.broadcast('loadRTLTextPlugin', args.pluginURL, args.completionCallback);
for (const id in self.sourceCaches) {
self.sourceCaches[id].reload(); // Should be a no-op if the plugin loads before any tiles load
self.dispatcher.broadcast('loadRTLTextPlugin', {pluginURL: args.pluginURL, lazy: args.lazy}, args.completionCallback);
if (args.pluginURL) {
for (const id in self.sourceCaches) {
self.sourceCaches[id].reload(); // Should be a no-op if the plugin loads before any tiles load
}
}
});

Expand Down
Loading