diff --git a/modules/core/validation/models.js b/modules/core/validation/models.js index b9819e060..d6215ba6d 100644 --- a/modules/core/validation/models.js +++ b/modules/core/validation/models.js @@ -14,6 +14,7 @@ export function validationIssue(attrs) { this.hash = attrs.hash; // optional - string to further differentiate the issue this.id = generateID.apply(this); // generated - see below + this.key = generateKey.apply(this); // generated - see below (call after generating this.id) this.autoFix = null; // generated - if autofix exists, will be set below // A unique, deterministic string hash. @@ -39,6 +40,13 @@ export function validationIssue(attrs) { return parts.join(':'); } + // An identifier suitable for use as the second argument to d3.selection#data(). + // (i.e. this should change whenever the data needs to be refreshed) + function generateKey() { + return this.id + ':' + Date.now().toString(); // include time of creation + } + + this.extent = function(resolver) { if (this.loc) { return geoExtent(this.loc); diff --git a/modules/core/validator.js b/modules/core/validator.js index 0f8a3a1c8..2e899d9b4 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -189,16 +189,22 @@ export function coreValidator(context) { let seen = new Set(); let results = []; - // collect head issues - caused by user edits + // collect head issues - present in the user edits if (_headCache.graph && _headCache.graph !== _baseCache.graph) { Object.values(_headCache.issuesByIssueID).forEach(issue => { + // In the head cache, only count features that the user is responsible for - #8632 + // For example, a user can undo some work and an issue will still present in the + // head graph, but we don't want to credit the user for causing that issue. + const userModified = (issue.entityIds || []).some(id => _completeDiff.hasOwnProperty(id)); + if (opts.what === 'edited' && !userModified) return; // present in head but user didn't touch it + if (!filter(issue)) return; seen.add(issue.id); results.push(issue); }); } - // collect base issues - not caused by user edits + // collect base issues - present before user edits if (opts.what === 'all') { Object.values(_baseCache.issuesByIssueID).forEach(issue => { if (!filter(issue)) return; @@ -488,10 +494,10 @@ export function coreValidator(context) { _headCache.graph = currGraph; // take snapshot _completeDiff = context.history().difference().complete(); const incrementalDiff = coreDifference(prevGraph, currGraph); - const entityIDs = Object.keys(incrementalDiff.complete()); + let entityIDs = Object.keys(incrementalDiff.complete()); + entityIDs = _headCache.withAllRelatedEntities(entityIDs); // expand set - // if (!entityIDs.size) { - if (!entityIDs.length) { + if (!entityIDs.size) { dispatch.call('validated'); return Promise.resolve(); } @@ -540,8 +546,8 @@ export function coreValidator(context) { if (!_headCache.graph) _headCache.graph = baseGraph; if (!_baseCache.graph) _baseCache.graph = baseGraph; - const entityIDs = entities.map(entity => entity.id); - // entityIDs = entityIDsToValidate(entityIDs, baseGraph); // expand set + let entityIDs = entities.map(entity => entity.id); + entityIDs = _baseCache.withAllRelatedEntities(entityIDs); // expand set validateEntitiesAsync(entityIDs, _baseCache); }); @@ -629,7 +635,7 @@ export function coreValidator(context) { // - the user did something to one of the entities involved in the issue // // Arguments - // `entityIDs` - Array containing entity IDs. + // `entityIDs` - Array or Set containing entity IDs. // function updateResolvedIssues(entityIDs) { entityIDs.forEach(entityID => { @@ -656,7 +662,7 @@ export function coreValidator(context) { // Schedule validation for many entities. // // Arguments - // `entityIDs` - Array containing entity IDs. + // `entityIDs` - Array or Set containing entityIDs. // `graph` - the graph to validate that contains those entities // `cache` - the cache to store results in (_headCache or _baseCache) // @@ -666,13 +672,14 @@ export function coreValidator(context) { // function validateEntitiesAsync(entityIDs, cache) { // Enqueue the work - const jobs = entityIDs.map(entityID => { + const jobs = Array.from(entityIDs).map(entityID => { if (cache.queuedEntityIDs.has(entityID)) return null; // queued already cache.queuedEntityIDs.add(entityID); + // Clear caches for existing issues related to this entity + cache.uncacheEntityID(entityID); + return () => { - // Clear caches for existing issues related to this entity - cache.uncacheEntityID(entityID); cache.queuedEntityIDs.delete(entityID); const graph = cache.graph; @@ -681,11 +688,6 @@ export function coreValidator(context) { const entity = graph.hasEntity(entityID); // Sanity check: don't validate deleted entities if (!entity) return; - // In the head cache, only validate features that the user is responsible for - #8632 - // For example, a user can undo some work and an issue will still present in the - // head graph, but we don't want to credit the user for causing that issue. - if (cache.which === 'head' && !_completeDiff.hasOwnProperty(entityID)) return; - // detect new issues and update caches const result = validateEntity(entity, graph); if (result.provisional) { // provisional result @@ -795,24 +797,20 @@ function validationCache(which) { issuesByEntityID: {} // entity.id -> Set(issue.id) }; - cache.cacheIssues = (issues) => { - issues.forEach(issue => { - const entityIDs = issue.entityIds || []; - entityIDs.forEach(entityID => { - if (!cache.issuesByEntityID[entityID]) { - cache.issuesByEntityID[entityID] = new Set(); - } - cache.issuesByEntityID[entityID].add(issue.id); - }); - cache.issuesByIssueID[issue.id] = issue; + + cache.cacheIssue = (issue) => { + (issue.entityIds || []).forEach(entityID => { + if (!cache.issuesByEntityID[entityID]) { + cache.issuesByEntityID[entityID] = new Set(); + } + cache.issuesByEntityID[entityID].add(issue.id); }); + cache.issuesByIssueID[issue.id] = issue; }; + cache.uncacheIssue = (issue) => { - // When multiple entities are involved (e.g. crossing_ways), - // remove this issue from the other entity caches too.. - const entityIDs = issue.entityIds || []; - entityIDs.forEach(entityID => { + (issue.entityIds || []).forEach(entityID => { if (cache.issuesByEntityID[entityID]) { cache.issuesByEntityID[entityID].delete(issue.id); } @@ -820,25 +818,33 @@ function validationCache(which) { delete cache.issuesByIssueID[issue.id]; }; + + cache.cacheIssues = (issues) => { + issues.forEach(cache.cacheIssue); + }; + + cache.uncacheIssues = (issues) => { issues.forEach(cache.uncacheIssue); }; + cache.uncacheIssuesOfType = (type) => { const issuesOfType = Object.values(cache.issuesByIssueID) .filter(issue => issue.type === type); cache.uncacheIssues(issuesOfType); }; + // Remove a single entity and all its related issues from the caches cache.uncacheEntityID = (entityID) => { - const issueIDs = cache.issuesByEntityID[entityID]; - if (issueIDs) { - issueIDs.forEach(issueID => { + const entityIssueIDs = cache.issuesByEntityID[entityID]; + if (entityIssueIDs) { + entityIssueIDs.forEach(issueID => { const issue = cache.issuesByIssueID[issueID]; if (issue) { cache.uncacheIssue(issue); - } else { + } else { // shouldnt happen, clean up delete cache.issuesByIssueID[issueID]; } }); @@ -849,5 +855,32 @@ function validationCache(which) { }; + // Return the expandeded set of entityIDs related to issues for the given entityIDs + // + // Arguments + // `entityIDs` - Array or Set containing entityIDs. + // + cache.withAllRelatedEntities = (entityIDs) => { + let result = new Set(); + (entityIDs || []).forEach(entityID => { + result.add(entityID); // include self + + const entityIssueIDs = cache.issuesByEntityID[entityID]; + if (entityIssueIDs) { + entityIssueIDs.forEach(issueID => { + const issue = cache.issuesByIssueID[issueID]; + if (issue) { + (issue.entityIds || []).forEach(relatedID => result.add(relatedID)); + } else { // shouldnt happen, clean up + delete cache.issuesByIssueID[issueID]; + } + }); + } + }); + + return result; + }; + + return cache; } diff --git a/modules/modes/select.js b/modules/modes/select.js index 582c956c5..d141ce773 100644 --- a/modules/modes/select.js +++ b/modules/modes/select.js @@ -429,11 +429,14 @@ export function modeSelect(context, selectedIDs) { actionAddMidpoint({ loc: choice.loc, edge: [prev, next] }, osmNode()), t('operations.add.annotation.vertex') ); + context.validator().validate(); } else if (entity.type === 'midpoint') { context.perform( actionAddMidpoint({ loc: entity.loc, edge: entity.edge }, osmNode()), - t('operations.add.annotation.vertex')); + t('operations.add.annotation.vertex') + ); + context.validator().validate(); } } @@ -688,6 +691,7 @@ export function modeSelect(context, selectedIDs) { // the user added this relation but didn't edit it at all, so just delete it var deleteAction = actionDeleteRelation(entity.id, true /* don't delete untagged members */); context.perform(deleteAction, t('operations.delete.annotation.relation')); + context.validator().validate(); } }; diff --git a/modules/ui/commit_warnings.js b/modules/ui/commit_warnings.js index e02656c2c..b83f528fc 100644 --- a/modules/ui/commit_warnings.js +++ b/modules/ui/commit_warnings.js @@ -43,7 +43,7 @@ export function uiCommitWarnings(context) { var items = container.select('ul').selectAll('li') - .data(issues, function(d) { return d.id; }); + .data(issues, function(d) { return d.key; }); items.exit() .remove(); diff --git a/modules/ui/sections/entity_issues.js b/modules/ui/sections/entity_issues.js index e3e8ebac1..5ca2d1de7 100644 --- a/modules/ui/sections/entity_issues.js +++ b/modules/ui/sections/entity_issues.js @@ -55,7 +55,7 @@ export function uiSectionEntityIssues(context) { _activeIssueID = _issues.length > 0 ? _issues[0].id : null; var containers = selection.selectAll('.issue-container') - .data(_issues, function(d) { return d.id; }); + .data(_issues, function(d) { return d.key; }); // Exit containers.exit() diff --git a/modules/ui/sections/validation_issues.js b/modules/ui/sections/validation_issues.js index b22625136..6b0ceeeac 100644 --- a/modules/ui/sections/validation_issues.js +++ b/modules/ui/sections/validation_issues.js @@ -73,7 +73,7 @@ export function uiSectionValidationIssues(id, severity, context) { var items = list.selectAll('li') - .data(issues, function(d) { return d.id; }); + .data(issues, function(d) { return d.key; }); // Exit items.exit()