Skip to content

Commit

Permalink
CLDR-14877 Coverage Progress Bars (#1469) (#1502)
Browse files Browse the repository at this point in the history 
-Remove superfluous path exclusion checks for altProposed, /references

-Refactor and format DataSection.ensureComplete to clarify it only applies to timeZoneNames paths

-Use ph.shouldHide instead of SurveyToolStatus.DEPRECATED/HIDE where the distinction is unneeded

-Simplify code that had impossible conditions in TestPathHeader

-Fix deprecation warnings for SurveyLog, setCldrFileToCheck

-Comments, clean-up

(cherry picked from commit 56ca5d5)

Co-authored-by: Tom Bishop <[email protected]>
  • Loading branch information
@pedberg-icu @btangmu
pedberg-icu and btangmu authored Sep 16, 2021
1 parent 7e6ef5f commit a62ef81
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 175 deletions.
199 changes: 100 additions & 99 deletions tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataSection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.logging.Logger;
import java.util.regex.Pattern;

import org.json.JSONArray;
Expand Down Expand Up @@ -76,6 +77,8 @@
*/
public class DataSection implements JSONString {

private final static Logger logger = SurveyLog.forClass(DataSection.class);

/*
* For debugging only (so far), setting USE_CANDIDATE_HISTORY to true causes
* a "history" to be constructed and passed to the client for each CandidateItem,
Expand Down Expand Up @@ -212,9 +215,8 @@ public String getProcessedValue() {
try {
return getProcessor().processForDisplay(xpath, rawValue);
} catch (Throwable t) {
if (SurveyLog.DEBUG) {
SurveyLog.logException(t, "While processing " + xpath + ":" + rawValue);
}
String msg = "getProcessedValue, while processing " + xpath + ":" + rawValue;
logger.log(java.util.logging.Level.FINE, msg, t);
return rawValue;
}
}
Expand Down Expand Up @@ -1773,119 +1775,119 @@ public DisplaySet createDisplaySet(SortMode sortMode, XPathMatcher matcher) {
}

/**
* Makes sure this DataSection contains the rows we'd like to see.
* Makes sure this DataSection contains the rows we'd like to see related to timeZoneNames
*
* Called only by DataSection.make, only when pageId == null
*
* TODO: explain this mechanism and why it's limited to DataSection, not shared with Dashboard,
* CLDRFile, or any other module. Is it in any way related to CLDRFile.getRawExtraPathsPrivate?
*/
private void ensureComplete(CLDRFile ourSrc, TestResultBundle checkCldr) {

if (!xpathPrefix.startsWith("//ldml/dates/timeZoneNames")) {
return;
}
STFactory stf = sm.getSTFactory();

if (xpathPrefix.startsWith("//ldml/dates/timeZoneNames")) {
// work on zones
boolean isMetazones = xpathPrefix.startsWith("//ldml/dates/timeZoneNames/metazone");
boolean isSingleXPath = false;
// Make sure the DataSection contains the rows we'd like to see.
// regular zone

Set<String> zoneIterator;
String podBase = xpathPrefix;

if (isMetazones) {
if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) {
String[] pieces = xpathPrefix.split(CONTINENT_DIVIDER, 2);
xpathPrefix = pieces[0];
zoneIterator = sm.getMetazones(pieces[1]);
} else { // This is just a single metazone from a zoom-in
Set<String> singleZone = new HashSet<>();
XPathParts xpp = XPathParts.getFrozenInstance(xpathPrefix);
String singleMetazoneName = xpp.findAttributeValue("metazone", "type");
if (singleMetazoneName == null) {
throw new NullPointerException("singleMetazoneName is null for xpp:" + xpathPrefix);
}
singleZone.add(singleMetazoneName);
zoneIterator = singleZone;

isSingleXPath = true;
}
podBase = "//ldml/dates/timeZoneNames/metazone";
} else {
if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) {
throw new InternalError("Error: CONTINENT_DIVIDER found on non-metazone xpath " + xpathPrefix);
// work on zones
boolean isMetazones = xpathPrefix.startsWith("//ldml/dates/timeZoneNames/metazone");
boolean isSingleXPath = false;
// Make sure the DataSection contains the rows we'd like to see.
// regular zone

Set<String> zoneIterator;
String podBase = xpathPrefix;

if (isMetazones) {
if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) {
String[] pieces = xpathPrefix.split(CONTINENT_DIVIDER, 2);
xpathPrefix = pieces[0];
zoneIterator = sm.getMetazones(pieces[1]);
} else { // This is just a single metazone from a zoom-in
Set<String> singleZone = new HashSet<>();
XPathParts xpp = XPathParts.getFrozenInstance(xpathPrefix);
String singleMetazoneName = xpp.findAttributeValue("metazone", "type");
if (singleMetazoneName == null) {
throw new NullPointerException("singleMetazoneName is null for xpp:" + xpathPrefix);
}
zoneIterator = StandardCodes.make().getGoodAvailableCodes("tzid");
podBase = "//ldml/dates/timeZoneNames/zone";
}
if (!isSingleXPath && xpathPrefix.contains("@type")) {
singleZone.add(singleMetazoneName);
zoneIterator = singleZone;

isSingleXPath = true;
}
podBase = "//ldml/dates/timeZoneNames/metazone";
} else {
if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) {
throw new InternalError("Error: CONTINENT_DIVIDER found on non-metazone xpath " + xpathPrefix);
}
zoneIterator = StandardCodes.make().getGoodAvailableCodes("tzid");
podBase = "//ldml/dates/timeZoneNames/zone";
}
if (!isSingleXPath && xpathPrefix.contains("@type")) {
isSingleXPath = true;
}

final String tzsuffs[] = {
"/exemplarCity" };
final String mzsuffs[] = { "/long/generic", "/long/daylight", "/long/standard", "/short/generic", "/short/daylight",
"/short/standard" };
final String tzsuffs[] = {
"/exemplarCity" };
final String mzsuffs[] = { "/long/generic", "/long/daylight", "/long/standard", "/short/generic", "/short/daylight",
"/short/standard" };

String suffs[];
if (isMetazones) {
suffs = mzsuffs;
} else {
suffs = tzsuffs;
String suffs[];
if (isMetazones) {
suffs = mzsuffs;
} else {
suffs = tzsuffs;
}

for (String zone : zoneIterator) {
if (zone == null) {
throw new NullPointerException("zoneIterator.next() returned null! zoneIterator.size: " + zoneIterator.size()
+ ", isEmpty: " + zoneIterator.isEmpty());
}
/** some compatibility **/
String ourSuffix = "[@type=\"" + zone + "\"]";

CLDRFile resolvedFile = ourSrc;
for (int i = 0; i < suffs.length; i++) {
String suff = suffs[i];

for (String zone : zoneIterator) {
if (zone == null) {
throw new NullPointerException("zoneIterator.next() returned null! zoneIterator.size: " + zoneIterator.size()
+ ", isEmpty: " + zoneIterator.isEmpty());
if (isSingleXPath && !xpathPrefix.contains(suff)) {
continue; // Try not to add paths that won't be shown.
}
/** some compatibility **/
String ourSuffix = "[@type=\"" + zone + "\"]";

for (int i = 0; i < suffs.length; i++) {
String suff = suffs[i];
// synthesize a new row..
String base_xpath_string = podBase + ourSuffix + suff;

if (isSingleXPath && !xpathPrefix.contains(suff)) {
continue; // Try not to add paths that won't be shown.
}
// synthesize a new row..
String base_xpath_string = podBase + ourSuffix + suff;
PathHeader ph = stf.getPathHeader(base_xpath_string);
if (ph == null || ph.shouldHide()) {
continue;
}

PathHeader ph = stf.getPathHeader(base_xpath_string);
if (ph == null || ph.shouldHide()) {
continue;
}
int xpid = sm.xpt.getByXpath(base_xpath_string);
if (matcher != null && !matcher.matches(base_xpath_string, xpid)) {
continue;
}
if (excludeAlways.matcher(base_xpath_string).matches()) {
continue;
}

int xpid = sm.xpt.getByXpath(base_xpath_string);
if (matcher != null && !matcher.matches(base_xpath_string, xpid)) {
// Only display metazone data for which an English value exists
if (isMetazones && suff != "/commonlyUsed") {
String engValue = translationHintsFile.getStringValue(base_xpath_string);
if (engValue == null || engValue.length() == 0) {
continue;
}
if (excludeAlways.matcher(base_xpath_string).matches()) {
continue;
}

// Only display metazone data for which an English value exists
if (isMetazones && suff != "/commonlyUsed") {
String engValue = translationHintsFile.getStringValue(base_xpath_string);
if (engValue == null || engValue.length() == 0) {
continue;
}
}
// Filter out data that is higher than the desired coverage level
int coverageValue = getCoverageInfo().getCoverageValue(base_xpath_string, locale.getBaseName());
}
// Filter out data that is higher than the desired coverage level
int coverageValue = getCoverageInfo().getCoverageValue(base_xpath_string, locale.getBaseName());

DataSection.DataRow myp = getDataRow(base_xpath_string); /* rowXPath */
DataSection.DataRow myp = getDataRow(base_xpath_string); /* rowXPath */

myp.coverageValue = coverageValue;
myp.coverageValue = coverageValue;

// set it up..
int base_xpath = sm.xpt.getByXpath(base_xpath_string);
// set it up..
int base_xpath = sm.xpt.getByXpath(base_xpath_string);

// set up tests
myp.setShimTests(base_xpath, base_xpath_string, checkCldr);
} // end inner for loop
} // end outer for loop
} // end if timezone
// set up tests
myp.setShimTests(base_xpath, base_xpath_string, checkCldr);
} // end inner for loop
} // end outer for loop
}

/**
Expand Down Expand Up @@ -1939,9 +1941,6 @@ public DataRow getDataRow(String xpath) {
*
* @param ourSrc the CLDRFile
* @param checkCldr the TestResultBundle
* Old param workingCoverageLevel was always "comprehensive"
*
* Called only by DataSection.make, as section.populateFrom(ourSrc, checkCldr, workingCoverageLevel).
*/
private void populateFrom(CLDRFile ourSrc, TestResultBundle checkCldr) {
STFactory stf = sm.getSTFactory();
Expand Down Expand Up @@ -2322,16 +2321,18 @@ public String toJSONString() throws JSONException {
JSONObject obj = new JSONObject(str);
itemList.put(d.fieldHash(), obj);
} catch (JSONException ex) {
SurveyLog.logException(ex, "JSON serialization error for row: "
+ d.xpath + " : Full row is: " + d.toString());
String msg = "JSON serialization error for row: "
+ d.xpath + " : Full row is: " + d.toString();
logger.log(java.util.logging.Level.WARNING, msg, ex);
throw new JSONException(ex);
}
}
result.put("rows", itemList);
result.put("xpathPrefix", xpathPrefix);
return result.toString();
} catch (Throwable t) {
SurveyLog.logException(t, "Trying to load rows for " + this.toString());
String msg = "Trying to load rows for " + this.toString();
logger.log(java.util.logging.Level.WARNING, msg, t);
throw new JSONException(t);
}
}
Expand Down
14 changes: 7 additions & 7 deletions tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,13 @@ public StatusAction getShowRowAction(
) {

PathHeader.SurveyToolStatus status = ph.getSurveyToolStatus();
// always forbid deprecated items - don't show.
/*
* Always forbid DEPRECATED items - don't show.
*
* Currently, bulk submission and TC voting are allowed even for SurveyToolStatus.HIDE,
* but not for SurveyToolStatus.DEPRECATED. If we ever want to treat HIDE and DEPRECATED
* the same here, then it would be simpler to call ph.shouldHide which is true for both.
*/
if (status == SurveyToolStatus.DEPRECATED) {
return StatusAction.FORBID_READONLY;
}
Expand All @@ -178,12 +184,6 @@ public StatusAction getShowRowAction(
return StatusAction.ALLOW_TICKET_ONLY;
}

/*
* TODO: is it intentional that bulk submission and TC voting are allowed even for SurveyToolStatus.HIDE?
* If not, fix it by calling PathHeader.shouldHide() above instead of referencing SurveyToolStatus.DEPRECATED
* above and SurveyToolStatus.HIDE below. Otherwise add a comment here confirming that these are allowed for
* SurveyToolStatus.HIDE. Reference: https://unicode-org.atlassian.net/browse/CLDR-14877
*/

// always forbid bulk import except in data submission.
if (inputMethod == InputMethod.BULK && this != Phase.SUBMISSION) {
Expand Down
2 changes: 1 addition & 1 deletion tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateSidewaysView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ private static void loadInformation(Factory cldrFactory) {
if (path.indexOf("/identity") >= 0) continue;
if (path.indexOf("/references") >= 0) continue;
PathHeader ph = fixPath(path, postFix);
if (ph.shouldHide()) {
if (ph == null || ph.shouldHide()) {
continue;
}
String fullPath = cldrFile.getFullXPath(path);
Expand Down
35 changes: 5 additions & 30 deletions tools/cldr-code/src/main/java/org/unicode/cldr/util/VettingViewer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
import java.util.TreeSet;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.RecursiveAction;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.unicode.cldr.test.CheckCLDR;
import org.unicode.cldr.test.CheckCLDR.CheckStatus;
import org.unicode.cldr.test.CheckCLDR.CheckStatus.Subtype;
import org.unicode.cldr.test.CheckCLDR.Options;
import org.unicode.cldr.test.CheckCoverage;
import org.unicode.cldr.test.CheckNew;
import org.unicode.cldr.test.CoverageLevel2;
Expand Down Expand Up @@ -62,8 +62,6 @@ public class VettingViewer<T> {

private static final String SPLIT_CHAR = "\uFFFE";

private static final Pattern ALT_PROPOSED = PatternCache.get("\\[@alt=\"[^\"]*proposed");

private static final boolean DEBUG_THREADS = false;

public static Set<CheckCLDR.CheckStatus.Subtype> OK_IF_VOTED = EnumSet.of(Subtype.sameAsEnglish);
Expand Down Expand Up @@ -338,7 +336,7 @@ public Status initErrorStatus(CLDRFile cldrFile) {
options = new HashMap<>();
result = new ArrayList<>();
checkCldr = CheckCLDR.getCheckAll(factory, ".*");
checkCldr.setCldrFileToCheck(cldrFile, options, result);
checkCldr.setCldrFileToCheck(cldrFile, new Options(options), result);
return Status.ok;
}

Expand Down Expand Up @@ -545,7 +543,6 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile,
final DefaultErrorStatus errorChecker = new DefaultErrorStatus(cldrFactory);

errorChecker.initErrorStatus(sourceFile);
Matcher altProposed = ALT_PROPOSED.matcher("");
problems = EnumSet.noneOf(Choice.class);

// now look through the paths
Expand All @@ -557,9 +554,9 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile,
boolean latin = VettingViewer.isLatinScriptLocale(sourceFile);
CLDRFile baselineFileUnresolved = (baselineFile == null) ? null : baselineFile.getUnresolved();
for (String path : sourceFile.fullIterable()) {
if (specificSinglePath != null && !specificSinglePath.equals(path))
if (specificSinglePath != null && !specificSinglePath.equals(path)) {
continue;

}
String value = sourceFile.getWinningValueForVettingViewer(path);
statusMessage.setLength(0);
subtypes.clear();
Expand All @@ -572,34 +569,12 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile,
progressCallback.nudge(); // Let the user know we're moving along.

PathHeader ph = pathTransform.fromPath(path);
if (ph.shouldHide()) {
if (ph == null || ph.shouldHide()) {
continue;
}

// note that the value might be missing!

// make sure we only look at the real values
if (altProposed.reset(path).find()) {
/*
* TODO: explain this exclusion mechanism and why it's not implemented with
* PathHeader.SurveyToolStatus.HIDE and PathHeader.shouldHide().
* I put a breakpoint on this “continue” and never hit it. Is this dead code?
* If there are any matching paths, are they getting filtered out earlier for
* some other reason? Does DataSection have or need a similar filter?
* Reference: https://unicode-org.atlassian.net/browse/CLDR-14877
*/
continue;
}

/*
* TODO: as above, explain this exclusion mechanism, and encapsulate it better;
* where else do we (or should we) skip such paths?
* Reference: https://unicode-org.atlassian.net/browse/CLDR-14877
*/
if (path.contains("/references")) {
continue;
}

Level level = supplementalDataInfo.getCoverageLevel(path, sourceFile.getLocaleID());

// skip all but errors above the requested level
Expand Down
Loading

0 comments on commit a62ef81

Please sign in to comment.