From 3fe9d75f35b2cf09f244b02100b7d165be3c0de5 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 22 Apr 2019 12:17:29 -0400 Subject: [PATCH] Fix issues with issue cache for entities that no longer exist --- modules/core/validator.js | 101 +++++++++++++++++++++----------------- modules/ui/tools/save.js | 11 +++-- 2 files changed, 63 insertions(+), 49 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index 249fb8e0a..136206701 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -69,8 +69,19 @@ export function coreValidator(context) { return issues.filter(function(issue) { if (_disabledRules[issue.type]) return false; + // Sanity check: This issue may be for an entity that not longer exists. + // If we detect this, uncache and return false so it is not incluced.. + var entities = issue.entities || []; + for (var i = 0; i < entities.length; i++) { + var entity = entities[i]; + if (!context.hasEntity(entity.id)) { + delete _issuesByEntityID[entity.id]; + delete _issuesByIssueID[issue.id]; + return false; + } + } + if (opts.what === 'edited') { - var entities = issue.entities || []; var isEdited = entities.some(function(entity) { return changes[entity.id]; }); if (entities.length && !isEdited) return false; } @@ -129,21 +140,30 @@ export function coreValidator(context) { // // Remove a single entity and all its related issues from the caches // - function uncacheEntity(entity) { - var issueIDs = _issuesByEntityID[entity.id]; + function uncacheEntityID(entityID) { + var issueIDs = _issuesByEntityID[entityID]; if (!issueIDs) return; issueIDs.forEach(function(issueID) { var issue = _issuesByIssueID[issueID]; - var entities = issue.entities || []; - entities.forEach(function(other) { // other entities will need to be revalidated - if (other.id !== entity.id) { - delete _issuesByEntityID[other.id]; - } - }); - delete _issuesByIssueID[issue.id]; + if (issue) { + // When multiple entities are involved (e.g. crossing_ways), + // remove this issue from the other entity caches too.. + var entities = issue.entities || []; + entities.forEach(function(other) { + if (other.id !== entityID) { + var otherIssueIDs = _issuesByEntityID[other.id]; + if (otherIssueIDs) { + otherIssueIDs.delete(issueID); + } + } + }); + } + + delete _issuesByIssueID[issueID]; }); - delete _issuesByEntityID[entity.id]; + + delete _issuesByEntityID[entityID]; } @@ -213,24 +233,24 @@ export function coreValidator(context) { validator.validateEntities = function(entityIDs) { var graph = context.graph(); - var entitiesToCheck = entityIDs.reduce(function(acc, entityID) { - var entity = graph.entity(entityID); - if (acc.has(entity)) return acc; + var entityIDsToCheck = entityIDs.reduce(function(acc, entityID) { + if (acc.has(entityID)) return acc; + acc.add(entityID); - acc.add(entity); + var entity = graph.entity(entityID); var checkParentRels = [entity]; if (entity.type === 'node') { // include parent ways graph.parentWays(entity).forEach(function(parentWay) { checkParentRels.push(parentWay); - acc.add(parentWay); + acc.add(parentWay.id); }); } checkParentRels.forEach(function(entity) { // include parent relations if (entity.type !== 'relation') { // but not super-relations - graph.parentRelations(entity).forEach(function(parentRel) { - acc.add(parentRel); + graph.parentRelations(entity).forEach(function(parentRelation) { + acc.add(parentRelation.id); }); } }); @@ -240,10 +260,11 @@ export function coreValidator(context) { }, new Set()); // clear caches for existing issues related to changed entities - entitiesToCheck.forEach(uncacheEntity); + entityIDsToCheck.forEach(uncacheEntityID); // detect new issues and update caches - entitiesToCheck.forEach(function(entity) { + entityIDsToCheck.forEach(function(entityID) { + var entity = graph.entity(entityID); var issues = validateEntity(entity); issues.forEach(function(issue) { @@ -285,40 +306,32 @@ export function coreValidator(context) { } var entityIDs = difference.extantIDs(); // created and modified - difference.deleted().forEach(uncacheEntity); // deleted + difference.deleted().forEach(uncacheEntityID); // deleted validator.validateEntities(entityIDs); // dispatches 'validated' }; - - // run validation upon restoring user's changes - context.history().on('restore.validator', function() { - validator.validate(); - }); - - // re-run validation upon merging fetched data - context.history().on('merge.validator', function(entities) { - utilCallWhenIdle(function() { - if (!entities) return; - var ids = entities.map(function(entity) { return entity.id; }); - validator.validateEntities(ids); - })(); - }); - - // // re-run validation on history change (frequent) - // context.history().on('change.validator', function(difference) { - // if (!difference) return; - // validator.validate(); - // }); + // WHEN TO RUN VALIDATION: + // When graph changes: context.history() - .on('undone.validator', validator.validate) - .on('redone.validator', validator.validate); + .on('restore.validator', validator.validate) // restore saved history + .on('undone.validator', validator.validate) // undo + .on('redone.validator', validator.validate); // redo + // but not on 'change' (e.g. while drawing) - // re-run validation when the user switches editing modes (less frequent) + // When user chages editing modes: context .on('exit.validator', validator.validate); + // When merging fetched data: + context.history() + .on('merge.validator', function(entities) { + if (!entities) return; + var ids = entities.map(function(entity) { return entity.id; }); + utilCallWhenIdle(function() { validator.validateEntities(ids); })(); + }); + return validator; } diff --git a/modules/ui/tools/save.js b/modules/ui/tools/save.js index 1efa5f910..16a0caee2 100644 --- a/modules/ui/tools/save.js +++ b/modules/ui/tools/save.js @@ -51,6 +51,7 @@ export function uiToolSave(context) { } } + function updateCount() { var val = history.difference().summary().length; if (val === _numChanges) return; @@ -75,7 +76,6 @@ export function uiToolSave(context) { tool.render = function(selection) { - tooltipBehavior = tooltip() .placement('bottom') .html(true) @@ -104,9 +104,10 @@ export function uiToolSave(context) { context.history() - .on('restore.save', updateCount) - .on('change.save', function(difference) { - if (difference) updateCount(); + .on('change.save', function(diff) { + if (!diff || diff.didChange.addition || diff.didChange.deletion) { + updateCount(); // only on significant changes + } }); context @@ -122,8 +123,8 @@ export function uiToolSave(context) { }); }; - tool.uninstall = function() { + tool.uninstall = function() { context.keybinding() .off(key, true);