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

Rtl labels support #5771

Merged
merged 60 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from 53 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
60da6aa
support rtl characters (currently, only hebrew) at label to show in c…
hodbauer May 4, 2017
fc812bc
add example of supporting rtl characters to sandcastle
hodbauer May 4, 2017
8762607
add unit tests to check rtl (unfortunately, not working yet)
hodbauer May 4, 2017
64fd8d2
RTL labels now work in constructor
YonatanKra Aug 22, 2017
e3c601f
Merge remote-tracking branch 'AnalyticalGraphicsInc/master'
YonatanKra Aug 22, 2017
db98c03
Merge branch 'master' into rtl-labels-support
YonatanKra Aug 22, 2017
5826c7e
Upadated Changes.md
YonatanKra Aug 22, 2017
775e637
Updated contributors
YonatanKra Aug 23, 2017
f09b883
Support for \n in RTL
YonatanKra Aug 23, 2017
70c3ab6
Cache RTL original string
YonatanKra Aug 23, 2017
8d18aa3
Prevent glyphs parser from running for nought
YonatanKra Aug 23, 2017
52dc64f
Documentation examples
YonatanKra Aug 23, 2017
8350668
Updated contributors list
YonatanKra Aug 23, 2017
a0f0c1a
* fix descriptions
hodbauer Sep 3, 2017
41cea83
support rtl characters (currently, only hebrew) at label to show in c…
hodbauer May 4, 2017
84354e4
add example of supporting rtl characters to sandcastle
hodbauer May 4, 2017
c22adcf
add unit tests to check rtl (unfortunately, not working yet)
hodbauer May 4, 2017
f91e96d
Merge branch 'master' into rtl-labels-support
hodbauer Sep 3, 2017
281d4d3
support rtl characters (currently, only hebrew) at label to show in c…
hodbauer May 4, 2017
c1adde1
add example of supporting rtl characters to sandcastle
hodbauer May 4, 2017
6d65918
add unit tests to check rtl (unfortunately, not working yet)
hodbauer May 4, 2017
5f304a8
RTL labels now work in constructor
YonatanKra Aug 22, 2017
304e774
Updated contributors
YonatanKra Aug 23, 2017
f7dfcaa
Support for \n in RTL
YonatanKra Aug 23, 2017
10b05d2
Cache RTL original string
YonatanKra Aug 23, 2017
8878a7b
Prevent glyphs parser from running for nought
YonatanKra Aug 23, 2017
12fe81d
Documentation examples
YonatanKra Aug 23, 2017
15a3c9c
Updated contributors list
YonatanKra Aug 23, 2017
9ab1704
* fix descriptions
hodbauer Sep 3, 2017
04538ba
rename rtl to rightToLeft
hodbauer Sep 11, 2017
eb9e155
Merge remote-tracking branch 'origin/rtl-labels-support' into rtl-lab…
hodbauer Sep 11, 2017
97c5c36
support rtl characters (currently, only hebrew) at label to show in c…
hodbauer May 4, 2017
7245d72
add example of supporting rtl characters to sandcastle
hodbauer May 4, 2017
4a69abf
add unit tests to check rtl (unfortunately, not working yet)
hodbauer May 4, 2017
cfb6419
RTL labels now work in constructor
YonatanKra Aug 22, 2017
ea5ceeb
Updated contributors
YonatanKra Aug 23, 2017
c098454
Support for \n in RTL
YonatanKra Aug 23, 2017
d3df024
Cache RTL original string
YonatanKra Aug 23, 2017
7068841
Prevent glyphs parser from running for nought
YonatanKra Aug 23, 2017
bdb582e
Documentation examples
YonatanKra Aug 23, 2017
99010f2
Updated contributors list
YonatanKra Aug 23, 2017
ab9ac19
* fix descriptions
hodbauer Sep 3, 2017
84e5d95
RTL labels now work in constructor
YonatanKra Aug 22, 2017
eec1e4d
rename rtl to rightToLeft
hodbauer Sep 11, 2017
b3971a9
add link to PR at CHANGES.md
hodbauer Sep 17, 2017
69a49e8
Merge remote-tracking branch 'origin/rtl-labels-support' into rtl-lab…
hodbauer Sep 17, 2017
bcc762c
Merge branch 'master' into rtl-labels-support
hodbauer Sep 28, 2017
7a0c0e3
Merge branch 'master' into rtl-labels-support
hodbauer Oct 2, 2017
3432f03
Merge branch 'master' into rtl-labels-support
hodbauer Oct 15, 2017
df1ddea
* add more tests for better code coverage
hodbauer Oct 16, 2017
80b370b
Merge remote-tracking branch 'origin/rtl-labels-support' into rtl-lab…
hodbauer Oct 16, 2017
0020337
Merge branch 'master' into rtl-labels-support
hodbauer Oct 19, 2017
abc7b9e
Merge branch 'master' into rtl-labels-support
mramato Oct 20, 2017
fac9512
Merge remote-tracking branch 'origin/master' into rtl-labels-support
hodbauer Oct 21, 2017
11b485e
Merge remote-tracking branch 'origin/master' into rtl-labels-support
hodbauer Oct 23, 2017
32c5657
declare textTypes of rightToLeft mechanism as object and not a function
hodbauer Oct 23, 2017
118abe4
manage rightToLeft as a static property of Label
hodbauer Oct 23, 2017
e109e98
fix tests by set text property also in the Label constructor, which c…
hodbauer Oct 24, 2017
c2aa9cc
Tweaks to right-to-left label handling.
mramato Oct 25, 2017
6bca30d
Tweak Sandcastle
mramato Oct 25, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Apps/Sandcastle/gallery/Labels.html
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,17 @@
});
}

function setRightToLeft() {
Sandcastle.declare(setRightToLeft);
viewer.entities.add({
position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
label : {
text : 'Master (אדון): Hello\nתלמיד (student): שלום',
rightToLeft : true
}
});
}

Sandcastle.addToolbarMenu([{
text : 'Add label',
onselect : function() {
Expand Down Expand Up @@ -157,6 +168,12 @@
scaleByDistance();
Sandcastle.highlight(scaleByDistance);
}
}, {
text : 'Set label with right-to-left language',
onselect : function() {
setRightToLeft();
Sandcastle.highlight(setRightToLeft);
}
}]);

Sandcastle.reset = function() {
Expand Down
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Change Log
==========
### 1.39 - 2017-11-01

* Added support for right-to-left languages in labels. [#5771](https://github.com/AnalyticalGraphicsInc/cesium/pull/5771)
* Added function that inserts missing namespace declarations into KML files. [#5860](https://github.com/AnalyticalGraphicsInc/cesium/pull/5860)
* Added support for the layer.json `parentUrl` property in `CesiumTerrainProvider` to allow for compositing of tilesets.
* Fixed a bug that caused KML ground overlays to appear distorted when rotation was applied. [#5914](https://github.com/AnalyticalGraphicsInc/cesium/issues/5914)
Expand Down
3 changes: 3 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
* [Jannes Bolling](https://github.com/jbo023)
* [Logilab](https://www.logilab.fr/)
* [Florent Cayré](https://github.com/fcayre/)
* [webiks](https://www.webiks.com)
* [Hod Bauer](https://github.com/hodbauer)
* [Yonatan Kra](https://github.com/yonatankra)
* [Novetta](http://www.novetta.com/)
* [Natanael Rivera](https://github.com/nrivera-Novetta/)
* [Justin Burr](https://github.com/jburr-nc/)
Expand Down
15 changes: 14 additions & 1 deletion Source/DataSources/LabelGraphics.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ define([
* @param {Property} [options.scaleByDistance] A {@link NearFarScalar} Property used to set scale based on distance from the camera.
* @param {Property} [options.heightReference=HeightReference.NONE] A Property specifying what the height is relative to.
* @param {Property} [options.distanceDisplayCondition] A Property specifying at what distance from the camera that this label will be displayed.
* @param {Property} [options.rightToLeft=false] A Property specifying if to modify text when there is possibly rightToLeft characters.
* @param {Property} [options.disableDepthTestDistance] A Property specifying the distance from the camera at which to disable the depth test to.
*
* @demo {@link http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Labels.html|Cesium Sandcastle Labels Demo}
Expand Down Expand Up @@ -94,6 +95,8 @@ define([
this._distanceDisplayConditionSubscription = undefined;
this._disableDepthTestDistance = undefined;
this._disableDepthTestDistanceSubscription = undefined;
this._rightToLeft = undefined;
this._rightToLeftSubscribtion = undefined;
this._definitionChanged = new Event();

this.merge(defaultValue(options, defaultValue.EMPTY_OBJECT));
Expand Down Expand Up @@ -319,7 +322,15 @@ define([
* @memberof LabelGraphics.prototype
* @type {Property}
*/
disableDepthTestDistance : createPropertyDescriptor('disableDepthTestDistance')
disableDepthTestDistance : createPropertyDescriptor('disableDepthTestDistance'),

/**
* Gets or sets the ability to modify text characters direction.
* @memberof LabelGraphics.prototype
* @type {Property}
* @default false
*/
rightToLeft: createPropertyDescriptor('rightToLeft')
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 basically an optimization, right? If the string doesn't contain RTL characters it is a no-op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is just an optimization, how do you feel about having a single static Label.enableRightToLeftDetection variable that an application sets once globally rather than having to set it on every label? It will save memory when there are many labels and I doubt it will have a performance impact in RTL apps since I would expect most of the labels to be RTL in that case. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds very interesting.
I'm not sure 100% how to implement it, but if it is the right way then I should do it as you offered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after a little bit of thinking I understand how to implement it, it will clean a lot of the work and it is nice.
my question is: how to declare the static variable so each application could manage it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just add a property directly on the Label object. Something like this should work.

   /**
     * Enabled detection and support of right to left text.
     *
     * @type {Boolean}
     * @default false
     *
     * @example
     * //Set once at the start of the application
     * Cesium.Label.enableRightToLeftDetection = true;
     * 
     * var myLabelEntity = viewer.entities.add({
     *   label: {
     *     id: 'my label',
     *     text: 'זה טקסט בעברית \n ועכשיו יורדים שורה'
     *   }
     * });
     */
    Label.enableRightToLeftDetection = false;

This will show up as static in the documentation and users just set Cesium.Label.enableRightToLeftDetection = true once in their app to use it. In your own code, you would just check Label.enableRightToLeftDetection directly as well.

Thanks.

});

/**
Expand Down Expand Up @@ -353,6 +364,7 @@ define([
result.scaleByDistance = this.scaleByDistance;
result.distanceDisplayCondition = this.distanceDisplayCondition;
result.disableDepthTestDistance = this.disableDepthTestDistance;
result.rightToLeft = this.rightToLeft;
return result;
};

Expand Down Expand Up @@ -390,6 +402,7 @@ define([
this.scaleByDistance = defaultValue(this.scaleByDistance, source.scaleByDistance);
this.distanceDisplayCondition = defaultValue(this.distanceDisplayCondition, source.distanceDisplayCondition);
this.disableDepthTestDistance = defaultValue(this.disableDepthTestDistance, source.disableDepthTestDistance);
this.rightToLeft = defaultValue(this.rightToLeft, source.rightToLeft);
};

return LabelGraphics;
Expand Down
2 changes: 2 additions & 0 deletions Source/DataSources/LabelVisualizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ define([
var defaultHorizontalOrigin = HorizontalOrigin.CENTER;
var defaultVerticalOrigin = VerticalOrigin.CENTER;
var defaultDisableDepthTestDistance = 0.0;
var defaultRightToLeft = false;

var position = new Cartesian3();
var fillColor = new Color();
Expand Down Expand Up @@ -165,6 +166,7 @@ define([
label.scaleByDistance = Property.getValueOrUndefined(labelGraphics._scaleByDistance, time, scaleByDistance);
label.distanceDisplayCondition = Property.getValueOrUndefined(labelGraphics._distanceDisplayCondition, time, distanceDisplayCondition);
label.disableDepthTestDistance = Property.getValueOrDefault(labelGraphics._disableDepthTestDistance, time, defaultDisableDepthTestDistance);
label.rightToLeft = Property.getValueOrDefault(labelGraphics._rightToLeft, time, defaultRightToLeft);
}
return true;
};
Expand Down
234 changes: 233 additions & 1 deletion Source/Scene/Label.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ define([
'../Core/defineProperties',
'../Core/DeveloperError',
'../Core/DistanceDisplayCondition',
'../Core/freezeObject',
'../Core/NearFarScalar',
'./Billboard',
'./HeightReference',
Expand All @@ -24,6 +25,7 @@ define([
defineProperties,
DeveloperError,
DistanceDisplayCondition,
freezeObject,
NearFarScalar,
Billboard,
HeightReference,
Expand Down Expand Up @@ -110,7 +112,6 @@ define([
distanceDisplayCondition = DistanceDisplayCondition.clone(distanceDisplayCondition);
}

this._text = defaultValue(options.text, '');
this._show = defaultValue(options.show, true);
this._font = defaultValue(options.font, '30px sans-serif');
this._fillColor = Color.clone(defaultValue(options.fillColor, Color.WHITE));
Expand Down Expand Up @@ -147,6 +148,9 @@ define([

this._clusterShow = true;

this._rightToLeft = defaultValue(options.rightToLeft, false);
this.text = defaultValue(options.text, '');

this._updateClamping();
}

Expand Down Expand Up @@ -279,6 +283,15 @@ define([
}
//>>includeEnd('debug');

if (this.rightToLeft) {
if (this._originalValue === value) {
value = this._text;
return;
}
this._originalValue = value;
value = reverseRtl(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that we are mutating the value supplied by the user so that the getter is actually the reversed text. I think this is undesired behavior. With few exceptions, a simple setter/getter should always return the value assigned to it. I can submit a change to prevent this, but if you feel strongly about it, please let me know.

}

if (this._text !== value) {
this._text = value;
rebindAllGlyphs(this);
Expand Down Expand Up @@ -1030,6 +1043,46 @@ define([
}
}
}
},

/**
* Determines whether or not run the reverseRtl algorithm on the text of the label
* @memberof Label.prototype
* @type {Boolean}
* @default false
*
* @example
* // Example 1.
* // Set a label's rightToLeft during init
* var myLabelEntity = viewer.entities.add({
* label: {
* id: 'my label',
* text: 'זה טקסט בעברית \n ועכשיו יורדים שורה',
* rightToLeft: true
* }
* });
*
* @example
* // Example 2.
* var myLabelEntity = viewer.entities.add({
* label: {
* id: 'my label',
* text: 'English text'
* }
* });
* // Set a label's rightToLeft after init
* myLabelEntity.rightToLeft = true;
* myLabelEntity.text = 'טקסט חדש';
*/
rightToLeft : {
get : function() {
return this._rightToLeft;
},
set : function(value) {
if (this._rightToLeft !== value) {
this._rightToLeft = value;
}
}
}
});

Expand Down Expand Up @@ -1196,5 +1249,184 @@ define([
return false;
};

function declareTypes() {
var TextTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just declare this at the top of the file instead of creating it every time. Then just use TextTypes everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mramato If I understand you, you want me to change the code to something like:

var TextTypes = freezeObject({
    LTR : 0,
    RTL : 1,
    WEAK : 2,
    BRACKETS : 3
});
return TextTypes;

and put it inside the define near to the glyph functions as is (without a function scope)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, though you won't need the return statement. You can just put the declaration at the top of the file (above rebindAllGlyphs), it will be scoped to Label.js.

LTR : 0,
RTL : 1,
WEAK : 2,
BRACKETS : 3
};
return freezeObject(TextTypes);
}

function convertTextToTypes(text, rtlDir, rtlChars) {
var ltrChars = /[a-zA-Z0-9]/;
var bracketsChars = /[()[\]{}<>]/;
var parsedText = [];
var word = '';
var types = declareTypes();
var lastType = rtlDir ? types.RTL : types.LTR;
var currentType = '';
var textLength = text.length;
for (var textIndex = 0; textIndex < textLength; ++textIndex) {
var character = text.charAt(textIndex);
if (rtlChars.test(character)) {
currentType = types.RTL;
}
else if (ltrChars.test(character)) {
currentType = types.LTR;
}
else if (bracketsChars.test(character)) {
currentType = types.BRACKETS;
}
else {
currentType = types.WEAK;
}

if (textIndex === 0) {
lastType = currentType;
}

if (lastType === currentType && currentType !== types.BRACKETS) {
word += character;
}
else {
if (word !== '') {
parsedText.push({Type : lastType, Word : word});
}
lastType = currentType;
word = character;
}
}
parsedText.push({Type : currentType, Word : word});
return parsedText;
}

function reverseWord(word) {
return word.split('').reverse().join('');
}

function spliceWord(result, pointer, word) {
return result.slice(0, pointer) + word + result.slice(pointer);
}

function reverseBrackets(bracket) {
switch(bracket) {
case '(':
return ')';
case ')':
return '(';
case '[':
return ']';
case ']':
return '[';
case '{':
return '}';
case '}':
return '{';
case '<':
return '>';
case '>':
return '<';
}
}

/**
*
* @param {String} value the text to parse and reorder
* @returns {String} the text as rightToLeft direction
* @private
*/
function reverseRtl(value) {
var rtlChars = /[\u05D0-\u05EA]/;
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I'm understanding this code correctly, this regex detects whether the letter is RTL (in this case specifically Hebrew) but otherwise the rest of the code is the same for other languages? So if we update this regex for other known RTL character sets (perhaps thise discussed here and here Is that correct? If so that's awesome and we can easily update this to handle the other languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but otherwise the rest of the code is the same for other languages?

yes. it only necessary when writing a RTL language in Cesium.

we can easily update this to handle the other languages.

basically yes, I do not do that, because i cannot read other RTL languages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, we can merge this with just Hebrew and then I'll open a second PR with an updated regex to support additional languages.

var texts = value.split('\n');
var result = '';
for (var i = 0; i < texts.length; i++) {
var text = texts[i];
var rtlDir = rtlChars.test(text.charAt(0));
var parsedText = convertTextToTypes(text, rtlDir, rtlChars);

var types = declareTypes();

var splicePointer = 0;
var line = '';
for (var wordIndex = 0; wordIndex < parsedText.length; ++wordIndex) {
var subText = parsedText[wordIndex];
var reverse = subText.Type === types.BRACKETS ? reverseBrackets(subText.Word) : subText.Word;
if (rtlDir) {
if (subText.Type === types.RTL) {
line = reverseWord(subText.Word) + line;
splicePointer = 0;
}
else if (subText.Type === types.LTR) {
line = spliceWord(line, splicePointer, subText.Word);
splicePointer += subText.Word.length;
}
else if (subText.Type === types.WEAK || subText.Type === types.BRACKETS) {
if (subText.Type === types.WEAK && parsedText[wordIndex - 1].Type === types.BRACKETS) {
line = reverseWord(subText.Word) + line;
}
else if (parsedText[wordIndex - 1].Type === types.RTL) {
line = reverse + line;
splicePointer = 0;
}
else if (parsedText.length > wordIndex + 1) {
if (parsedText[wordIndex + 1].Type === types.RTL) {
line = reverse + line;
splicePointer = 0;
}
else {
line = spliceWord(line, splicePointer, subText.Word);
splicePointer += subText.Word.length;
}
}
else {
line = spliceWord(line, 0, reverse);
}
}
}
else if (subText.Type === types.RTL) {
line = spliceWord(line, splicePointer, reverseWord(subText.Word));
}
else if (subText.Type === types.LTR) {
line += subText.Word;
splicePointer = line.length;
}
else if (subText.Type === types.WEAK || subText.Type === types.BRACKETS) {
if (wordIndex > 0) {
if (parsedText[wordIndex - 1].Type === types.RTL) {
if (parsedText.length > wordIndex + 1) {
if (parsedText[wordIndex + 1].Type === types.RTL) {
line = spliceWord(line, splicePointer, reverse);
}
else {
line += subText.Word;
splicePointer = line.length;
}
}
else {
line += subText.Word;
}
}
else {
line += subText.Word;
splicePointer = line.length;
}
}
else {
line += subText.Word;
splicePointer = line.length;
}
}
}

result += line;
if (i < texts.length - 1) {
result += '\n';
}
}
return result;
}

return Label;
});
Loading