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

Formatted "text-field" property setter #13358

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

LukasPaczos
Copy link
Contributor

Closes #13172.

Exposes PropertyFactory#textField(Formatted) property setter. A Java Formatted object is passed as an array that's converted to a core Formatted object.

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Nov 13, 2018
@LukasPaczos LukasPaczos added this to the android-v7.0.0-iowaska milestone Nov 13, 2018
@LukasPaczos LukasPaczos force-pushed the lp-13173-formatted-plain-text-getter branch from 16c8a66 to b5f2eba Compare November 14, 2018 11:16
@LukasPaczos LukasPaczos force-pushed the lp-13172-formatted-property-setter branch from 111b17c to 80e6cbc Compare November 14, 2018 11:17
@LukasPaczos LukasPaczos changed the base branch from lp-13173-formatted-plain-text-getter to master December 3, 2018 15:11
@LukasPaczos LukasPaczos force-pushed the lp-13172-formatted-property-setter branch 2 times, most recently from b91aa6c to 66d3d3b Compare December 3, 2018 15:28
@LukasPaczos
Copy link
Contributor Author

Tests are finally fixed, this one is ready for a review @tobrun @ChrisLoer.

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

🎉 This looks good! But it does concern me a little that we have a new array-based serialization format for Formatted. I don't actually see a problem with it, it just seems confusing that sometimes a Formatted will be represented as:

['format', 'foo', { 'font-scale': 1.5 }, 'bar', { 'font-scale': 0.5 }]

And another time it'll be represented as:

[['foo', { 'font-scale': 1.5 }], ['bar', { 'font-scale': 0.5 }]]

return Formatted(result->c_str());
} else {
error.message = "Not supported Formatted type property.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "Formatted must be plain string or array type"?

@LukasPaczos
Copy link
Contributor Author

True, the first one represents an expression, the other represents a constant value. If we were to represent the consant on the Java side using the same array as the expression, it would have gone through the coercion proccess and be evaluated to a constant as well, but since it's a new base type, distinguishing the paths and types seems clearer to me in this specific case.

@LukasPaczos LukasPaczos force-pushed the lp-13172-formatted-property-setter branch from 66d3d3b to 44a87fd Compare December 4, 2018 15:36
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

🎉

@LukasPaczos LukasPaczos merged commit 794b851 into master Dec 4, 2018
@LukasPaczos LukasPaczos deleted the lp-13172-formatted-property-setter branch December 4, 2018 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants