From 1bcc0f613c465666200709d1ac02dc846aa34d30 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Fri, 5 Apr 2019 17:41:04 -0400 Subject: [PATCH] WIP: fix caches, replace `id()` function with plain `id` property --- modules/core/context.js | 8 +-- modules/core/validator.js | 86 ++++++++++++++------------- modules/ui/entity_issues.js | 19 +++--- modules/ui/issues.js | 2 +- test/spec/validations/missing_role.js | 4 +- 5 files changed, 63 insertions(+), 56 deletions(-) diff --git a/modules/core/context.js b/modules/core/context.js index 864b8ef04..582e148a5 100644 --- a/modules/core/context.js +++ b/modules/core/context.js @@ -466,12 +466,10 @@ export function coreContext() { validator = coreValidator(context); -// todo: put these back in somehow, -// but for now try to just validate an edit difference // run validation upon restoring from page reload - // history.on('restore', function() { - // validator.validate(); - // }); + history.on('restore', function() { + validator.validate(); + }); // re-run validation upon a significant graph change history.on('change', function(difference) { if (difference) { diff --git a/modules/core/validator.js b/modules/core/validator.js index d3e899066..a0efcf58c 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -15,7 +15,6 @@ export function coreValidator(context) { var _entityRules = []; // var _changesRules = []; // skip for now - // var _issues = []; var _issuesByIssueID = {}; var _issuesByEntityID = {}; @@ -40,7 +39,6 @@ export function coreValidator(context) { validator.reset = function() { // clear caches - // _issues = []; _issuesByIssueID = {}; _issuesByEntityID = {}; @@ -54,31 +52,22 @@ export function coreValidator(context) { validator.getIssues = function() { return Object.values(_issuesByIssueID); - // return _issues; }; validator.getWarnings = function() { - // return _issues return Object.values(_issuesByIssueID) .filter(function(d) { return d.severity === 'warning'; }); }; validator.getErrors = function() { - // return _issues return Object.values(_issuesByIssueID) .filter(function(d) { return d.severity === 'error'; }); }; validator.getEntityIssues = function(entityID) { - // var entity = context.hasEntity(entityID); - // if (!entity) return []; - - // if (!_issuesByEntityID[entityID]) { - // _issuesByEntityID[entityID] = validator.validateEntity(entity); - // } return _issuesByEntityID[entityID] || []; }; @@ -182,7 +171,7 @@ export function coreValidator(context) { // })); var graph = context.graph(); - var entityIDs = difference.extantIDs(); + var entityIDs = difference.extantIDs(); // created and modified var entitiesToCheck = entityIDs.reduce(function(acc, entityID) { var entity = graph.entity(entityID); @@ -207,31 +196,48 @@ export function coreValidator(context) { }); return acc; + }, new Set()); - // var issuesByID = {}; + // clear caches for existing issues related to changed entities + difference.deleted().forEach(clearCaches); + entitiesToCheck.forEach(clearCaches); + + // detect new issues and update caches entitiesToCheck.forEach(function(entity) { - var detected = validator.validateEntity(entity); - _issuesByEntityID[entity.id] = detected; - detected.forEach(function(d) { _issuesByIssueID[d.id] = d; }); - // entityIssues.forEach(function(issue) { - // Different entities can produce the same issue so store them by - // the ID to ensure that there are no duplicate issues. - // issuesByID[issue.id()] = issue; - // }); + var issues = validator.validateEntity(entity); + + issues.forEach(function(issue) { + var entities = issue.entities || []; + entities.forEach(function(entity) { + if (!_issuesByEntityID[entity.id]) { + _issuesByEntityID[entity.id] = []; + } + _issuesByEntityID[entity.id].push(issue); + }); + _issuesByIssueID[issue.id] = issue; + }); }); - // for (var issueID in issuesByID) { - // _issues.push(issuesByID[issueID]); - // } - - // dispatch.call('reload', validator, _issues); dispatch.call('reload'); }; return utilRebind(validator, dispatch, 'on'); + + + + function clearCaches(entity) { + var issues = _issuesByEntityID[entity.id] || []; + issues.forEach(function(issue) { + var entities = issue.entities || []; + entities.forEach(function(entity) { + delete _issuesByEntityID[entity.id]; + }); + delete _issuesByIssueID[issue.id]; + }); + } } @@ -246,32 +252,32 @@ export function validationIssue(attrs) { this.fixes = attrs.fixes; // optional - array of validationIssueFix objects this.hash = attrs.hash; // optional - string to further differentiate the issue - - var _id; + this.id = generateID.apply(this); // generated - see below // A unique, deterministic string hash. // Issues with identical id values are considered identical. - this.id = function() { - if (_id) return _id; - - _id = this.type; + function generateID() { + var parts = [this.type]; if (this.hash) { // subclasses can pass in their own differentiator - _id += this.hash; + parts.push(this.hash); } - // factor in the entities this issue is for + // include entities this issue is for // (sort them so the id is deterministic) - var entityKeys = this.entities.map(osmEntity.key); - _id += entityKeys.sort().join(); + if (this.entities) { + var entityKeys = this.entities.map(osmEntity.key).sort(); + parts.push.apply(parts, entityKeys); + } - // factor in loc since two separate issues can have an + // include loc since two separate issues can have an // idential type and entities, e.g. in crossing_ways if (this.loc) { - _id += this.loc.join(); + parts.push.apply(parts, this.loc); } - return _id; - }; + + return parts.join(':'); + } this.extent = function(resolver) { diff --git a/modules/ui/entity_issues.js b/modules/ui/entity_issues.js index 698adf09e..48d373532 100644 --- a/modules/ui/entity_issues.js +++ b/modules/ui/entity_issues.js @@ -49,10 +49,10 @@ export function uiEntityIssues(context) { function render(selection) { var issues = context.validator().getEntityIssues(_entityID); - _expandedIssueID = issues.length > 0 ? issues[0].id() : null; + _expandedIssueID = issues.length > 0 ? issues[0].id : null; var items = selection.selectAll('.issue') - .data(issues, function(d) { return d.id(); }); + .data(issues, function(d) { return d.id; }); // Exit items.exit() @@ -69,13 +69,17 @@ export function uiEntityIssues(context) { ) .on('mouseover.highlight', function(d) { // don't hover-highlight the selected entity - var ids = d.entities.filter(function(e) { return e.id !== _entityID; }) + var ids = d.entities + .filter(function(e) { return e.id !== _entityID; }) .map(function(e) { return e.id; }); + utilHighlightEntities(ids, true, context); }) .on('mouseout.highlight', function(d) { - var ids = d.entities.filter(function(e) { return e.id !== _entityID; }) + var ids = d.entities + .filter(function(e) { return e.id !== _entityID; }) .map(function(e) { return e.id; }); + utilHighlightEntities(ids, false, context); }); @@ -83,10 +87,9 @@ export function uiEntityIssues(context) { .append('button') .attr('class', 'message') .on('click', function(d) { - - _expandedIssueID = d.id(); // expand only the clicked item + _expandedIssueID = d.id; // expand only the clicked item selection.selectAll('.issue') - .classed('expanded', function(d) { return d.id() === _expandedIssueID; }); + .classed('expanded', function(d) { return d.id === _expandedIssueID; }); var extent = d.extent(context.graph()); if (extent) { @@ -115,7 +118,7 @@ export function uiEntityIssues(context) { // Update items = items .merge(itemsEnter) - .classed('expanded', function(d) { return d.id() === _expandedIssueID; }); + .classed('expanded', function(d) { return d.id === _expandedIssueID; }); items.select('.issue-icon svg use') // propagate bound data .attr('href', function(d) { diff --git a/modules/ui/issues.js b/modules/ui/issues.js index 0dfd9b76c..878f5363e 100644 --- a/modules/ui/issues.js +++ b/modules/ui/issues.js @@ -88,7 +88,7 @@ export function uiIssues(context) { function drawIssuesList(selection, issues) { var items = selection.selectAll('li') - .data(issues, function(d) { return d.id(); }); + .data(issues, function(d) { return d.id; }); // Exit items.exit() diff --git a/test/spec/validations/missing_role.js b/test/spec/validations/missing_role.js index 78a577884..0ed6d43b6 100644 --- a/test/spec/validations/missing_role.js +++ b/test/spec/validations/missing_role.js @@ -77,7 +77,7 @@ describe('iD.validations.missing_role', function () { createRelation({ type: 'multipolygon' }, null); var issues = validate(); expect(issues).to.have.lengthOf(2); - expect(issues[0].id()).to.eql(issues[1].id()); + expect(issues[0].id).to.eql(issues[1].id); var issue = issues[0]; expect(issue.type).to.eql('missing_role'); expect(issue.entities).to.have.lengthOf(2); @@ -89,7 +89,7 @@ describe('iD.validations.missing_role', function () { createRelation({ type: 'multipolygon' }, ' '); var issues = validate(); expect(issues).to.have.lengthOf(2); - expect(issues[0].id()).to.eql(issues[1].id()); + expect(issues[0].id).to.eql(issues[1].id); var issue = issues[0]; expect(issue.type).to.eql('missing_role'); expect(issue.entities).to.have.lengthOf(2);