From 3608659d216e8aaaefc29bbec4f6363c5bab78cc Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 10 Feb 2020 16:22:02 -0500 Subject: [PATCH] Restore field inheritance --- modules/presets/index.js | 13 ++-- modules/presets/preset.js | 111 ++++++++++++++--------------- modules/ui/fields/localized.js | 2 +- modules/ui/preset_editor.js | 14 ++-- test/spec/actions/change_preset.js | 22 +++--- test/spec/presets/preset.js | 32 ++++++--- 6 files changed, 104 insertions(+), 90 deletions(-) diff --git a/modules/presets/index.js b/modules/presets/index.js index a553779e3..28ae87e82 100644 --- a/modules/presets/index.js +++ b/modules/presets/index.js @@ -95,8 +95,7 @@ export function presetIndex(context) { const p = d.presets[presetID]; if (p) { // add or replace const isAddable = !_addablePresetIDs || _addablePresetIDs.has(presetID); - _presets[presetID] = presetPreset(presetID, p, _fields, isAddable); - // _presets[presetID] = presetPreset(presetID, p, _fields, isAddable, _presets); + _presets[presetID] = presetPreset(presetID, p, isAddable, _fields, _presets); } else { // remove (but not if it's a fallback) const existing = _presets[presetID]; if (existing && !existing.isFallback()) { @@ -132,11 +131,11 @@ export function presetIndex(context) { }); } - // Rebuild universal fields - _universal = Object.values(_fields).reduce((acc, field) => { - if (field.universal) acc.push(field); - return acc; - }, []); + // Rebuild universal fields array + _universal = Object.values(_fields).filter(field => field.universal); + + // Reset all the preset fields - they'll need to be resolved again + Object.values(_presets).forEach(preset => preset.resetFields()); // Rebuild _this.collection _this.collection = Object.values(_presets).concat(Object.values(_categories)); diff --git a/modules/presets/preset.js b/modules/presets/preset.js index 4dd702b3f..e56cb85bf 100644 --- a/modules/presets/preset.js +++ b/modules/presets/preset.js @@ -8,9 +8,13 @@ import { utilSafeClassName } from '../util/util'; // `presetPreset` decorates a given `preset` Object // with some extra methods for searching and matching geometry // -export function presetPreset(presetID, preset, allFields, addable, rawPresets) { +export function presetPreset(presetID, preset, addable, allFields, allPresets) { + allFields = allFields || {}; + allPresets = allPresets || {}; let _this = Object.assign({}, preset); // shallow copy let _addable = addable || false; + let _resolvedFields; // cache + let _resolvedMoreFields; // cache _this.id = presetID; @@ -24,13 +28,15 @@ export function presetPreset(presetID, preset, allFields, addable, rawPresets) { _this.originalReference = _this.reference || {}; - _this.fields = (_this.fields || []).map(f => allFields[f]); + _this.originalFields = (_this.fields || []); - _this.moreFields = (_this.moreFields || []).map(f => allFields[f]); + _this.originalMoreFields = (_this.moreFields || []); - if (rawPresets) { - resolveFieldInheritance(); - } + _this.fields = () => _resolvedFields || (_resolvedFields = resolve('fields')); + + _this.moreFields = () => _resolvedMoreFields || (_resolvedMoreFields = resolve('moreFields')); + + _this.resetFields = () => _resolvedFields = _resolvedMoreFields = null; _this.tags = _this.tags || {}; @@ -141,7 +147,7 @@ export function presetPreset(presetID, preset, allFields, addable, rawPresets) { tags = utilObjectOmit(tags, Object.keys(_this.removeTags)); if (geometry && !skipFieldDefaults) { - _this.fields.forEach(field => { + _this.fields().forEach(field => { if (field.matchGeometry(geometry) && field.key && field.default === tags[field.key]) { delete tags[field.key]; } @@ -188,7 +194,7 @@ export function presetPreset(presetID, preset, allFields, addable, rawPresets) { } if (geometry && !skipFieldDefaults) { - _this.fields.forEach(field => { + _this.fields().forEach(field => { if (field.matchGeometry(geometry) && field.key && !tags[field.key] && field.default) { tags[field.key] = field.default; } @@ -201,66 +207,57 @@ export function presetPreset(presetID, preset, allFields, addable, rawPresets) { // For a preset without fields, use the fields of the parent preset. // Replace {preset} placeholders with the fields of the specified presets. - function resolveFieldInheritance() { + function resolve(which) { + const fieldIDs = (which === 'fields' ? _this.originalFields : _this.originalMoreFields); + let resolved = []; - ['fields', 'moreFields'].forEach(prop => { - let fieldIDs = []; - if (preset[prop] && preset[prop].length) { // fields were defined - preset[prop].forEach(fieldID => { - const match = fieldID.match(/\{(.*)\}/); - if (match !== null) { // presetID wrapped in braces {} - const inheritIDs = inheritedFieldIDs(match[1], prop); - if (inheritIDs !== null) { - fieldIDs = fieldIDs.concat(inheritIDs); - } else { - /* eslint-disable no-console */ - console.log(`Cannot resolve presetID ${match[0]} found in ${_this.id} ${prop}`); - /* eslint-enable no-console */ - } - } else { - fieldIDs.push(fieldID); // no braces - just a normal field - } - }); - - } else { // no fields defined, so use the parent's if possible - const endIndex = _this.id.lastIndexOf('/'); - const parentID = endIndex && _this.id.substring(0, endIndex); - if (parentID) { - fieldIDs = inheritedFieldIDs(parentID, prop); - } + fieldIDs.forEach(fieldID => { + const match = fieldID.match(/\{(.*)\}/); + if (match !== null) { // a presetID wrapped in braces {} + resolved = resolved.concat(inheritFields(match[1], which)); + } else if (allFields[fieldID]) { // a normal fieldID + resolved.push(allFields[fieldID]); + } else { + console.log(`Cannot resolve "${fieldID}" found in ${_this.id}.${which}`); // eslint-disable-line no-console } - - fieldIDs = utilArrayUniq(fieldIDs); - preset[prop] = fieldIDs; - rawPresets[_this.id][prop] = fieldIDs; }); - // Skip `fields` for the keys which define the _this. - // These are usually `typeCombo` fields like `shop=*` - function shouldInheritFieldWithID(fieldID) { - const f = allFields[fieldID]; - if (f.key) { - if (_this.tags[f.key] !== undefined && - // inherit anyway if multiple values are allowed or just a checkbox - f.type !== 'multiCombo' && f.type !== 'semiCombo' && f.type !== 'check' - ) return false; + // no fields resolved, so use the parent's if possible + if (!resolved.length) { + const endIndex = _this.id.lastIndexOf('/'); + const parentID = endIndex && _this.id.substring(0, endIndex); + if (parentID) { + resolved = inheritFields(parentID, which); } - return true; } - // returns an array of field IDs to inherit from the given presetID, if found - function inheritedFieldIDs(presetID, prop) { - if (!presetID) return null; + return utilArrayUniq(resolved); - const inheritPreset = rawPresets[presetID]; - if (!inheritPreset) return null; - let inheritFieldIDs = inheritPreset[prop] || []; - if (prop === 'fields') { - inheritFieldIDs = inheritFieldIDs.filter(shouldInheritFieldWithID); + // returns an array of fields to inherit from the given presetID, if found + function inheritFields(presetID, which) { + const parent = allPresets[presetID]; + if (!parent) return []; + + if (which === 'fields') { + return parent.fields().filter(shouldInherit); + } else if (which === 'moreFields') { + return parent.moreFields(); + } else { + return []; } + } - return inheritFieldIDs; + + // Skip `fields` for the keys which define the preset. + // These are usually `typeCombo` fields like `shop=*` + function shouldInherit(f) { + if (f.key && _this.tags[f.key] !== undefined && + // inherit anyway if multiple values are allowed or just a checkbox + f.type !== 'multiCombo' && f.type !== 'semiCombo' && f.type !== 'check' + ) return false; + + return true; } } diff --git a/modules/ui/fields/localized.js b/modules/ui/fields/localized.js index 96c410e51..a7a5d886e 100644 --- a/modules/ui/fields/localized.js +++ b/modules/ui/fields/localized.js @@ -106,7 +106,7 @@ export function uiFieldLocalized(field, context) { var preset = context.presets().match(entity, context.graph()); var isSuggestion = preset && preset.suggestion; - var showsBrand = preset && preset.fields.filter(function(d) { + var showsBrand = preset && preset.originalFields.filter(function(d) { return d.id === 'brand'; }).length; // protect standardized brand names diff --git a/modules/ui/preset_editor.js b/modules/ui/preset_editor.js index fed55d997..a63e4a06a 100644 --- a/modules/ui/preset_editor.js +++ b/modules/ui/preset_editor.js @@ -43,18 +43,22 @@ export function uiPresetEditor(context) { var presetsManager = context.presets(); - var allFields = [], allMoreFields = []; + var allFields = []; + var allMoreFields = []; var sharedTotalFields; _presets.forEach(function(preset) { - allFields = utilArrayUnion(allFields, preset.fields); - allMoreFields = utilArrayUnion(allMoreFields, preset.moreFields); + var fields = preset.fields(); + var moreFields = preset.moreFields(); + + allFields = utilArrayUnion(allFields, fields); + allMoreFields = utilArrayUnion(allMoreFields, moreFields); if (!sharedTotalFields) { - sharedTotalFields = utilArrayUnion(preset.fields, preset.moreFields); + sharedTotalFields = utilArrayUnion(fields, moreFields); } else { sharedTotalFields = sharedTotalFields.filter(function(field) { - return preset.fields.indexOf(field) !== -1 || preset.moreFields.indexOf(field) !== -1; + return fields.indexOf(field) !== -1 || moreFields.indexOf(field) !== -1; }); } }); diff --git a/test/spec/actions/change_preset.js b/test/spec/actions/change_preset.js index a80623849..8f677e2eb 100644 --- a/test/spec/actions/change_preset.js +++ b/test/spec/actions/change_preset.js @@ -1,25 +1,25 @@ describe('iD.actionChangePreset', function() { - var oldPreset = iD.presetPreset('old', {tags: {old: 'true'}}), - newPreset = iD.presetPreset('new', {tags: {new: 'true'}}); + var oldPreset = iD.presetPreset('old', {tags: {old: 'true'}}); + var newPreset = iD.presetPreset('new', {tags: {new: 'true'}}); it('changes from one preset\'s tags to another\'s', function() { - var entity = iD.osmNode({tags: {old: 'true'}}), - graph = iD.coreGraph([entity]), - action = iD.actionChangePreset(entity.id, oldPreset, newPreset); + var entity = iD.osmNode({tags: {old: 'true'}}); + var graph = iD.coreGraph([entity]); + var action = iD.actionChangePreset(entity.id, oldPreset, newPreset); expect(action(graph).entity(entity.id).tags).to.eql({new: 'true'}); }); it('adds the tags of a new preset to an entity without an old preset', function() { - var entity = iD.osmNode(), - graph = iD.coreGraph([entity]), - action = iD.actionChangePreset(entity.id, null, newPreset); + var entity = iD.osmNode(); + var graph = iD.coreGraph([entity]); + var action = iD.actionChangePreset(entity.id, null, newPreset); expect(action(graph).entity(entity.id).tags).to.eql({new: 'true'}); }); it('removes the tags of an old preset from an entity without a new preset', function() { - var entity = iD.osmNode({tags: {old: 'true'}}), - graph = iD.coreGraph([entity]), - action = iD.actionChangePreset(entity.id, oldPreset, null); + var entity = iD.osmNode({tags: {old: 'true'}}); + var graph = iD.coreGraph([entity]); + var action = iD.actionChangePreset(entity.id, oldPreset, null); expect(action(graph).entity(entity.id).tags).to.eql({}); }); }); diff --git a/test/spec/presets/preset.js b/test/spec/presets/preset.js index e18f03131..11093f62b 100644 --- a/test/spec/presets/preset.js +++ b/test/spec/presets/preset.js @@ -1,7 +1,17 @@ describe('iD.presetPreset', function() { - it('has optional fields', function() { - var preset = iD.presetPreset('test', {}); - expect(preset.fields).to.eql([]); + + describe('#fields', function() { + it('has no fields by default', function() { + var preset = iD.presetPreset('test', {}); + expect(preset.fields()).to.eql([]); + }); + }); + + describe('#moreFields', function() { + it('has no moreFields by default', function() { + var preset = iD.presetPreset('test', {}); + expect(preset.moreFields()).to.eql([]); + }); }); describe('#matchGeometry', function() { @@ -136,14 +146,16 @@ describe('iD.presetPreset', function() { }); it('adds default tags of fields with matching geometry', function() { + var isAddable = true; var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}); - var preset = iD.presetPreset('test', {fields: ['field']}, {field: field}); + var preset = iD.presetPreset('test', {fields: ['field']}, isAddable, {field: field}); expect(preset.setTags({}, 'area')).to.eql({area: 'yes', building: 'yes'}); }); it('adds no default tags of fields with non-matching geometry', function() { + var isAddable = true; var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}); - var preset = iD.presetPreset('test', {fields: ['field']}, {field: field}); + var preset = iD.presetPreset('test', {fields: ['field']}, isAddable, {field: field}); expect(preset.setTags({}, 'point')).to.eql({}); }); @@ -179,8 +191,9 @@ describe('iD.presetPreset', function() { }); it('removes tags that match field default tags', function() { - var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}), - preset = iD.presetPreset('test', {fields: ['field']}, {field: field}); + var isAddable = true; + var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}); + var preset = iD.presetPreset('test', {fields: ['field']}, isAddable, {field: field}); expect(preset.unsetTags({building: 'yes'}, 'area')).to.eql({}); }); @@ -190,8 +203,9 @@ describe('iD.presetPreset', function() { }); it('preserves tags that do not match field default tags', function() { - var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}), - preset = iD.presetPreset('test', {fields: ['field']}, {field: field}); + var isAddable = true; + var field = iD.presetField('field', {key: 'building', geometry: 'area', default: 'yes'}); + var preset = iD.presetPreset('test', {fields: ['field']}, isAddable, {field: field}); expect(preset.unsetTags({building: 'yep'}, 'area')).to.eql({ building: 'yep'}); });