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 13 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 setRtl() {
Sandcastle.declare(setRtl);
viewer.entities.add({
position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
label : {
text : 'שלום',
rtl : true
}
});
}

Sandcastle.addToolbarMenu([{
text : 'Add label',
onselect : function() {
Expand Down Expand Up @@ -157,6 +168,12 @@
scaleByDistance();
Sandcastle.highlight(scaleByDistance);
}
}, {
text : 'Set rtl',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you be more descriptive for the button label? Maybe Set label with right-to-left language

onselect : function() {
setRtl();
Sandcastle.highlight(setRtl);
}
}]);

Sandcastle.reset = function() {
Expand Down
2 changes: 1 addition & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Change Log
==========

### 1.37 - 2017-09-01

* Added suppport for RTL labels
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a little more descriptive, maybe Added support for right-to-left languages in labels

* Fixed `replaceState` bug that was causing the `CesiumViewer` demo application to crash in Safari and iOS
* Fixed issue where `Model` and `BillboardCollection` would throw an error if the globe is undefined [#5638](https://github.com/AnalyticalGraphicsInc/cesium/issues/5638)
* Fixed issue where the `Model` glTF cache loses reference to the model's buffer data. [#5720](https://github.com/AnalyticalGraphicsInc/cesium/issues/5720)
Expand Down
4 changes: 4 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
* [Jason Crow](https://github.com/jason-crow)
* [Flightradar24 AB](https://www.flightradar24.com)
* [Aleksei Kalmykov](https://github.com/kalmykov)
* [webiks](https://www.webiks.com)
* [Hod Bauer](https://github.com/hodbauer)
* [Yonatan Kra](https://github.com/yonatankra)

## [Individual CLA](Documentation/Contributors/CLAs/individual-cla-agi-v1.0.txt)
* [Victor Berchet](https://github.com/vicb)
Expand Down Expand Up @@ -152,4 +155,5 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
* [Rishabh Shah](https://github.com/rms13)
* [Rudraksha Shah](https://github.com/Rudraksha20)
* [Cody Guldner](https://github.com/burn123)
* [Yonatan Kra](https://github.com/yonatankra)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to add yourself to this section as well as the webiks section above?


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.rtl=false] A Property specifying if to modify text when there is possibly rtl characters.
*
* @demo {@link http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Labels.html|Cesium Sandcastle Labels Demo}
*/
Expand Down Expand Up @@ -93,6 +94,8 @@ define([
this._distanceDisplayConditionSubscription = undefined;
this._disableDepthTestDistance = undefined;
this._disableDepthTestDistanceSubscription = undefined;
this._rtl = undefined;
this._rtlSubscribtion = undefined;
this._definitionChanged = new Event();

this.merge(defaultValue(options, defaultValue.EMPTY_OBJECT));
Expand Down Expand Up @@ -318,7 +321,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
*/
rtl: createPropertyDescriptor('rtl')
});

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

Expand Down Expand Up @@ -389,6 +401,7 @@ define([
this.scaleByDistance = defaultValue(this.scaleByDistance, source.scaleByDistance);
this.distanceDisplayCondition = defaultValue(this.distanceDisplayCondition, source.distanceDisplayCondition);
this.disableDepthTestDistance = defaultValue(this.disableDepthTestDistance, source.disableDepthTestDistance);
this.rtl = defaultValue(this.rtl, source.rtl);
};

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 defaultRtl = 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.rtl = Property.getValueOrDefault(labelGraphics._rtl, time, defaultRtl);
}
return true;
};
Expand Down
216 changes: 216 additions & 0 deletions 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 @@ -147,6 +149,9 @@ define([

this._clusterShow = true;

this._rtl = defaultValue(options.rtl, false);
this.text = defaultValue(options.text, '');

this._updateClamping();
}

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

if (this.rtl) {
if (this._originalValue === value) {
value = this._text;
return;
}
this._originalValue = value;
value = this.reverseRtl(value);
}

if (this._text !== value) {
this._text = value;
rebindAllGlyphs(this);
Expand Down Expand Up @@ -1030,6 +1044,42 @@ 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 rtl during init
* var myLabelEntity = viewer.entities.add({
* id: 'my label',
* text: 'זה טקסט בעברית \n ועכשיו יורדים שורה',
* rtl: true
* });
*
* @example
* // Example 2.
* var myLabelEntity = viewer.entities.add({
* id: 'my label',
* text: 'English text'
* });
* // Set a label's rtl after init
* myLabelEntity.rtl = true;
* myLabelEntity.text = 'טקסט חדש'
*/
rtl : {
get : function() {
return this._rtl;
},
set : function(value) {
if (this._rtl !== value) {
this._rtl = 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 think you should call reverseRtl here in addition to when setting the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ggetz for your comments. I will fix them after the weekend.
I have a specific question about this comment. Why to call reverseRtl here? If the text is written in rtl language, I don't see a reason to change the rtl flag while using it, the algorithm will work good even for ltr (left to right) languages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @hodbauer!

If I understand correctly, if rtl is set to false, and you update text to an rtl language, then set rtl to true, it won't display correctly:

label.text = 'שלום';
label.rtl = true;

That may be confusing to people using this feature.

}
}
}
});

Expand Down Expand Up @@ -1196,5 +1246,171 @@ 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 (lastType === currentType && currentType !== types.BRACKETS) {
word += character;
}
else {
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} text the text to parse and reorder
* @returns {String} the text as rtl direction
*/
Label.prototype.reverseRtl = function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should be private, I don't think anyone should be calling this directly.

var rtlChars = /[א-ת]/;
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;
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) {
result = reverseWord(subText.Word) + result;
splicePointer = 0;
}
else if (subText.Type === types.LTR) {
result = spliceWord(result,splicePointer, subText.Word);
splicePointer += subText.Word.length;
}
else if (subText.Type === types.WEAK || subText.Type ===types.BRACKETS) {
if (parsedText[wordIndex -1].Type === types.RTL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after the operator here and throughout.

result = reverse + result;
splicePointer = 0;
}
else if (parsedText.length > wordIndex +1) {
if (parsedText[wordIndex +1].Type === types.RTL) {
result = reverse + result;
splicePointer = 0;
}
else {
result = spliceWord(result,splicePointer, subText.Word);
splicePointer += subText.Word.length;
}
}
else {
result = spliceWord(result,splicePointer, subText.Word);
}
}
}
else if (subText.Type === types.RTL) {
result = spliceWord(result, splicePointer, reverseWord(subText.Word));
}
else if (subText.Type === types.LTR) {
result += subText.Word;
splicePointer = result.length;
}
else if (subText.Type === types.WEAK || subText.Type ===types.BRACKETS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing after ===

Copy link
Contributor

Choose a reason for hiding this comment

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

and on line 1354

if (wordIndex > 0) {
if (parsedText[wordIndex -1].Type === types.RTL) {
if (parsedText.length > wordIndex +1) {
if (parsedText[wordIndex +1].Type === types.LTR) {
result += subText.Word;
splicePointer = result.length;
}
else if (parsedText[wordIndex +1].Type === types.RTL) {
result = spliceWord(result, splicePointer, reverse);
}
}
else {
result += subText.Word;
}
}
else {
result += subText.Word;
splicePointer = result.length;
}
}
else {
result += subText.Word;
splicePointer = result.length;
}
}
}
if (i < texts.length - 1) {
result += '\n';
}
}
return result;
};

return Label;
});
Loading