From 7251a2ab906190e6de2ba2c485a399bb3449bf7d Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Fri, 4 Oct 2019 16:50:44 +0200 Subject: [PATCH] Make "edited issues" mean "issues the user created" instead of "issues for features edited by the user" Only include issues created by the user in the "warnings" changeset tags Include counts of issues resolved by the user in the changeset tags (close #6459) Don't include "fixme" issue counts in "warnings" changeset tags since they're not created by the user (close #6658) Don't cache crossing ways issues at the rule level --- modules/core/validator.js | 213 +++++++++++++++------------ modules/ui/commit.js | 41 +++--- modules/validations/crossing_ways.js | 48 +----- 3 files changed, 151 insertions(+), 151 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index 28d49ec4a..d5c42f74c 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -16,9 +16,9 @@ export function coreValidator(context) { var _rules = {}; var _disabledRules = {}; - var _ignoredIssueIDs = {}; // issue.id -> true - var _issuesByIssueID = {}; // issue.id -> issue - var _issuesByEntityID = {}; // entity.id -> set(issue.id) + var _ignoredIssueIDs = {}; // issue.id -> true + var _baseCache = validationCache(); // issues before any user edits + var _headCache = validationCache(); // issues after all user edits var _validatedGraph = null; var _deferred = new Set(); @@ -53,15 +53,9 @@ export function coreValidator(context) { // clear caches _ignoredIssueIDs = {}; - _issuesByIssueID = {}; - _issuesByEntityID = {}; + _baseCache = validationCache(); + _headCache = validationCache(); _validatedGraph = null; - - for (var key in _rules) { - if (typeof _rules[key].reset === 'function') { - _rules[key].reset(); // 'crossing_ways' is the only one like this - } - } }; validator.resetIgnoredIssues = function() { @@ -73,28 +67,29 @@ export function coreValidator(context) { // when the user changes the squaring thereshold, rerun this on all buildings validator.changeSquareThreshold = function() { + + reloadUnsquareIssues(_headCache, context.graph()); + reloadUnsquareIssues(_baseCache, context.history().base()); + + dispatch.call('validated'); + }; + + function reloadUnsquareIssues(cache, graph) { + var checkUnsquareWay = _rules.unsquare_way; if (typeof checkUnsquareWay !== 'function') return; // uncache existing - Object.values(_issuesByIssueID) - .filter(function(issue) { return issue.type === 'unsquare_way'; }) - .forEach(function(issue) { - var entityId = issue.entityIds[0]; // always 1 entity for unsquare way - if (_issuesByEntityID[entityId]) { - _issuesByEntityID[entityId].delete(issue.id); - } - delete _issuesByIssueID[issue.id]; - }); + cache.uncacheIssuesOfType('unsquare_way'); - var buildings = context.intersects(geoExtent([-180,-90],[180, 90])) // everywhere + var buildings = context.history().tree().intersects(geoExtent([-180,-90],[180, 90]), graph) // everywhere .filter(function(entity) { return entity.type === 'way' && entity.tags.building && entity.tags.building !== 'no'; }); // rerun for all buildings buildings.forEach(function(entity) { - var detected = checkUnsquareWay(entity, context.graph()); + var detected = checkUnsquareWay(entity, graph); if (detected.length !== 1) return; var issue = detected[0]; @@ -109,15 +104,13 @@ export function coreValidator(context) { ignoreFix.issue = issue; issue.fixes.push(ignoreFix); - if (!_issuesByEntityID[entity.id]) { - _issuesByEntityID[entity.id] = new Set(); + if (!cache.issuesByEntityID[entity.id]) { + cache.issuesByEntityID[entity.id] = new Set(); } - _issuesByEntityID[entity.id].add(issue.id); - _issuesByIssueID[issue.id] = issue; + cache.issuesByEntityID[entity.id].add(issue.id); + cache.issuesByIssueID[issue.id] = issue; }); - - dispatch.call('validated'); - }; + } // options = { @@ -128,8 +121,7 @@ export function coreValidator(context) { // }; validator.getIssues = function(options) { var opts = Object.assign({ what: 'all', where: 'all', includeIgnored: false, includeDisabledRules: false }, options); - var issues = Object.values(_issuesByIssueID); - var changes = context.history().difference().changes(); + var issues = Object.values(_headCache.issuesByIssueID); var view = context.map().extent(); return issues.filter(function(issue) { @@ -145,16 +137,13 @@ export function coreValidator(context) { for (var i = 0; i < entityIds.length; i++) { var entityId = entityIds[i]; if (!context.hasEntity(entityId)) { - delete _issuesByEntityID[entityId]; - delete _issuesByIssueID[issue.id]; + delete _headCache.issuesByEntityID[entityId]; + delete _headCache.issuesByIssueID[issue.id]; return false; } } - if (opts.what === 'edited') { - var isEdited = entityIds.some(function(entityId) { return changes[entityId]; }); - if (entityIds.length && !isEdited) return false; - } + if (opts.what === 'edited' && _baseCache.issuesByIssueID[issue.id]) return false; if (opts.where === 'visible') { var extent = issue.extent(context.graph()); @@ -165,6 +154,13 @@ export function coreValidator(context) { }); }; + validator.getResolvedIssues = function() { + var baseIssues = Object.values(_baseCache.issuesByIssueID); + return baseIssues.filter(function(issue) { + return !_headCache.issuesByIssueID[issue.id]; + }); + }; + validator.focusIssue = function(issue) { var extent = issue.extent(context.graph()); @@ -193,13 +189,15 @@ export function coreValidator(context) { validator.getEntityIssues = function(entityID, options) { - var issueIDs = _issuesByEntityID[entityID]; + var cache = _headCache; + + var issueIDs = cache.issuesByEntityID[entityID]; if (!issueIDs) return []; var opts = options || {}; return Array.from(issueIDs) - .map(function(id) { return _issuesByIssueID[id]; }) + .map(function(id) { return cache.issuesByIssueID[id]; }) .filter(function(issue) { if (opts.includeDisabledRules === 'only' && !_disabledRules[issue.type]) return false; if (!opts.includeDisabledRules && _disabledRules[issue.type]) return false; @@ -250,35 +248,6 @@ export function coreValidator(context) { }; - // - // Remove a single entity and all its related issues from the caches - // - function uncacheEntityID(entityID) { - var issueIDs = _issuesByEntityID[entityID]; - if (!issueIDs) return; - - issueIDs.forEach(function(issueID) { - var issue = _issuesByIssueID[issueID]; - if (issue) { - // When multiple entities are involved (e.g. crossing_ways), - // remove this issue from the other entity caches too.. - var entityIds = issue.entityIds || []; - entityIds.forEach(function(other) { - if (other !== entityID) { - if (_issuesByEntityID[other]) { - _issuesByEntityID[other].delete(issueID); - } - } - }); - } - - delete _issuesByIssueID[issueID]; - }); - - delete _issuesByEntityID[entityID]; - } - - function ignoreIssue(id) { _ignoredIssueIDs[id] = true; } @@ -404,14 +373,13 @@ export function coreValidator(context) { } // - // Run validation for several entities, supplied `entityIDs` + // Run validation for several entities, supplied `entityIDs`, + // against `graph` for the given `cache` // - function validateEntities(entityIDs) { - - var graph = context.graph(); + function validateEntities(entityIDs, graph, cache) { // clear caches for existing issues related to these entities - entityIDs.forEach(uncacheEntityID); + entityIDs.forEach(cache.uncacheEntityID); // detect new issues and update caches entityIDs.forEach(function(entityID) { @@ -420,19 +388,8 @@ export function coreValidator(context) { if (!entity) return; var issues = validateEntity(entity, graph); - issues.forEach(function(issue) { - var entityIds = issue.entityIds || []; - entityIds.forEach(function(entityId) { - if (!_issuesByEntityID[entityId]) { - _issuesByEntityID[entityId] = new Set(); - } - _issuesByEntityID[entityId].add(issue.id); - }); - _issuesByIssueID[issue.id] = issue; - }); + cache.cacheIssues(issues); }); - - dispatch.call('validated'); } @@ -442,6 +399,7 @@ export function coreValidator(context) { // and dispatches a `validated` event when finished. // validator.validate = function() { + var currGraph = context.graph(); _validatedGraph = _validatedGraph || context.history().base(); if (currGraph === _validatedGraph) { @@ -452,12 +410,6 @@ export function coreValidator(context) { var difference = coreDifference(oldGraph, currGraph); _validatedGraph = currGraph; - for (var key in _rules) { - if (typeof _rules[key].reset === 'function') { - _rules[key].reset(); // 'crossing_ways' is the only one like this - } - } - var createdAndModifiedEntityIDs = difference.extantIDs(true); // created/modified (true = w/relation members) var entityIDsToCheck = entityIDsToValidate(createdAndModifiedEntityIDs, currGraph); @@ -470,7 +422,9 @@ export function coreValidator(context) { // concat the sets entityIDsToCheckForOldGraph.forEach(entityIDsToCheck.add, entityIDsToCheck); - validateEntities(entityIDsToCheck); // dispatches 'validated' + validateEntities(entityIDsToCheck, context.graph(), _headCache); + + dispatch.call('validated'); }; @@ -491,8 +445,14 @@ export function coreValidator(context) { .on('merge.validator', function(entities) { if (!entities) return; var handle = window.requestIdleCallback(function() { - var ids = entities.map(function(entity) { return entity.id; }); - validateEntities(entityIDsToValidate(ids, context.graph())); + var entityIDs = entities.map(function(entity) { return entity.id; }); + var headGraph = context.graph(); + validateEntities(entityIDsToValidate(entityIDs, headGraph), headGraph, _headCache); + + var baseGraph = context.history().base(); + validateEntities(entityIDsToValidate(entityIDs, baseGraph), baseGraph, _baseCache); + + dispatch.call('validated'); }); _deferred.add(handle); }); @@ -500,3 +460,68 @@ export function coreValidator(context) { return validator; } + + +function validationCache() { + + var cache = { + issuesByIssueID: {}, // issue.id -> issue + issuesByEntityID: {} // entity.id -> set(issue.id) + }; + + cache.cacheIssues = function(issues) { + issues.forEach(function(issue) { + var entityIds = issue.entityIds || []; + entityIds.forEach(function(entityId) { + if (!cache.issuesByEntityID[entityId]) { + cache.issuesByEntityID[entityId] = new Set(); + } + cache.issuesByEntityID[entityId].add(issue.id); + }); + cache.issuesByIssueID[issue.id] = issue; + }); + }; + + cache.uncacheIssue = function(issue) { + // When multiple entities are involved (e.g. crossing_ways), + // remove this issue from the other entity caches too.. + var entityIds = issue.entityIds || []; + entityIds.forEach(function(entityId) { + if (cache.issuesByEntityID[entityId]) { + cache.issuesByEntityID[entityId].delete(issue.id); + } + }); + delete cache.issuesByIssueID[issue.id]; + }; + + cache.uncacheIssues = function(issues) { + issues.forEach(cache.uncacheIssue); + }; + + cache.uncacheIssuesOfType = function(type) { + var issuesOfType = Object.values(cache.issuesByIssueID) + .filter(function(issue) { return issue.type === type; }); + cache.uncacheIssues(issuesOfType); + }; + + // + // Remove a single entity and all its related issues from the caches + // + cache.uncacheEntityID = function(entityID) { + var issueIDs = cache.issuesByEntityID[entityID]; + if (!issueIDs) return; + + issueIDs.forEach(function(issueID) { + var issue = cache.issuesByIssueID[issueID]; + if (issue) { + cache.uncacheIssue(issue); + } else { + delete cache.issuesByIssueID[issueID]; + } + }); + + delete cache.issuesByEntityID[entityID]; + }; + + return cache; +} diff --git a/modules/ui/commit.js b/modules/ui/commit.js index 0b0a402d0..3b758aa5d 100644 --- a/modules/ui/commit.js +++ b/modules/ui/commit.js @@ -24,7 +24,8 @@ var readOnlyTags = [ /^imagery_used$/, /^host$/, /^locale$/, - /^warnings:/ + /^warnings:/, + /^resolved:/ ]; // treat most punctuation (except -, _, +, &) as hashtag delimiters - #4398 @@ -147,31 +148,37 @@ export function uiCommit(context) { } } - // remove existing warning counts + // remove existing issue counts for (var key in tags) { - if (key.match(/^warnings:/)) { + if (key.match(/(^warnings:)|(^resolved:)/)) { delete tags[key]; } } + function addIssueCounts(issues, prefix) { + var issuesByType = utilArrayGroupBy(issues, 'type'); + for (var issueType in issuesByType) { + var issuesOfType = issuesByType[issueType]; + if (issuesOfType[0].subtype) { + var issuesBySubtype = utilArrayGroupBy(issuesOfType, 'subtype'); + for (var issueSubtype in issuesBySubtype) { + var issuesOfSubtype = issuesBySubtype[issueSubtype]; + tags[prefix + ':' + issueType + ':' + issueSubtype] = issuesOfSubtype.length.toString().substr(0, 255); + } + } else { + tags[prefix + ':' + issueType] = issuesOfType.length.toString().substr(0, 255); + } + } + } + // add counts of warnings generated by the user's edits var warnings = context.validator() .getIssuesBySeverity({ what: 'edited', where: 'all', includeIgnored: true, includeDisabledRules: true }).warning; + addIssueCounts(warnings, 'warnings'); - var warningsByType = utilArrayGroupBy(warnings, 'type'); - for (var warningType in warningsByType) { - var warningsOfType = warningsByType[warningType]; - if (warningsOfType[0].subtype) { - var warningsBySubtype = utilArrayGroupBy(warningsOfType, 'subtype'); - for (var warningSubtype in warningsBySubtype) { - var warningsOfSubtype = warningsBySubtype[warningSubtype]; - tags['warnings:' + warningType + ':' + warningSubtype] = warningsOfSubtype.length.toString(); - } - } else { - tags['warnings:' + warningType] = warningsOfType.length.toString(); - } - } - + // add counts of issues resolved by the user's edits + var resolvedIssues = context.validator().getResolvedIssues(); + addIssueCounts(resolvedIssues, 'resolved'); _changeset = _changeset.update({ tags: tags }); diff --git a/modules/validations/crossing_ways.js b/modules/validations/crossing_ways.js index 08f2c7d4b..d07e951e2 100644 --- a/modules/validations/crossing_ways.js +++ b/modules/validations/crossing_ways.js @@ -12,19 +12,6 @@ import { validationIssue, validationIssueFix } from '../core/validation'; export function validationCrossingWays(context) { var type = 'crossing_ways'; - /* - Avoid duplicate work by cacheing issues. The same issues live under two paths. - { - w-123: { - w-456: [{issue1}, {issue2}…] - }, - w-456: { - w-123: [{issue1}, {issue2}…] - } - } - */ - var _issueCache = {}; - // returns the way or its parent relation, whichever has a useful feature type function getFeatureWithFeatureTypeTagsForWay(way, graph) { if (getFeatureTypeForTags(way.tags) === null) { @@ -252,9 +239,6 @@ export function validationCrossingWays(context) { // skip if this way was already checked and only one issue is needed if (checkedSingleCrossingWays[way2.id]) continue; - // don't re-check previously checked features - if (_issueCache[way1.id] && _issueCache[way1.id][way2.id]) continue; - // mark this way as checked even if there are no crossings comparedWays[way2.id] = true; @@ -295,12 +279,6 @@ export function validationCrossingWays(context) { } } } - for (var way2ID in comparedWays) { - if (!_issueCache[way1.id]) _issueCache[way1.id] = {}; - if (!_issueCache[way1.id][way2ID]) _issueCache[way1.id][way2ID] = []; - if (!_issueCache[way2ID]) _issueCache[way2ID] = {}; - if (!_issueCache[way2ID][way1.id]) _issueCache[way2ID][way1.id] = []; - } return edgeCrossInfos; } @@ -337,20 +315,11 @@ export function validationCrossingWays(context) { var issues = []; // declare these here to reduce garbage collection - var wayIndex, crossingIndex, key, crossings, crossing, issue; + var wayIndex, crossingIndex, crossings; for (wayIndex in ways) { - var way = ways[wayIndex]; - crossings = findCrossingsByWay(way, graph, tree); + crossings = findCrossingsByWay(ways[wayIndex], graph, tree); for (crossingIndex in crossings) { - crossing = crossings[crossingIndex]; - var way2 = crossing.ways[1]; - issue = createIssue(crossing, graph); - // cache the issues for each way - _issueCache[way.id][way2.id].push(issue); - _issueCache[way2.id][way.id].push(issue); - } - for (key in _issueCache[way.id]) { - issues = issues.concat(_issueCache[way.id][key]); + issues.push(createIssue(crossings[crossingIndex], graph)); } } return issues; @@ -457,9 +426,12 @@ export function validationCrossingWays(context) { connectionTags: connectionTags }, // differentiate based on the loc since two ways can cross multiple times - hash: JSON.stringify(crossing.crossPoint) + + hash: crossing.crossPoint.toString() + // if the edges change then so does the fix - JSON.stringify(crossing.edges) + + crossing.edges.slice().sort(function(edge1, edge2) { + // order to assure hash is deterministic + return edge1[0] < edge2[0] ? -1 : 1; + }).toString() + // ensure the correct connection tags are added in the fix JSON.stringify(connectionTags), loc: crossing.crossPoint, @@ -569,10 +541,6 @@ export function validationCrossingWays(context) { }); } - validation.reset = function() { - _issueCache = {}; - }; - validation.type = type; return validation;