Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Multiple text formats for symbols via "format" expression (native port) #12624

Merged
merged 12 commits into from
Oct 15, 2018
Merged

Multiple text formats for symbols via "format" expression (native port) #12624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fcf395b
[core] Add automatic argument coercion for compound expressions with …
ChrisLoer Aug 9, 2018
d21e39c
[core,test] Update expression style parsing test fixture.
ChrisLoer Aug 13, 2018
2f63b7e
[core] Bidi support for styled text.
ChrisLoer Sep 19, 2018
3016300
[core] Initial implementation of 'format' expression
ChrisLoer Oct 3, 2018
b030d93
[ios, macos] Minimal darwin wrappers of 'format' expression
ChrisLoer Sep 19, 2018
3010467
[android] Minimal Android support for 'format' expression
ChrisLoer Sep 19, 2018
01c435b
[core, tests] Don't break text-field support "identity functions"
ChrisLoer Oct 1, 2018
6ab0e43
[test] Bump GL JS pin.
ChrisLoer Oct 1, 2018
69ebae6
[test] Enable 'format' Expression test.
ChrisLoer Oct 3, 2018
9328c27
[test] Add unit test for TaggedString::trim()
ChrisLoer Oct 8, 2018
37e88c0
[core, test] Add BiDi unit test
ChrisLoer Oct 9, 2018
fc71883
[docs] Changelog draft for 'format' expression
ChrisLoer Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ jobs:
JOBS: 1 # https://github.com/mapbox/mapbox-gl-native/issues/9108
command: |
xvfb-run --server-args="-screen 0 1024x768x24" \
build/qt-linux-x86_64/Release/mbgl-test --gtest_filter=-*.Load --gtest_filter=-Memory.Vector
build/qt-linux-x86_64/Release/mbgl-test --gtest_filter=-*.Load --gtest_filter=-Memory.Vector --gtest_filter=-BiDi.*

# ------------------------------------------------------------------------------
qt5-macos-debug:
Expand Down
6 changes: 6 additions & 0 deletions cmake/core-files.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,8 @@ include/mbgl/style/expression/dsl.hpp
include/mbgl/style/expression/error.hpp
include/mbgl/style/expression/expression.hpp
include/mbgl/style/expression/find_zoom_curve.hpp
include/mbgl/style/expression/format_expression.hpp
include/mbgl/style/expression/formatted.hpp
include/mbgl/style/expression/get_covering_stops.hpp
include/mbgl/style/expression/interpolate.hpp
include/mbgl/style/expression/interpolator.hpp
Expand All @@ -491,6 +493,8 @@ src/mbgl/style/expression/compound_expression.cpp
src/mbgl/style/expression/dsl.cpp
src/mbgl/style/expression/expression.cpp
src/mbgl/style/expression/find_zoom_curve.cpp
src/mbgl/style/expression/format_expression.cpp
src/mbgl/style/expression/formatted.cpp
src/mbgl/style/expression/get_covering_stops.cpp
src/mbgl/style/expression/interpolate.cpp
src/mbgl/style/expression/is_constant.cpp
Expand Down Expand Up @@ -620,6 +624,8 @@ src/mbgl/text/quads.cpp
src/mbgl/text/quads.hpp
src/mbgl/text/shaping.cpp
src/mbgl/text/shaping.hpp
src/mbgl/text/tagged_string.cpp
src/mbgl/text/tagged_string.hpp

# tile
include/mbgl/tile/tile_id.hpp
Expand Down
2 changes: 2 additions & 0 deletions cmake/test-files.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,14 @@ test/src/mbgl/test/util.cpp
test/src/mbgl/test/util.hpp

# text
test/text/bidi.test.cpp
test/text/cross_tile_symbol_index.test.cpp
test/text/glyph_manager.test.cpp
test/text/glyph_pbf.test.cpp
test/text/language_tag.test.cpp
test/text/local_glyph_rasterizer.test.cpp
test/text/quads.test.cpp
test/text/tagged_string.test.cpp

# tile
test/tile/custom_geometry_tile.test.cpp
Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/conversion/function.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace style {
namespace conversion {

bool hasTokens(const std::string&);
std::unique_ptr<expression::Expression> convertTokenStringToFormatExpression(const std::string&);
std::unique_ptr<expression::Expression> convertTokenStringToExpression(const std::string&);

optional<std::unique_ptr<expression::Expression>> convertFunctionToExpression(expression::type::Type, const Convertible&, Error&, bool convertTokens);
Expand Down
11 changes: 11 additions & 0 deletions include/mbgl/style/conversion/property_value.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@ struct Converter<PropertyValue<T>> {
? PropertyValue<T>(PropertyExpression<T>(convertTokenStringToExpression(t)))
: PropertyValue<T>(t);
}

PropertyValue<T> maybeConvertTokens(const expression::Formatted& t) const {
// This only works with a single-section `Formatted` created automatically
// by parsing a plain-text `text-field` property.
// Token conversion happens later than the initial string->Formatted conversion
// General purpose `format` expressions with embedded tokens are not supported
const std::string& firstUnformattedSection = t.sections[0].text;
return hasTokens(firstUnformattedSection)
? PropertyValue<T>(PropertyExpression<T>(convertTokenStringToFormatExpression(firstUnformattedSection)))
: PropertyValue<T>(t);
}
};

} // namespace conversion
Expand Down
7 changes: 2 additions & 5 deletions include/mbgl/style/expression/coercion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ namespace mbgl {
namespace style {
namespace expression {

/**
* Special form for error-coalescing coercion expressions "to-number",
* "to-color". Since these coercions can fail at runtime, they accept multiple
* arguments, only evaluating one at a time until one succeeds.
*/
class Coercion : public Expression {
public:
Coercion(type::Type type_, std::vector<std::unique_ptr<Expression>> inputs_);
Expand All @@ -23,6 +18,8 @@ class Coercion : public Expression {

EvaluationResult evaluate(const EvaluationContext& params) const override;
void eachChild(const std::function<void(const Expression&)>& visit) const override;

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Actually I think that header comment may be outdated? It implies Coercion is just for to-number and to-color, which is no longer true. But it does make me wonder whether the default Coercion behavior of serial evaluation should really be supported for the format coercion -- since I don't think there's even any way to use it (the only way to create this coercion is through the implicit coerce on the text-field property).

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 moved the comment and disabled the coercion for type::Formatted (although this is a divergence from gl-js, there shouldn't actually be a way to exercise this code path either way, and this seems like a clearer expression of intent).

mbgl::Value serialize() const override;

bool operator==(const Expression& e) const override;

Expand Down
6 changes: 5 additions & 1 deletion include/mbgl/style/expression/dsl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ std::unique_ptr<Expression> boolean(std::unique_ptr<Expression>);

std::unique_ptr<Expression> toColor(std::unique_ptr<Expression>);
std::unique_ptr<Expression> toString(std::unique_ptr<Expression>);

std::unique_ptr<Expression> toFormatted(std::unique_ptr<Expression>);

std::unique_ptr<Expression> get(const char* value);
std::unique_ptr<Expression> get(std::unique_ptr<Expression>);

Expand Down Expand Up @@ -78,6 +79,9 @@ std::unique_ptr<Expression> interpolate(Interpolator interpolator,
double input3, std::unique_ptr<Expression> output3);

std::unique_ptr<Expression> concat(std::vector<std::unique_ptr<Expression>> inputs);

std::unique_ptr<Expression> format(const char* value);
std::unique_ptr<Expression> format(std::unique_ptr<Expression>);

} // namespace dsl
} // namespace expression
Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/expression/expression.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ enum class Kind : int32_t {
Any,
All,
Comparison,
FormatExpression,
};

class Expression {
Expand Down
52 changes: 52 additions & 0 deletions include/mbgl/style/expression/format_expression.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#pragma once

#include <mbgl/style/expression/expression.hpp>
#include <mbgl/style/expression/formatted.hpp>
#include <mbgl/style/expression/parsing_context.hpp>
#include <mbgl/style/conversion.hpp>

#include <memory>

namespace mbgl {
namespace style {
namespace expression {

struct FormatExpressionSection {
FormatExpressionSection(std::unique_ptr<Expression> text_,
optional<std::unique_ptr<Expression>> fontScale_,
optional<std::unique_ptr<Expression>> textFont_);

std::shared_ptr<Expression> text;
optional<std::shared_ptr<Expression>> fontScale;
optional<std::shared_ptr<Expression>> textFont;
};

class FormatExpression : public Expression {
public:
FormatExpression(std::vector<FormatExpressionSection> sections);

EvaluationResult evaluate(const EvaluationContext&) const override;
static ParseResult parse(const mbgl::style::conversion::Convertible&, ParsingContext&);

void eachChild(const std::function<void(const Expression&)>&) const override;

bool operator==(const Expression& e) const override;

std::vector<optional<Value>> possibleOutputs() const override {
// Technically the combinatoric set of all children
// Usually, this.text will be undefined anyway
return { nullopt };
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this value contain an empty optional rather than being an empty vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The direct answer is "because CompoundExpression does and I copied it". But once I started looking for a justification I found at least one place where we have logic that depends on a nullopt being in the list in order to indicate "can't statically determine outputs" (vs. "no possible outputs"):

mapbox-gl-native/src/mbgl/util/font_stack.cpp

Lines 41 to 48 in d21c126

for (const auto& value : function.possibleOutputs()) {
if (value) {
result.insert(*value);
} else {
Log::Warning(Event::ParseStyle, "Layer '%s' has an invalid value for text-font and will not render text. Output values must be contained as literals within the expression.", impl.id.c_str());
break;
}
}

This actually makes me wonder if the {} returned by Error::possibleOutputs is correct... 🤔

}

mbgl::Value serialize() const override;
std::string getOperator() const override { return "format"; }
private:
std::vector<FormatExpressionSection> sections;
std::unique_ptr<Expression> text;
optional<std::unique_ptr<Expression>> fontScale;
optional<std::unique_ptr<Expression>> textFont;
};

} // namespace expression
} // namespace style
} // namespace mbgl
62 changes: 62 additions & 0 deletions include/mbgl/style/expression/formatted.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
#pragma once

#include <mbgl/style/conversion.hpp>
#include <mbgl/util/font_stack.hpp>
#include <mbgl/util/optional.hpp>
#include <mbgl/util/variant.hpp>

#include <vector>
#include <string>

namespace mbgl {
namespace style {
namespace expression {

struct FormattedSection {
FormattedSection(std::string text_, optional<double> fontScale_, optional<FontStack> fontStack_)
: text(std::move(text_))
, fontScale(std::move(fontScale_))
, fontStack(std::move(fontStack_))
{}
std::string text;
optional<double> fontScale;
optional<FontStack> fontStack;
};

class Formatted {
public:
Formatted() = default;

Formatted(const char* plainU8String) {
sections.emplace_back(std::string(plainU8String), nullopt, nullopt);
}

Formatted(std::vector<FormattedSection> sections_)
: sections(std::move(sections_))
{}

bool operator==(const Formatted& ) const;

std::string toString() const;

bool empty() const {
return sections.empty() || sections.at(0).text.empty();
}

std::vector<FormattedSection> sections;
};

} // namespace expression

namespace conversion {

template <>
struct Converter<mbgl::style::expression::Formatted> {
public:
optional<mbgl::style::expression::Formatted> operator()(const Convertible& value, Error& error) const;
};

} // namespace conversion

} // namespace style
} // namespace mbgl
9 changes: 9 additions & 0 deletions include/mbgl/style/expression/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ struct CollatorType {
std::string getName() const { return "collator"; }
bool operator==(const CollatorType&) const { return true; }
};

struct FormattedType {
constexpr FormattedType() {}; // NOLINT
std::string getName() const { return "formatted"; }
bool operator==(const FormattedType&) const { return true; }
};


constexpr NullType Null;
constexpr NumberType Number;
Expand All @@ -75,6 +82,7 @@ constexpr ColorType Color;
constexpr ValueType Value;
constexpr ObjectType Object;
constexpr CollatorType Collator;
constexpr FormattedType Formatted;
constexpr ErrorType Error;

struct Array;
Expand All @@ -89,6 +97,7 @@ using Type = variant<
ValueType,
mapbox::util::recursive_wrapper<Array>,
CollatorType,
FormattedType,
ErrorType>;

struct Array {
Expand Down
2 changes: 2 additions & 0 deletions include/mbgl/style/expression/value.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <mbgl/style/expression/collator.hpp>
#include <mbgl/style/expression/formatted.hpp>
#include <mbgl/style/expression/type.hpp>
#include <mbgl/style/position.hpp>
#include <mbgl/style/types.hpp>
Expand All @@ -25,6 +26,7 @@ using ValueBase = variant<
std::string,
Color,
Collator,
Formatted,
mapbox::util::recursive_wrapper<std::vector<Value>>,
mapbox::util::recursive_wrapper<std::unordered_map<std::string, Value>>>;
struct Value : ValueBase {
Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/background_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/circle_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/fill_extrusion_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/fill_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/heatmap_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/hillshade_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/layer.hpp.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/line_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
1 change: 1 addition & 0 deletions include/mbgl/style/layers/raster_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down
7 changes: 4 additions & 3 deletions include/mbgl/style/layers/symbol_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/style/layer.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/property_value.hpp>
#include <mbgl/style/expression/formatted.hpp>

#include <mbgl/util/color.hpp>

Expand Down Expand Up @@ -121,9 +122,9 @@ class SymbolLayer : public Layer {
PropertyValue<AlignmentType> getTextRotationAlignment() const;
void setTextRotationAlignment(PropertyValue<AlignmentType>);

static PropertyValue<std::string> getDefaultTextField();
PropertyValue<std::string> getTextField() const;
void setTextField(PropertyValue<std::string>);
static PropertyValue<expression::Formatted> getDefaultTextField();
PropertyValue<expression::Formatted> getTextField() const;
void setTextField(PropertyValue<expression::Formatted>);

static PropertyValue<std::vector<std::string>> getDefaultTextFont();
PropertyValue<std::vector<std::string>> getTextFont() const;
Expand Down
5 changes: 3 additions & 2 deletions include/mbgl/util/font_stack.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ namespace mbgl {

// An array of font names
using FontStack = std::vector<std::string>;
using FontStackHash = std::size_t;

std::string fontStackToString(const FontStack&);

struct FontStackHash {
std::size_t operator()(const FontStack&) const;
struct FontStackHasher {
FontStackHash operator()(const FontStack&) const;
};

// Statically evaluate layer properties to determine what font stacks are used.
Expand Down
2 changes: 1 addition & 1 deletion mapbox-gl-js
Open in desktop
Submodule mapbox-gl-js updated 9 files
+5 −2 src/css/mapbox-gl.css
+3 −7 src/source/canvas_source.js
+8 −4 src/style-spec/expression/types/formatted.js
+2 −7 test/integration/expression-tests/format/basic/test.json
+ − test/integration/render-tests/canvas/default/expected.png
+52 −0 test/integration/render-tests/canvas/default/style.json
+ − test/integration/render-tests/regressions/mapbox-gl-js#7302/expected.png
+52 −0 test/integration/render-tests/regressions/mapbox-gl-js#7302/style.json
+20 −0 test/suite_implementation.js