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.

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

Add preset aliases property [WIP, Feedback required] #6412

Closed
wants to merge 4 commits into from
Closed

Add preset aliases property [WIP, Feedback required] #6412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff01e22
add array utility function
westnordost May 22, 2019
40501c0
add aliases to preset
westnordost May 22, 2019
18949e7
remove unnecessary emtpy terms array from tests
westnordost May 22, 2019
aeca7e1
add aliases to search and make search return match objects
westnordost May 22, 2019
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
33 changes: 28 additions & 5 deletions build_data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ function generatePresets(tstrings, faIcons, tnpIcons) {
validate(file, preset, presetSchema);

tstrings.presets[id] = {
name: preset.name,
name: [preset.name].concat(preset.aliases || []).join(','),
terms: (preset.terms || []).join(',')
};

Expand Down Expand Up @@ -351,14 +351,17 @@ function generateTranslations(fields, presets, tstrings) {
var tags = p.tags || {};
var keys = Object.keys(tags);

var tagsDescription = keys.map(function(k) { return k + '=' + tags[k]; }).join(', ');

if (keys.length) {
preset['name#'] = keys.map(function(k) { return k + '=' + tags[k]; }).join(', ');
preset['name#'] = `Name for '${tagsDescription}'. Aliases may be added after the name, separated by commas.`;
}
if (p.searchable !== false) {
preset['terms#'] = `Related terms beside the name and aliases with which the preset for '${tagsDescription}' should be found, separated by commas. '<>' if none are eligible.`;
if (p.terms && p.terms.length) {
preset['terms#'] = 'terms: ' + p.terms.join();
preset['terms#'] += ' English terms: ' + p.terms.join();
}
preset.terms = '<translate with synonyms or related terms for \'' + preset.name + '\', separated by commas>';
preset.terms = '<>';
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ....# stuff is the description that is shown in Transifex as "Developer Notes".

The description for name mentions the possibility to add aliases after the name.
The description for terms now also mentions what they are used for and how to explicitly omit them. The previous message directly in the translation was sometimes misunderstood by translators so that they literally translated the English terms.

cc @1ec5

delete preset.terms;
}
Expand Down Expand Up @@ -652,8 +655,28 @@ function writeEnJson(tstrings) {
var imagery = YAML.load(data[1]);
var community = YAML.load(data[2]);

function transifexToPreset(transifex) {
var names = transifex.name.trim().split(/\s*,+\s*/);
var preset = {};
preset.name = names[0];
if(names.length > 1) preset.aliases = names.slice(1);
// terms stay Transifex'ied (comma separated string)
if(transifex.terms.length) preset.terms = transifex.terms;
Copy link
Contributor Author

@westnordost westnordost May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

#6298

The ifs are not necessary but keep unnecessary terms: "" and aliases: [] out of en.json.

return preset;
}

var tstringPresets = {};
for (const id of Object.keys(tstrings.presets)) {
tstringPresets[id] = transifexToPreset(tstrings.presets[id]);
}
var enjsonPresetStrings = {
categories: tstrings.categories,
fields: tstrings.fields,
presets: tstringPresets
};

var enjson = core;
enjson.en.presets = tstrings;
enjson.en.presets = enjsonPresetStrings;
enjson.en.imagery = imagery.en.imagery;
enjson.en.community = community.en;

Expand Down
12 changes: 12 additions & 0 deletions data/presets/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ A features can only match one preset even if its tags and geometry could technic

This property is required. There is no default.

##### `aliases`

A list of aliases beside the name for this preset. Optional.

Aliases, like names, use [title case](https://en.wikipedia.org/wiki/Letter_case#Title_case).

This may be displayed alongside `name` in the result list when searching for presets.

##### `terms`

A list of related terms beside the name and aliases with which the preset should be found. Optional.

##### `addTags`

The tags that are added to the feature when selecting this preset. Defaults to `tags`. If needed, this property will typically be a superset of `tags`.
Expand Down
9 changes: 8 additions & 1 deletion data/presets/schema/preset.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,15 @@
"description": "The URL of a remote image that is more specific than 'icon'",
"type": "string"
},
"aliases": {
"description": "English aliases / synonyms",
"type": "array",
"items": {
"type": "string"
}
},
"terms": {
"description": "English synonyms or related terms",
"description": "English related terms / keywords",
"type": "array",
"items": {
"type": "string"
Expand Down
23 changes: 16 additions & 7 deletions data/update_locales.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,27 @@ function getResource(resource, callback) {

} else {
if (resource === 'presets') {
// remove terms that were not really translated
// remove terms that were not really translated, transform names back to name+aliases
var presets = (result.presets && result.presets.presets) || {};
for (const key of Object.keys(presets)) {
var preset = presets[key];
if (!preset.terms) continue;
preset.terms = preset.terms.replace(/<.*>/, '').trim();
if (!preset.terms) {
delete preset.terms;
if (!Object.keys(preset).length) {
delete presets[key];
if (preset.terms) {
preset.terms = preset.terms.replace(/<.*>/, '').trim();
if (!preset.terms) {
delete preset.terms;
}
}
if (preset.name) {
var names = preset.name.trim().split(/\s*,+\s*/);
preset.name = names[0];
if(names.length > 1) {
preset.aliases = names.slice(1);
}
}

if (!Object.keys(preset).length) {
delete presets[key];
}
}
}

Expand Down
155 changes: 99 additions & 56 deletions modules/presets/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { utilArrayUniq, utilEditDistance } from '../util';
import { utilEditDistance, utilArrayMapTruthy } from '../util';


export function presetCollection(collection) {
Expand Down Expand Up @@ -42,7 +42,7 @@ export function presetCollection(collection) {
},

search: function(value, geometry, countryCode) {
if (!value) return this;
if (!value) return [];

value = value.toLowerCase();

Expand All @@ -58,26 +58,48 @@ export function presetCollection(collection) {
return index === 0;
}

function sortNames(a, b) {
var aCompare = (a.suggestion ? a.originalName : a.name()).toLowerCase();
var bCompare = (b.suggestion ? b.originalName : b.name()).toLowerCase();
function getMatchWord(match) {
if (match.alias) return match.alias;
if (match.term) return match.term;
if (match.tagValue) return match.tagValue;
if (match.preset.suggestion) return match.preset.originalName;
return match.preset.name();
}

function sortMatches(a, b) {
var aCompare = getMatchWord(a).toLowerCase();
var bCompare = getMatchWord(b).toLowerCase();

// priority if search string matches preset name exactly - #4325
// priority if search string matches word exactly - #4325
if (value === aCompare) return -1;
if (value === bCompare) return 1;

// priority for smaller edit distance
var i = a.fuzziness - b.fuzziness;
if (i !== 0) return i;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this since it is now easy to get this information


// priority for higher matchScore
var i = b.originalScore - a.originalScore;
i = b.preset.originalScore - a.preset.originalScore;
if (i !== 0) return i;

// priority if search string appears earlier in preset name
// priority if search string appears earlier in word
i = aCompare.indexOf(value) - bCompare.indexOf(value);
if (i !== 0) return i;

// priority for shorter preset names
// priority for shorter words
return aCompare.length - bCompare.length;
}

// take those presets from the source array for which the given function
// returns a match and sort the resulting list of matches
function take(array, fn) {
return utilArrayMapTruthy(array, function (val, i) {
var r = fn.call(null, val, i, array);
if (r) array[i] = null;
return r;
}).sort(sortMatches);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides making this filter + map + sort more concise in the code below, this is also a performance improvement: Presets that are added to the results are not searched again.

var pool = this.collection;
if (countryCode) {
pool = pool.filter(function(a) {
Expand All @@ -93,81 +115,102 @@ export function presetCollection(collection) {
});

// matches value to preset.name
var leading_name = searchable
.filter(function(a) {
return leading(a.name().toLowerCase());
}).sort(sortNames);
var leading_name = take(searchable, function(a) {
if (leading(a.name().toLowerCase())) return { preset: a };
});

// matches value to preset.aliases values
var leading_aliases = take(searchable, function(a) {
var aliases = a.aliases();
for (var i = 0; i < aliases.length; i++) {
var alias = aliases[i];
if (leading(alias.toLowerCase())) return { preset: a, alias: alias };
}
});

// matches value to preset.terms values
var leading_terms = searchable
.filter(function(a) {
return (a.terms() || []).some(leading);
});
var leading_terms = take(searchable, function(a) {
var terms = a.terms();
for (var i = 0; i < terms.length; i++) {
var term = terms[i];
if (leading(term)) return { preset: a, term: term };
}
});

// matches value to preset.tags values
var leading_tag_values = searchable
.filter(function(a) {
return Object.values(a.tags || {})
.filter(function(val) { return val !== '*'; })
.some(leading);
});
var leading_tag_values = take(searchable, function(a) {
var tagValues = Object.values(a.tags || []);
for (var i = 0; i < tagValues.length; i++) {
var tagValue = tagValues[i];
if (tagValue !== '*' && leading(tagValue)) {
return { preset: a, tagValue: tagValue };
}
}
});
Copy link
Contributor Author

@westnordost westnordost May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Both tagValue and term could as well be just booleans isTagMatch, isTermMatch or similar or as well be left out completely because it is currently not used in the UI besides sorting (which did not happen before). It is simply "free" to record this info at this point and could be used in the UI to display term-matches differently.


var leading_suggestions = suggestions
.filter(function(a) {
return leadingStrict(a.originalName.toLowerCase());
}).sort(sortNames);
var leading_suggestions = take(suggestions, function(a) {
if (leadingStrict(a.originalName.toLowerCase())) return { preset: a };
});

// finds close matches to value in preset.name
var similar_name = searchable
.map(function(a) {
return { preset: a, dist: utilEditDistance(value, a.name()) };
}).filter(function(a) {
return a.dist + Math.min(value.length - a.preset.name().length, 0) < 3;
}).sort(function(a, b) {
return a.dist - b.dist;
}).map(function(a) {
return a.preset;
});
var similar_name = take(searchable, function(a) {
var dist = utilEditDistance(value, a.name());
if (dist + Math.min(value.length - a.name().length, 0) < 3) {
return { preset: a, fuzziness: dist };
}
});

// finds close matches to value in preset.aliases
var similar_aliases = take(searchable, function(a) {
return utilArrayMapTruthy(a.aliases(), function(alias) {
var dist = utilEditDistance(value, alias);
if (dist + Math.min(value.length - alias.length, 0) < 3) {
return { preset: a, alias: alias, fuzziness: dist };
}
// only keep the one match with the lowest fuzziness
}).sort(function(a,b) { return a.fuzziness - b.fuzziness; })[0];
});

// finds close matches to value in preset.terms
var similar_terms = searchable
.filter(function(a) {
return (a.terms() || []).some(function(b) {
return utilEditDistance(value, b) + Math.min(value.length - b.length, 0) < 3;
});
});
var similar_terms = take(searchable, function(a) {
return utilArrayMapTruthy(a.terms(), function(term) {
var dist = utilEditDistance(value, term);
if (dist + Math.min(value.length - term.length, 0) < 3) {
return { preset: a, term: term, fuzziness: dist };
}
// only keep the one match with the lowest fuzziness
}).sort(function(a,b) { return a.fuzziness - b.fuzziness; })[0];
});

var similar_suggestions = suggestions
.map(function(a) {
return { preset: a, dist: utilEditDistance(value, a.originalName.toLowerCase()) };
}).filter(function(a) {
return a.dist + Math.min(value.length - a.preset.originalName.length, 0) < 1;
}).sort(function(a, b) {
return a.dist - b.dist;
}).map(function(a) {
return a.preset;
});
var similar_suggestions = take(suggestions, function(a) {
var dist = utilEditDistance(value, a.originalName);
if (dist + Math.min(value.length - a.originalName.length, 0) < 1) {
return { preset: a, fuzziness: dist };
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fuzziness is used for sorting and could be used (in the future) to display those items differently.


var results = leading_name.concat(
leading_aliases,
leading_suggestions,
leading_terms,
leading_tag_values,
similar_name,
similar_aliases,
similar_suggestions,
similar_terms
).slice(0, maxSearchResults - 1);

if (geometry) {
if (typeof geometry === 'string') {
results.push(presets.fallback(geometry));
results.push({ preset: presets.fallback(geometry) });
} else {
geometry.forEach(function(geom) {
results.push(presets.fallback(geom));
results.push({ preset: presets.fallback(geom) });
});
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: If the desired geometry is already passed in this function as a parameter, why is the whole collection not filtered to only return presets that can be used with this geometry? Is this is a bug?


return presetCollection(utilArrayUniq(results));
return results;
}
};

Expand Down
7 changes: 5 additions & 2 deletions modules/presets/preset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ export function presetPreset(id, preset, fields, visible, rawPresets) {

preset.originalName = preset.name || '';


preset.name = function() {
if (preset.suggestion) {
var path = id.split('/');
Expand All @@ -154,11 +153,15 @@ export function presetPreset(id, preset, fields, visible, rawPresets) {

preset.originalTerms = (preset.terms || []).join();


preset.terms = function() {
return preset.t('terms', { 'default': preset.originalTerms }).toLowerCase().trim().split(/\s*,+\s*/);
};

preset.originalAliases = (preset.aliases || []);

preset.aliases = function() {
return preset.t('aliases', { 'default': preset.originalAliases });
};

preset.isFallback = function() {
var tagCount = Object.keys(preset.tags).length;
Expand Down
13 changes: 13 additions & 0 deletions modules/util/array.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,16 @@ export function utilArrayUniqBy(a, key) {
}, []);
}

// like Array.prototype.map(fn), only that null elements and elements for
// which the function returns something are filtered out.
// utilArrayMapTruthy([1,2,null,3,4,5], a => a%2 ? a*2 : null) == [2,6,10]
export function utilArrayMapTruthy(a, fn) {
var result = [];
for (var i = 0; i < a.length; i++) {
if (a[i] != null) {
var r = fn.call(null, a[i], i, a);
if (r != null) result.push(r);
}
}
return result;
}
Loading