Skip to content

Commit

Permalink
Complete transition to coercing 'text-field' to 'formatted' at evalua…
Browse files Browse the repository at this point in the history 
…tion time.

 - Add coercion after call to 'getValueAndResolveTokens`, since the non-expression pathway here skips the coercion logic in parsing_context
 - Remove 'string | Formatted' typing and 'text instanceof Formatted' checks
 - Add Coercion support for 'Formatted', along with dedicated serialization
 - Use Coercion when parsing expected.kind === 'formatted' instead of directly creating a FormatExpression. This is necessary to accommodate expressions such as 'coalesce' that introduce a typeAnnotation.
  • Loading branch information
@ChrisLoer
ChrisLoer committed Sep 17, 2018
1 parent 11b6a19 commit f5cd8c3
Show file tree
Hide file tree
Showing 11 changed files with 62 additions and 68 deletions.
2 changes: 1 addition & 1 deletion build/generate-style-code.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ global.flowType = function (property) {
case 'color':
return `Color`;
case 'formatted':
return `string | Formatted`;
return `Formatted`;
case 'array':
if (property.length) {
return `[${new Array(property.length).fill(flowType({type: property.value})).join(', ')}]`;
Expand Down
33 changes: 16 additions & 17 deletions src/data/bucket/symbol_bucket.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import Anchor from '../../symbol/anchor';
import { getSizeData } from '../../symbol/symbol_size';
import { register } from '../../util/web_worker_transfer';
import EvaluationParameters from '../../style/evaluation_parameters';
import Formatted from '../../style-spec/expression/types/formatted';
import Formatted, { FormattedSection } from '../../style-spec/expression/types/formatted';


import type {
Expand Down Expand Up @@ -56,7 +56,7 @@ export type CollisionArrays = {
};

export type SymbolFeature = {|
text: string | Formatted | void,
text: Formatted | void,
icon: string | void,
index: number,
sourceLayerIndex: number,
Expand Down Expand Up @@ -349,10 +349,16 @@ class SymbolBucket implements Bucket {
continue;
}

let text;
let text: Formatted | void;
if (hasText) {
text = layer.getValueAndResolveTokens('text-field', feature);
text = transformText(text, layer, feature);
// Expression evaluation will automatically coerce to Formatted
// but plain string token evaluation skips that pathway so do the
// conversion here.
const resolvedTokens = layer.getValueAndResolveTokens('text-field', feature);
text = transformText(resolvedTokens instanceof Formatted ?
resolvedTokens :
Formatted.fromString(resolvedTokens),
layer, feature);
}

let icon;
Expand Down Expand Up @@ -384,20 +390,13 @@ class SymbolBucket implements Bucket {

if (text) {
const fontStack = textFont.evaluate(feature, {}).join(',');
const stack = stacks[fontStack] = stacks[fontStack] || {};
const textAlongLine = layout.get('text-rotation-alignment') === 'map' && layout.get('symbol-placement') !== 'point';
if (text instanceof Formatted) {
for (const section of text.sections) {
const doesAllowVerticalWritingMode = allowsVerticalWritingMode(text.toString());
const sectionFont = section.fontStack || fontStack;
const sectionStack = stacks[sectionFont] = stacks[sectionFont] || {};
this.calculateGlyphDependencies(section.text, sectionStack, textAlongLine, doesAllowVerticalWritingMode);
}
} else {
const doesAllowVerticalWritingMode = allowsVerticalWritingMode(text);
this.calculateGlyphDependencies(text, stack, textAlongLine, doesAllowVerticalWritingMode);
for (const section of text.sections) {
const doesAllowVerticalWritingMode = allowsVerticalWritingMode(text.toString());
const sectionFont = section.fontStack || fontStack;
const sectionStack = stacks[sectionFont] = stacks[sectionFont] || {};
this.calculateGlyphDependencies(section.text, sectionStack, textAlongLine, doesAllowVerticalWritingMode);
}

}
}

Expand Down
9 changes: 9 additions & 0 deletions src/style-spec/expression/definitions/coercion.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import assert from 'assert';
import {BooleanType, ColorType, NumberType, StringType, ValueType} from '../types';
import {Color, toString as valueToString, validateRGBA} from '../values';
import RuntimeError from '../runtime_error';
import Formatted, {FormattedSection} from '../types/formatted';
import FormatExpression from '../definitions/format';

import type { Expression } from '../expression';
import type ParsingContext from '../parsing_context';
Expand Down Expand Up @@ -93,6 +95,10 @@ class Coercion implements Expression {
return num;
}
throw new RuntimeError(`Could not convert ${JSON.stringify(value)} to number.`);
} else if (this.type.kind === 'formatted') {
// There is no explicit 'to-formatted' but this coercion can be implicitly
// created by properties that expect the 'formatted' type.
return Formatted.fromString(valueToString(this.args[0].evaluate(ctx)));
} else {
return valueToString(this.args[0].evaluate(ctx));
}
Expand All @@ -107,6 +113,9 @@ class Coercion implements Expression {
}

serialize() {
if (this.type.kind === 'formatted') {
return new FormatExpression([{text: this.args[0], scale: null, font: null}]).serialize();
}
const serialized = [`to-${this.type.kind}`];
this.eachChild(child => { serialized.push(child.serialize()); });
return serialized;
Expand Down
7 changes: 1 addition & 6 deletions src/style-spec/expression/parsing_context.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// @flow

import assert from 'assert';
import Scope from './scope';
import { checkSubtype } from './types';
import ParsingError from './parsing_error';
Expand All @@ -10,7 +9,6 @@ import Coercion from './definitions/coercion';
import EvaluationContext from './evaluation_context';
import CompoundExpression from './compound_expression';
import CollatorExpression from './definitions/collator';
import FormatExpression from './definitions/format';
import {isGlobalPropertyConstant, isFeatureConstant} from './is_constant';
import Var from './definitions/var';

Expand Down Expand Up @@ -115,11 +113,8 @@ class ParsingContext {
//
if ((expected.kind === 'string' || expected.kind === 'number' || expected.kind === 'boolean' || expected.kind === 'object' || expected.kind === 'array') && actual.kind === 'value') {
parsed = annotate(parsed, expected, options.typeAnnotation || 'assert');
} else if (expected.kind === 'color' && (actual.kind === 'value' || actual.kind === 'string')) {
} else if ((expected.kind === 'color' || expected.kind === 'formatted') && (actual.kind === 'value' || actual.kind === 'string')) {
parsed = annotate(parsed, expected, options.typeAnnotation || 'coerce');
} else if (expected.kind === 'formatted' && actual.kind !== 'formatted') {
assert(!options.typeAnnotation);
parsed = new FormatExpression([{text: parsed, scale: null, font: null}]);
} else if (this.checkSubtype(expected, actual)) {
return null;
}
Expand Down
4 changes: 4 additions & 0 deletions src/style-spec/expression/types/formatted.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ export default class Formatted {
this.sections = sections;
}

static fromString(unformatted: string): Formatted {
return new Formatted([new FormattedSection(unformatted, null, null)]);
}

toString(): string {
return this.sections.map(section => section.text).join('');
}
Expand Down
2 changes: 1 addition & 1 deletion src/style/style_layer/symbol_style_layer_properties.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export type LayoutProps = {|
"icon-pitch-alignment": DataConstantProperty<"map" | "viewport" | "auto">,
"text-pitch-alignment": DataConstantProperty<"map" | "viewport" | "auto">,
"text-rotation-alignment": DataConstantProperty<"map" | "viewport" | "auto">,
"text-field": DataDrivenProperty<string | Formatted>,
"text-field": DataDrivenProperty<Formatted>,
"text-font": DataDrivenProperty<Array<string>>,
"text-size": DataDrivenProperty<number>,
"text-max-width": DataDrivenProperty<number>,
Expand Down
3 changes: 1 addition & 2 deletions src/symbol/mergelines.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// @flow

import type {SymbolFeature} from '../data/bucket/symbol_bucket';
import Formatted from '../style-spec/expression/types/formatted';

export default function (features: Array<SymbolFeature>): Array<SymbolFeature> {
const leftIndex: {[string]: number} = {};
Expand Down Expand Up @@ -42,7 +41,7 @@ export default function (features: Array<SymbolFeature>): Array<SymbolFeature> {
for (let k = 0; k < features.length; k++) {
const feature = features[k];
const geom = feature.geometry;
const text = feature.text instanceof Formatted ? feature.text.toString() : feature.text;
const text = feature.text ? feature.text.toString() : null;

if (!text) {
add(k);
Expand Down
30 changes: 11 additions & 19 deletions src/symbol/shaping.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,25 +53,17 @@ class TaggedString {
this.sections = [];
}

static fromFeature(text: string | Formatted, defaultFontStack: string) {
static fromFeature(text: Formatted, defaultFontStack: string) {
const result = new TaggedString();
if (text instanceof Formatted) {
for (let i = 0; i < text.sections.length; i++) {
const section = text.sections[i];
result.sections.push({
scale: section.scale || 1,
fontStack: section.fontStack || defaultFontStack
});
result.text += section.text;
for (let j = 0; j < section.text.length; j++) {
result.sectionIndex.push(i);
}
}
} else {
result.text = text;
result.sections.push({ scale: 1, fontStack: defaultFontStack });
for (let i = 0; i < text.length; i++) {
result.sectionIndex.push(0);
for (let i = 0; i < text.sections.length; i++) {
const section = text.sections[i];
result.sections.push({
scale: section.scale || 1,
fontStack: section.fontStack || defaultFontStack
});
result.text += section.text;
for (let j = 0; j < section.text.length; j++) {
result.sectionIndex.push(i);
}
}
return result;
Expand Down Expand Up @@ -142,7 +134,7 @@ function breakLines(input: TaggedString, lineBreakPoints: Array<number>): Array<
return lines;
}

function shapeText(text: string | Formatted,
function shapeText(text: Formatted,
glyphs: {[string]: {[number]: ?StyleGlyph}},
defaultFontStack: string,
maxWidth: number,
Expand Down
3 changes: 1 addition & 2 deletions src/symbol/symbol_layout.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import classifyRings from '../util/classify_rings';
import EXTENT from '../data/extent';
import SymbolBucket from '../data/bucket/symbol_bucket';
import EvaluationParameters from '../style/evaluation_parameters';
import Formatted from '../style-spec/expression/types/formatted';
import {SIZE_PACK_FACTOR} from './symbol_size';

import type {Shaping, PositionedIcon} from './shaping';
Expand Down Expand Up @@ -106,7 +105,7 @@ export function performSymbolLayout(bucket: SymbolBucket,
const shapedTextOrientations = {};
const text = feature.text;
if (text) {
const unformattedText = text instanceof Formatted ? text.toString() : text;
const unformattedText = text.toString();
const textOffset: [number, number] = (layout.get('text-offset').evaluate(feature, {}).map((t)=> t * oneEm): any);
const spacing = layout.get('text-letter-spacing').evaluate(feature, {}) * oneEm;
const spacingIfAllowed = allowsLetterSpacing(unformattedText) ? spacing : 0;
Expand Down
14 changes: 5 additions & 9 deletions src/symbol/transform_text.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,9 @@ function transformText(text: string, layer: SymbolStyleLayer, feature: Feature)
}


export default function(text: string | Formatted, layer: SymbolStyleLayer, feature: Feature) {
if (text instanceof Formatted) {
text.sections.forEach(section => {
section.text = transformText(section.text, layer, feature);
});
return text;
} else {
return transformText(text, layer, feature);
}
export default function(text: Formatted, layer: SymbolStyleLayer, feature: Feature): Formatted {
text.sections.forEach(section => {
section.text = transformText(section.text, layer, feature);
});
return text;
}
23 changes: 12 additions & 11 deletions test/unit/symbol/shaping.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { test } from 'mapbox-gl-js-test';
import fs from 'fs';
import path from 'path';
import * as shaping from '../../../src/symbol/shaping';
import Formatted from '../../../src/style-spec/expression/types/formatted';
const WritingMode = shaping.WritingMode;

let UPDATE = false;
Expand All @@ -20,50 +21,50 @@ test('shaping', (t) => {

JSON.parse('{}');

shaped = shaping.shapeText(`hi${String.fromCharCode(0)}`, glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString(`hi${String.fromCharCode(0)}`), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-null.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, JSON.parse(fs.readFileSync(path.join(__dirname, '/../../expected/text-shaping-null.json'))));

// Default shaping.
shaped = shaping.shapeText('abcde', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-default.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, JSON.parse(fs.readFileSync(path.join(__dirname, '/../../expected/text-shaping-default.json'))));

// Letter spacing.
shaped = shaping.shapeText('abcde', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0.125 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0.125 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-spacing.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, JSON.parse(fs.readFileSync(path.join(__dirname, '/../../expected/text-shaping-spacing.json'))));

// Line break.
shaped = shaping.shapeText('abcde abcde', glyphs, fontStack, 4 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde abcde'), glyphs, fontStack, 4 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-linebreak.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, require('../../expected/text-shaping-linebreak.json'));

const expectedNewLine = JSON.parse(fs.readFileSync(path.join(__dirname, '/../../expected/text-shaping-newline.json')));

shaped = shaping.shapeText('abcde\nabcde', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde\nabcde'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-newline.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, expectedNewLine);

shaped = shaping.shapeText('abcde\r\nabcde', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde\r\nabcde'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
t.deepEqual(shaped.positionedGlyphs, expectedNewLine.positionedGlyphs);

const expectedNewLinesInMiddle = JSON.parse(fs.readFileSync(path.join(__dirname, '/../../expected/text-shaping-newlines-in-middle.json')));

shaped = shaping.shapeText('abcde\n\nabcde', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString('abcde\n\nabcde'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0, [0, 0], oneEm, WritingMode.horizontal);
if (UPDATE) fs.writeFileSync(path.join(__dirname, '/../../expected/text-shaping-newlines-in-middle.json'), JSON.stringify(shaped, null, 2));
t.deepEqual(shaped, expectedNewLinesInMiddle);

// Null shaping.
shaped = shaping.shapeText('', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString(''), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
t.equal(false, shaped);

shaped = shaping.shapeText(String.fromCharCode(0), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString(String.fromCharCode(0)), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
t.equal(false, shaped);

// https://github.com/mapbox/mapbox-gl-js/issues/3254
shaped = shaping.shapeText(' foo bar\n', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
const shaped2 = shaping.shapeText('foo bar', glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
shaped = shaping.shapeText(Formatted.fromString(' foo bar\n'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
const shaped2 = shaping.shapeText(Formatted.fromString('foo bar'), glyphs, fontStack, 15 * oneEm, oneEm, 'center', 'center', 0 * oneEm, [0, 0], oneEm, WritingMode.horizontal);
t.same(shaped.positionedGlyphs, shaped2.positionedGlyphs);

t.end();
Expand Down

0 comments on commit f5cd8c3

Please sign in to comment.