Skip to content

Commit

Permalink
support aliases in preset search
Browse files Browse the repository at this point in the history
closes #6139
  • Loading branch information
tyrasd committed May 24, 2022
1 parent b4d0f0c commit 8796a41
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 7 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,14 @@ _Breaking developer changes, which may affect downstream projects or sites that
* Imply `access=no` in access field of `highway=construction` objects ([#9102])
* Don't show non-language tag-suffixes in multilingual name field ([#9124], thanks [@wcedmisten])
* Render horse riding centers like farmyards ([#9118])
* Support searching presets by their `aliases` ([#6139])
#### Other
* Redact more API tokens from custom imagery sources in changeset metadata tags ([#8976], thanks [@k-yle])
#### :hammer: Development
* Switch build system to [esbuild](https://esbuild.github.io/) for much faster builds ([#8774], thanks [@mbrzakovic] and [@bhousel])
* Upgrade some dependencies: maki to `v7.1`, `fontawesome` to `v6.1`, `d3` to `v7.3`, `node-diff` to `v3.1`, `mocha` to `v9.2`, `svg-sprite` to `v1.5.4`, `marked` to `v4.0`

[#6139]: https://github.com/openstreetmap/iD/issues/6139
[#8774]: https://github.com/openstreetmap/iD/pull/8774
[#8811]: https://github.com/openstreetmap/iD/issues/8811
[#8905]: https://github.com/openstreetmap/iD/issues/8905
Expand Down
3 changes: 3 additions & 0 deletions modules/presets/category.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,8 @@ export function presetCategory(categoryID, category, allPresets) {
return _searchNameStripped;
};

_this.searchAliases = () => [];
_this.searchAliasesStripped = () => [];

return _this;
}
4 changes: 2 additions & 2 deletions modules/presets/collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export function presetCollection(collection) {

// matches value to preset.name
const leadingNames = searchable
.filter(a => leading(a.searchName()))
.filter(a => leading(a.searchName()) || a.searchAliases().some(leading))
.sort(sortPresets('searchName'));

This comment has been minimized.

Copy link
@westnordost

westnordost May 24, 2022

Contributor

How can this work? Doesn't the sortPresets function sort by preset name here? It looks like that matches with aliases will not be sorted at all. (Hence, unit tests would make sense)

This comment has been minimized.

Copy link
@tyrasd

tyrasd May 25, 2022

Author Member

good point, I overlooked that sortPresets does do more than just sort by matchScore. Should be fixed in d486ab1.


// matches value to preset suggestion name
Expand All @@ -107,7 +107,7 @@ export function presetCollection(collection) {
.sort(sortPresets('searchName'));

const leadingNamesStripped = searchable
.filter(a => leading(a.searchNameStripped()))
.filter(a => leading(a.searchNameStripped()) || a.searchAliasesStripped().some(leading))
.sort(sortPresets('searchNameStripped'));

const leadingSuggestionsStripped = suggestions
Expand Down
38 changes: 33 additions & 5 deletions modules/presets/preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export function presetPreset(presetID, preset, addable, allFields, allPresets) {
let _resolvedMoreFields; // cache
let _searchName; // cache
let _searchNameStripped; // cache
let _searchAliases; // cache
let _searchAliasesStripped; // cache

_this.id = presetID;

Expand All @@ -26,6 +28,8 @@ export function presetPreset(presetID, preset, addable, allFields, allPresets) {

_this.originalName = _this.name || '';

_this.originalAliases = _this.aliases || '';

_this.originalScore = _this.matchScore || 1;

_this.originalReference = _this.reference || {};
Expand Down Expand Up @@ -123,6 +127,11 @@ export function presetPreset(presetID, preset, addable, allFields, allPresets) {
};


_this.aliases = () => {
return _this.t('aliases', { 'default': _this.originalAliases }).trim().split(/\s*\n\s*/);

This comment has been minimized.

Copy link
@westnordost

westnordost May 24, 2022

Contributor

Is it possible that \r\n ends up in the translation file somehow (e.g. by translators on Windows)? Or just \r? Also, duplicate endlines?

I use a regex like \s*[\r\n]+\s*.

This comment has been minimized.

Copy link
@tyrasd

tyrasd May 25, 2022

Author Member

I've only seen single \n's in the dist output files until now, but I guess that that using a more forgiving regex wouldn't hurt either.

};


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

Expand All @@ -135,15 +144,26 @@ export function presetPreset(presetID, preset, addable, allFields, allPresets) {

_this.searchNameStripped = () => {
if (!_searchNameStripped) {
_searchNameStripped = _this.searchName();
// split combined diacritical characters into their parts
if (_searchNameStripped.normalize) _searchNameStripped = _searchNameStripped.normalize('NFD');
// remove diacritics
_searchNameStripped = _searchNameStripped.replace(/[\u0300-\u036f]/g, '');
_searchNameStripped = stripDiacritics(_this.searchName());
}
return _searchNameStripped;
};

_this.searchAliases = () => {
if (!_searchAliases) {
_searchAliases = _this.aliases().map(alias => alias.toLowerCase());
}
return _searchAliases;
};

_this.searchAliasesStripped = () => {
if (!_searchAliasesStripped) {
_searchAliasesStripped = _this.searchAliases();
_searchAliasesStripped = _searchAliasesStripped.map(stripDiacritics);
}
return _searchAliasesStripped;
};

_this.isFallback = () => {
const tagCount = Object.keys(_this.tags).length;
return tagCount === 0 || (tagCount === 1 && _this.tags.hasOwnProperty('area'));
Expand Down Expand Up @@ -306,5 +326,13 @@ export function presetPreset(presetID, preset, addable, allFields, allPresets) {
}


function stripDiacritics(s) {
// split combined diacritical characters into their parts
if (s.normalize) s = s.normalize('NFD');
// remove diacritics
s = s.replace(/[\u0300-\u036f]/g, '');
return s;
}

return _this;
}

1 comment on commit 8796a41

@westnordost
Copy link
Contributor

@westnordost westnordost commented on 8796a41 May 24, 2022

Choose a reason for hiding this comment

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

For preset matching, there are a lot of tests that also serve as kind of a documentation how the presets match. It would be consistent and helpful to replicate the same matching in other libs/apps to also add tests for this new behavior.

Please sign in to comment.