Skip to content

DM-7665: fixed the region parsing issue regarding region used separators contained in some property values #191

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 23, 2016
Merged

DM-7665: fixed the region parsing issue regarding region used separators contained in some property values #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
353761c
DM-7665: fixed the region parsing issue regarding region used separat…
Sep 20, 2016
ca9f1f1
fixed a typo in 'refactorRegionValueUnit' of file 'RegionFactory.js'
Sep 23, 2016
501f55d
move function 'getStringUnits' out of some function which calls reduc…
Sep 23, 2016
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
16 changes: 15 additions & 1 deletion src/firefly/html/demo/ffapi-highlevel-test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,19 @@
'image; box 280 200 72 72 0 # color=cyan text={right syntax}'
];

/*
window.regionWithSpecialChars = [
'circle 202.4844693750341 47.23118060364845 13.9268922364868 # color=blue text={j2000 ; circle on ; J2000}',
'J2000;box 202.4844693750341 47.23118060364845 0.0069268922364 0.0069268922364 0 # color=red text={box on # J2000 with radius ## 0.00198268922364868}',
'circle 100.4844693750341 147.23118060364845 0.0239268922364868 # color=purple text="j2000 ; circle on # J2000"'
];
*/
window.regionWithSpecialChars = [
'circle 202.4844693750341 47.23118060364845 13.9268922364868 # color=cyan text={physical;circle ;; color=cyan}',
'J2000;box 202.4844693750341 47.23118060364845 0.0069268922364 0.0069268922364 0 # color=red text={box on # J2000 with size ## 0.0069268922364}',
'image;circle 100.4844693750341 147.23118060364845 10.0239268922364868 # color=#B8E986 text="image ; circle # color=#B8E986"'
];


window.moreRegionAry = [
'image;ellipse 100 100 20p 40p 30p 60p 40p 80p 20 # color=green text={ellipseannulus 2}',
Expand Down Expand Up @@ -359,7 +372,8 @@
document.getElementById('createRegionId').value = layerId;

}
document.getElementById('regionLayerContent').value = window.smallregion.join('\n');
document.getElementById('regionLayerContent').value = window.regionWithSpecialChars.join('\n');

/*
if (window.regionId === 0) {
document.getElementById('regionLayerContent').value = window.emptyRegion;
Expand Down
110 changes: 83 additions & 27 deletions src/firefly/java/edu/caltech/ipac/util/RegionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Arrays;
import java.util.stream.Collectors;


/**
* @author Trey Roby
Expand All @@ -62,9 +65,11 @@ public static ParseRet processInput(LineGetter lineGetter) {
try {
List<RegionFileElement> resultList= RegionFactory.parsePart(s, coordSys, g, allowHeader);
if (allowHeader) {
g= RegionFactory.getGlobal(resultList, g);
coordSys= RegionFactory.getCsys(resultList, coordSys);
g = RegionFactory.getGlobal(resultList, g);
}
coordSys= RegionFactory.getCsys(resultList, coordSys); //current coordinate applies to next region
//which has no coordinate set in front.

if (RegionFactory.containsRegion(resultList)) {
allowHeader= false;
RegionFactory.addAllRegions(resultList, regList);
Expand All @@ -78,9 +83,54 @@ public static ParseRet processInput(LineGetter lineGetter) {
return new ParseRet(regList,msgList);
}

private static Boolean isInText;
private static String crtSeg;
private static int dLimitIdx;
private static List<String> oneLineUnits;
private static String[] dLeft = {"{", "\"", "\'"};
private static String[] dRight = {"}", "\"", "\'"};


private static String[] splitStringBySeparator(String inString, String sep) {
List<String> charsInString = Arrays.asList(inString.split(""));

dLimitIdx = -1;
isInText = false;
crtSeg = "";
oneLineUnits = new ArrayList<String>();

charsInString
.stream()
.forEach( (c) -> {
if (c.equals(sep)) {
if (isInText) {
crtSeg += c;
} else {
if (crtSeg.length() > 0) {
oneLineUnits.add(crtSeg);
crtSeg = "";
}
}
} else {
if (isInText) {
if (c.equals(dRight[dLimitIdx])) {
isInText = false;
}
} else {
dLimitIdx = Arrays.asList(dLeft).indexOf(c.toString());
isInText = dLimitIdx >= 0;
}
crtSeg += c;

}
});


if (crtSeg.length() > 0) {
oneLineUnits.add(crtSeg);
crtSeg = "";
}
return oneLineUnits.toArray(new String[oneLineUnits.size()]);
}

public static List<RegionFileElement> parsePart(String inString) throws RegParseException {
return parsePart(inString, RegionCsys.IMAGE, new Global(new RegionOptions()),false);
Expand All @@ -91,7 +141,7 @@ public static List<RegionFileElement> parsePart(String inString, RegionCsys coor

if (coordSys==null) coordSys= RegionCsys.IMAGE;
RegionOptions globalOps= global!=null ? global.getOptions() : null;
String sAry[]= inString.split(";");
String sAry[]= splitStringBySeparator(inString, ";"); // inString.split(";");
List<RegionFileElement> retList= new ArrayList<RegionFileElement>(4);

for (String virtualLine: sAry) {
Expand All @@ -116,7 +166,7 @@ public static List<RegionFileElement> parsePart(String inString, RegionCsys coor
Region region = null;
String region_type= null;
boolean include= true;
boolean isWorldCoord = false;


try
{
Expand All @@ -129,9 +179,7 @@ public static List<RegionFileElement> parsePart(String inString, RegionCsys coor
}
if (isCoordSys(lineBegin)) {
coordSys = getCoordSys(lineBegin);
if (allowHeader) {
retList.add(coordSys);
}
retList.add(coordSys); // a new coordinate setting
continue;
}
else {
Expand All @@ -144,8 +192,9 @@ public static List<RegionFileElement> parsePart(String inString, RegionCsys coor
}
}
}

boolean isWorldCoord = isWorldCoords(coordSys);
if (st.hasMoreToken()) {
isWorldCoord = isWorldCoords(coordSys);

if (region_type.equals("vector") ||
region_type.equals("ruler") ||
Expand Down Expand Up @@ -589,6 +638,7 @@ private static RegOpsParseRet parseRegionOptionPlus(String s, RegionOptions fall
StringTokenizer st= new StringTokenizer(s, " ");
String workStr= s;
retval.ops.setInclude(include);

while (st.hasMoreToken())
{
String token = st.nextToken();
Expand Down Expand Up @@ -618,39 +668,45 @@ else if (token.toLowerCase().startsWith("offsety=")) {
retval.ops.setOffsetY(parseInt(token, 0));
}
else if (token.toLowerCase().startsWith("text=")) {
int endIdx= 5;
int endIdx= 0;
String textPlus= workStr.substring(workStr.indexOf("text=")+5);
String start= getStartCharOfString(textPlus);
if (start!=null && textPlus.startsWith(start)) {
String end= (start.equals("{")) ? "}" : start;
textPlus= textPlus.substring(1);
endIdx= textPlus.indexOf(end);
if (endIdx>-1) {
String textString= textPlus.substring(0,endIdx);
endIdx= textPlus.indexOf(end, 1);
if (endIdx > 0) { // end is found
String textString= textPlus.substring(1,endIdx);
retval.ops.setText(textString);
endIdx++;
} else { // end is not found
endIdx = 1;
}
else {
endIdx= 5;
}
}
workStr= workStr.substring(endIdx+1).trim();

workStr = textPlus.substring(endIdx).trim();
if (workStr.length() == 0) {
break;
}
st= new StringTokenizer(workStr, " ");
}
else if (token.toLowerCase().startsWith("font=")) {
int endIdx= 5;
int endIdx= 0;
String textPlus= workStr.substring(workStr.indexOf("font=")+5);
if (textPlus.startsWith("\"")) {
textPlus= textPlus.substring(1);
endIdx= textPlus.indexOf("\"");
if (endIdx>-1) {
String fontString= textPlus.substring(0,endIdx);
endIdx = textPlus.indexOf("\"", 1);
if (endIdx > 0) {
String fontString = textPlus.substring(1, endIdx);
retval.ops.setFont(new RegionFont(fontString));
}
else {
endIdx= 5;
endIdx++;
} else {
endIdx = 1;
}
}
workStr= workStr.substring(endIdx+1).trim();
workStr = textPlus.substring(endIdx).trim();
if (workStr.length() == 0) {
break;
}

st= new StringTokenizer(workStr, " ");
}
else if (token.toLowerCase().startsWith("point=")) {
Expand Down
123 changes: 87 additions & 36 deletions src/firefly/js/visualize/region/RegionFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,11 @@ export class RegionFactory {
* @returns {null}
*/
static parseRegionJson(regionData) {

var globalOptions = Object.assign({}, makeRegionOptions({[regionPropsList.COORD]: defaultCoord}));

return regionData? regionData.reduce ( (prev, region, index) => {
const rg = RegionFactory.parsePart(region, index);
const rg = RegionFactory.parsePart(region, index, globalOptions);

if (rg) { // skip comment line and no good line
if (outputError(rg, region) === 0) prev.push(rg);
Expand All @@ -74,37 +77,80 @@ export class RegionFactory {
* @returns {array} an array of Region object
*/
static parseRegionDS9(regionData, bAllowHeader = true, stopAt) {
var regionLines = regionData.reduce ((prev, oneLine) => {
var resRegions = oneLine.split(';');

var crtCsys = null;
var preCsys = null;
var lastIdx = resRegions.length - 1;
const sep = ';';
const dLeft = ['{', '"', '\''];
const dRight = ['}', '"', '\''];

// split each string into a set of units which are separated by ';' with the consideration that the semicolon
// contained in the text or tag string is not counted as separator.
// Each unit could contain a coordinate string or a region description string

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 a lot of code for a reduce function. I think you should break it up so that it is a little easier to follow. I see that the old approach was a reduce as well but it was not as big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment.

var getStringUnits = (oneLine) => {
var isInText = false; // check if sep is inside a text string enclosed by a pair of delimiter
var crtSeg = ''; // current string unit
var dLimitIdx = -1;
var isLeftDelimiter = (c) => dLeft.findIndex((t) => (t === c));

var addNewUnitTo = (prev) => {
if (crtSeg.length > 0) {
prev.push(crtSeg.slice(0));
crtSeg = '';
}
};

// combine coordinate and region definition of the same line
resRegions.forEach( (crtVal, index) => {
crtCsys = getRegionCoordSys(crtVal);
var incUnit = (v) => {
crtSeg = crtSeg.concat(v);
};

if (crtCsys !== RegionCsys.UNDEFINED) { // coordinate string
if (index === lastIdx) {
prev = [...prev, crtVal];
} else {
preCsys = crtCsys;
var lUnits = oneLine.split('').reduce((oneLineUnits, v) => {
if (v === sep) {
isInText ? incUnit(v) : addNewUnitTo(oneLineUnits);
} else {
if (isInText) {
if (v === dRight[dLimitIdx]) {
isInText = false;
}
} else {
// region string or the last one is coordinate
if (!preCsys) {
prev = [...prev, crtVal]; // no coordinate prior to current string
} else {
prev = [...prev, `${resRegions[index - 1]};${crtVal}`]; // combine with coordinate
}
preCsys = null;
dLimitIdx = isLeftDelimiter(v);
isInText = dLimitIdx >= 0;
}
});
incUnit(v);
}
return oneLineUnits;
}, []);

return prev;
if (crtSeg.length > 0) {
lUnits.push(crtSeg.slice());
}
return lUnits;
};

// collect region lines and each line may contain coordinate, region description or both
var regionLines = regionData.reduce( (rLines, oneLine) => {
var units = getStringUnits(oneLine);
var lastIdx = units.length - 1;
var preCsys = null; // coordinate status of previous unit

// combine the coordinate unit and region description unit from the same line into one string
var lines = units.reduce((unitSet, crtVal, index) => {
var crtCsys = getRegionCoordSys(crtVal);

if (crtCsys !== RegionCsys.UNDEFINED) { // current string is coordinate unit
if (index === lastIdx) {
unitSet = [...unitSet, crtVal];
} else {
preCsys = crtCsys;
}
} else {
unitSet = [...unitSet, (preCsys ? `${units[index - 1]};${crtVal}` : crtVal)];
preCsys = null;
}
return unitSet;
}, []);

return [...rLines,...lines];
}, []);

var globalOptions = Object.assign({}, makeRegionOptions({[regionPropsList.COORD]: defaultCoord}));

return regionLines.reduce ( (prev, region, index) => {
Expand All @@ -131,7 +177,7 @@ export class RegionFactory {
}
/**
* parsePart parses the region data of JSON result (one item from RegionData array)
* @param regionStr
* @param regionStr each string is either like coordinate;region_description or region_descrption
* @param index line number shown in message
* @param globalOptions
* @param bAllowHeader
Expand Down Expand Up @@ -176,23 +222,28 @@ export class RegionFactory {

// check if coordination system (from RegionDS9)
tmpAry = regionStr.split(';');
var csys = getRegionCoordSys(tmpAry[0]); // test the split first string

if (tmpAry.length <= 1) {
var csys = getRegionCoordSys(tmpAry[0]);
if (csys !== RegionCsys.UNDEFINED) {
if (globalOptions) globalOptions.coordSys = tmpAry[0].toLowerCase();

if (csys !== RegionCsys.UNDEFINED && globalOptions) {
globalOptions.coordSys = tmpAry[0].toLowerCase();
if (tmpAry.length <= 1) { // pure coordinate string
return null;
}
if (tmpAry.length > 2) { // description contain ';'
tmpAry = [tmpAry[0], tmpAry.slice(1).join(';')]; // coordinate and description
}
} else if (tmpAry.length >= 2) { // no coordinate and description contain ';'
tmpAry = [tmpAry.join(';')];
}

// check if string contains coordinate, description and property portions
// -- regionCoord --
if (tmpAry.length > 1) {

if (tmpAry.length > 1) { // coordinate and description
regionCoord = tmpAry[0].trim();
tmpAry.shift();
bCoord = true; // coordinate is defined in this line
} else {
tmpAry.shift(); // remove the coordinate element

bCoord = true;
} else { // description only
// default coordinate is PHYSICAL in case not specified
regionCoord = globalOptions && has(globalOptions, regionPropsList.COORD) ?
globalOptions[regionPropsList.COORD] : defaultCoord;
Expand Down Expand Up @@ -630,7 +681,7 @@ export class RegionFactory {
(c !== RegionCsys.DETECTOR) );

var makePt = (vx, vy, cs) => {
if (vx.unit === RegionValueUnit.IMAGE_PIXEL) {
if (vx.unit === RegionValueUnit.IMAGE_PIXEL || vx.unit === RegionValueUnit.SCREEN_PIXEL) {
return makeImagePt(vx.value, vy.value);
} else {
return makeWorldPt(vx.value, vy.value, this.parse_coordinate(cs));
Expand Down