Skip to content

Commit 731da70

Browse files
tyrasdtyrasd
authored
include all tags from special fields when removing tags during preset change
1 parent 6143e6d commit 731da70

5 files changed

Lines changed: 182 additions & 38 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
7878
* Standardize tooltips in combo boxes: always show (full) raw tag, don't duplicate the already rendered titles, and show descriptions from wiki/taginfo where available ([#12010], thanks [@tordans])
7979
* Use full width for the dropdown box for the values of the `access` field ([#12065])
8080
* Preserve the order of options when a combo field accepts both static options as well as autosuggestions from taginfo and display additional tag values from taginfo as raw-options only ([#12117])
81+
* When removing tags while changing presets: include all tags associated with `localized`, `multiCombo` and `directionalCombo` fields ([#12075], [#11696])
8182
#### :hammer: Development
8283
* Replace `sinon` with `vitest`'s built in spy/mock library ([#12058])
8384
* Add type annotations to `context.js` module ([#11589], thanks [@k-yle])
@@ -101,6 +102,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
101102
[#12063]: https://github.com/openstreetmap/iD/issues/12063
102103
[#12065]: https://github.com/openstreetmap/iD/issues/12065
103104
[#12070]: https://github.com/openstreetmap/iD/issues/12070
105+
[#12075]: https://github.com/openstreetmap/iD/issues/12075
104106
[#12078]: https://github.com/openstreetmap/iD/issues/12078
105107
[#12110]: https://github.com/openstreetmap/iD/issues/12110
106108
[#12117]: https://github.com/openstreetmap/iD/issues/12117

‎modules/actions/change_preset.js‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,27 +14,38 @@ export function actionChangePreset(entityID, oldPreset, newPreset, skipFieldDefa
1414
if (newPreset.addTags) {
1515
preserveKeys = preserveKeys.concat(Object.keys(newPreset.addTags));
1616
}
17-
if (oldPreset && !oldPreset.id.startsWith(newPreset.id)) {
18-
// only if old preset is not a sub-preset of the new one:
19-
// preserve tags for which the new preset has a field
20-
// https://github.com/openstreetmap/iD/issues/9372
17+
if (oldPreset) {
18+
const wasSubPreset = oldPreset.id.startsWith(newPreset.id);
2119
newPreset.fields(loc).concat(newPreset.moreFields(loc))
2220
.filter(f => f.matchGeometry(geometry))
23-
.flatMap(f => f.allKeys())
21+
.flatMap(f => f.allKeys(tags))
22+
.filter(key => {
23+
if (wasSubPreset) {
24+
// if old preset was a sub-preset of the new one:
25+
// don't preserve tags which defined the old sub-preset,
26+
// even if the new preset has a field for it
27+
// for example:
28+
// amenity=restaurant + cuisine=pizza + name, etc.
29+
// should result in: amenity=restaurant + name, etc.
30+
// https://github.com/openstreetmap/iD/issues/9372
31+
return oldPreset.tags[key] === undefined;
32+
}
33+
return true;
34+
})
2435
.filter(Boolean)
2536
.forEach(key => preserveKeys.push(key));
26-
}
2737

28-
if (oldPreset && (oldPreset.id !== newPreset.id)) {
29-
// 'field-keys' are keys used by fields (different to the keys used by preset itself)
30-
const oldPresetFieldKeys = [
31-
...oldPreset.fields(loc),
32-
...oldPreset.moreFields(loc)
33-
].flatMap(f => f.allKeys());
38+
if (oldPreset.id !== newPreset.id) {
39+
// 'field-keys' are keys used by fields (different to the keys used by preset itself)
40+
const oldPresetFieldKeys = [
41+
...oldPreset.fields(loc),
42+
...oldPreset.moreFields(loc)
43+
].flatMap(f => f.allKeys(tags));
3444

35-
// field-keys used by the old preset but not the new preset
36-
const fieldKeysToRemove = utilArrayDifference(oldPresetFieldKeys, preserveKeys);
37-
tags = utilObjectOmit(tags, fieldKeysToRemove);
45+
// field-keys used by the old preset but not the new preset
46+
const fieldKeysToRemove = utilArrayDifference(oldPresetFieldKeys, preserveKeys);
47+
tags = utilObjectOmit(tags, fieldKeysToRemove);
48+
}
3849
}
3950
}
4051
if (oldPreset) tags = oldPreset.unsetTags(tags, geometry, preserveKeys, false, loc);

‎modules/presets/field.js‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { localizer, t } from '../core/localizer';
2+
import { LANGUAGE_SUFFIX_REGEX } from '../ui/fields';
23
import { utilSafeClassName } from '../util/util';
34

45

@@ -53,11 +54,32 @@ export function presetField(fieldID, field, allFields) {
5354
_this.increment = (_this.type === 'number' || _this.type === 'integer') ? (_this.increment || 1) : undefined;
5455

5556
/** all keys controlled by this field */
56-
_this.allKeys = () => {
57-
const allKeys = [];
58-
if (_this.key) allKeys.push(_this.key);
59-
if (_this.keys) allKeys.push(..._this.keys);
60-
return allKeys;
57+
_this.allKeys = (tags) => {
58+
const allKeys = new Set();
59+
if (_this.key) allKeys.add(_this.key);
60+
if (_this.keys) _this.keys.forEach(key => allKeys.add(key));
61+
if (field.type === 'directionalCombo' && _this.key) {
62+
// directionalCombo fields can have an additional key describing the for
63+
// cases where both directions share a "common" value.
64+
// The field also support *:both. The preset decides which field to write to.
65+
const baseKey = field.key.replace(/:both$/, '');
66+
allKeys.add(baseKey);
67+
allKeys.add(`${baseKey}:both`);
68+
}
69+
if (field.type === 'localized' && field.key && tags) {
70+
const prefix = `${field.key}:`;
71+
Object.keys(tags)
72+
.filter(k => k.startsWith(prefix))
73+
.filter(k => LANGUAGE_SUFFIX_REGEX.test(k))
74+
.forEach(key => allKeys.add(key));
75+
}
76+
if (field.type === 'multiCombo' && field.key && tags) {
77+
const prefix = field.key + (field.key.endsWith(':') ? '' : ':');
78+
Object.keys(tags)
79+
.filter(k => k.startsWith(prefix))
80+
.forEach(key => allKeys.add(key));
81+
}
82+
return [...allKeys];
6183
};
6284

6385
return _this;

‎modules/ui/field.js‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -66,15 +66,7 @@ export function uiField(context, presetField, entityIDs, options) {
6666

6767

6868
function allKeys() {
69-
let keys = field.keys || [field.key];
70-
if (field.type === 'directionalCombo' && field.key) {
71-
// directionalCombo fields can have an additional key describing the for
72-
// cases where both directions share a "common" value.
73-
// The field also support *:both. The preset decides which field to write to.
74-
const baseKey = field.key.replace(/:both$/, '');
75-
keys = keys.concat(baseKey, `${baseKey}:both`);
76-
}
77-
return keys;
69+
return field.allKeys();
7870
}
7971

8072

‎test/spec/actions/change_preset.js‎

Lines changed: 126 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -165,15 +165,132 @@ describe('iD.actionChangePreset', function() {
165165
});
166166
});
167167

168-
// https://github.com/openstreetmap/iD/issues/9372
169-
it('does not preserve field tags when changing from a subpreset to its parent', function() {
170-
var entity = new iD.osmNode({tags: {highway: 'service', service: 'driveway'}});
171-
var graph = new iD.coreGraph([entity]);
172-
var oldPreset = iD.presetPreset('highway/service/driveway', {tags: {highway: 'service', service: 'driveway'}});
173-
var newPreset = iD.presetPreset('highway/service', {tags: {highway: 'service'}, fields: ['field']}, undefined, {
174-
field: iD.presetField('field', {key: 'service'})
168+
// https://github.com/openstreetmap/iD/issues/12075
169+
it.each([{
170+
fieldType: 'multiCombo',
171+
fieldId: 'recycling',
172+
fieldKey: 'recycling:',
173+
fieldKeys: [],
174+
oldTags: { 'recycling:paper': 'yes', 'recycling:others': 'no' },
175+
tagsToPreserve: {}
176+
}, {
177+
fieldType: 'localized',
178+
fieldId: 'name',
179+
fieldKey: 'name',
180+
fieldKeys: [],
181+
oldTags: { 'name': 'foo', 'name:en': 'bar', 'name:etymology:wikidata': 'Q860' },
182+
tagsToPreserve: { 'name:etymology:wikidata': 'Q860' }
183+
}, {
184+
fieldType: 'directionalCombo',
185+
fieldId: 'cycleway',
186+
fieldKey: 'cycleway',
187+
fieldKeys: [ 'cycleway:left', 'cycleway:right' ],
188+
oldTags: { 'cycleway:left': 'no', 'cycleway:both': 'separate', 'cycleway:foo': 'bar' },
189+
tagsToPreserve: { 'cycleway:foo': 'bar' }
190+
}])('does not preserve $fieldType field tags that are only present in the old preset', ({
191+
fieldType, fieldId, fieldKey, fieldKeys, oldTags, tagsToPreserve
192+
}) => {
193+
const entity = iD.osmNode({
194+
tags: {
195+
amenity: 'recycling',
196+
...oldTags,
197+
unrelatedTagKey: 'unrelatedTagValue',
198+
},
199+
loc: [0, 0],
175200
});
176-
var action = iD.actionChangePreset(entity.id, oldPreset, newPreset);
177-
expect(action(graph).entity(entity.id).tags).to.eql({highway: 'service'});
201+
const graph = new iD.coreGraph([entity]);
202+
203+
const fields = {
204+
[fieldId]: iD.presetField('recycling', { type: fieldType, key: fieldKey, keys: fieldKeys }),
205+
};
206+
207+
const oldPreset = iD.presetPreset(
208+
'amenity/recycling',
209+
{ tags: { amenity: 'recycling' }, fields: [fieldId] },
210+
undefined,
211+
fields,
212+
);
213+
const newPreset = iD.presetPreset(
214+
'amenity/bench',
215+
{ tags: { amenity: 'bench' }, fields: [] },
216+
undefined,
217+
fields,
218+
);
219+
const action = iD.actionChangePreset(entity.id, oldPreset, newPreset);
220+
expect(action(graph).entity(entity.id).tags).toStrictEqual({
221+
// no field tags are preserved
222+
amenity: 'bench',
223+
...tagsToPreserve,
224+
unrelatedTagKey: 'unrelatedTagValue',
225+
});
226+
});
227+
228+
// https://github.com/openstreetmap/iD/pull/12218#issuecomment-4314204446
229+
it.each([{
230+
fieldType: 'multiCombo',
231+
fieldId: 'recycling',
232+
fieldKey: 'recycling:',
233+
fieldKeys: [],
234+
oldTags: { 'recycling:paper': 'yes', 'recycling:others': 'no' }
235+
}, {
236+
fieldType: 'localized',
237+
fieldId: 'name',
238+
fieldKey: 'name',
239+
fieldKeys: [],
240+
oldTags: { 'name': 'foo', 'name:en': 'bar', 'name:etymology:wikidata': 'Q860' }
241+
}, {
242+
fieldType: 'directionalCombo',
243+
fieldId: 'cycleway',
244+
fieldKey: 'cycleway',
245+
fieldKeys: [ 'cycleway:left', 'cycleway:right' ],
246+
oldTags: { 'cycleway:left': 'no', 'cycleway:both': 'separate', 'cycleway:foo': 'bar' }
247+
}])('preserve $fieldType field tags when they are present in the old and the new preset', ({
248+
fieldType, fieldId, fieldKey, fieldKeys, oldTags
249+
}) => {
250+
const entity = iD.osmNode({
251+
tags: {
252+
amenity: 'recycling',
253+
...oldTags,
254+
},
255+
loc: [0, 0],
256+
});
257+
const graph = new iD.coreGraph([entity]);
258+
259+
const fields = {
260+
[fieldId]: iD.presetField('recycling', { type: fieldType, key: fieldKey, keys: fieldKeys }),
261+
};
262+
263+
const oldPreset = iD.presetPreset(
264+
'amenity/recycling',
265+
{ tags: { amenity: 'recycling' }, fields: [fieldId] },
266+
undefined,
267+
fields,
268+
);
269+
const newPreset = iD.presetPreset(
270+
'amenity/bench',
271+
{ tags: { amenity: 'bench' }, fields: [fieldId] },
272+
undefined,
273+
fields,
274+
);
275+
const action = iD.actionChangePreset(entity.id, oldPreset, newPreset);
276+
expect(action(graph).entity(entity.id).tags).toStrictEqual({
277+
// all field tags are preserved
278+
...oldTags,
279+
amenity: 'bench', // override primary preset tag
280+
});
281+
});
282+
283+
// https://github.com/openstreetmap/iD/issues/9372
284+
it('does not preserve old preset\'s primary tags when changing from a subpreset to its parent', () => {
285+
const entity = new iD.osmNode({tags: {highway: 'service', service: 'driveway', name: 'foo bar'}});
286+
const graph = new iD.coreGraph([entity]);
287+
const fields = {
288+
field: iD.presetField('field', {key: 'service'}),
289+
name: iD.presetField('name', {key: 'name'})
290+
};
291+
const oldPreset = iD.presetPreset('highway/service/driveway', {tags: {highway: 'service', service: 'driveway'}, fields: ['name']}, undefined, fields);
292+
const newPreset = iD.presetPreset('highway/service', {tags: {highway: 'service'}, fields: ['field', 'name']}, undefined, fields);
293+
const action = iD.actionChangePreset(entity.id, oldPreset, newPreset);
294+
expect(action(graph).entity(entity.id).tags).to.eql({highway: 'service', name: 'foo bar'});
178295
});
179296
});

0 commit comments

Comments
 (0)