From 35049ab40a60f88fea59a6c310763badff5028c5 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Thu, 11 Apr 2019 14:48:23 -0400 Subject: [PATCH] Replace `getErrors`/`getWarnings` with `getIssues`/`getIssuesBySeverity` The idea here is that the validator will now hold onto lots of issues, but the calling code will only want some small subset of them (edited/everything) (inview/everywhere) and can pass these as options so that we don't need filtering code spread throughtout the app. --- modules/core/validator.js | 42 +++++++++++++++++++++++++---------- modules/ui/commit.js | 39 +++++++++++++++++--------------- modules/ui/commit_warnings.js | 10 ++++----- modules/ui/issues.js | 29 ++++-------------------- 4 files changed, 59 insertions(+), 61 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index 9f95a4eb5..4c2364af7 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -3,7 +3,7 @@ import { dispatch as d3_dispatch } from 'd3-dispatch'; import { coreDifference } from './difference'; import { geoExtent } from '../geo'; import { osmEntity } from '../osm'; -import { utilRebind } from '../util'; +import { utilArrayGroupBy, utilRebind } from '../util'; import * as Validations from '../validations/index'; @@ -58,20 +58,38 @@ export function coreValidator(context) { }; - validator.getIssues = function() { - return Object.values(_issuesByIssueID); + // options = { + // what: 'edited', // 'all' or 'edited' + // where: 'visible' // 'all' or 'visible' + // }; + validator.getIssues = function(options) { + var opts = Object.assign({ what: 'all', where: 'all' }, options); + var issues = Object.values(_issuesByIssueID); + var changes = context.history().difference().changes(); + var view = context.map().extent(); + + return issues.filter(function(issue) { + if (opts.what === 'edited') { + var entities = issue.entities || []; + var isEdited = entities.some(function(entity) { return changes[entity.id]; }); + if (entities.length && !isEdited) return false; + } + + if (opts.where === 'visible') { + var extent = issue.extent(context.graph()); + if (!view.intersects(extent)) return false; + } + + return true; + }); }; - validator.getWarnings = function() { - return Object.values(_issuesByIssueID) - .filter(function(d) { return d.severity === 'warning'; }); - }; - - - validator.getErrors = function() { - return Object.values(_issuesByIssueID) - .filter(function(d) { return d.severity === 'error'; }); + validator.getIssuesBySeverity = function(options) { + var groups = utilArrayGroupBy(validator.getIssues(options), 'severity'); + groups.error = groups.error || []; + groups.warning = groups.warning || []; + return groups; }; diff --git a/modules/ui/commit.js b/modules/ui/commit.js index fae3910ea..5857d6586 100644 --- a/modules/ui/commit.js +++ b/modules/ui/commit.js @@ -3,17 +3,17 @@ import { select as d3_select } from 'd3-selection'; import deepEqual from 'fast-deep-equal'; import { t } from '../util/locale'; +import { modeBrowse } from '../modes'; import { osmChangeset } from '../osm'; +import { svgIcon } from '../svg'; import { services } from '../services'; +import { tooltip } from '../util/tooltip'; import { uiChangesetEditor } from './changeset_editor'; import { uiCommitChanges } from './commit_changes'; import { uiCommitWarnings } from './commit_warnings'; import { uiRawTagEditor } from './raw_tag_editor'; +import { utilArrayGroupBy, utilRebind } from '../util'; import { utilDetect } from '../util/detect'; -import { tooltip } from '../util/tooltip'; -import { utilRebind } from '../util'; -import { modeBrowse } from '../modes'; -import { svgIcon } from '../svg'; var _changeset; @@ -114,23 +114,24 @@ export function uiCommit(context) { } } + // remove existing warning counts for (var key in tags) { - // remove existing warning counts if (key.match(/^warnings:/)) delete tags[key]; } - var warningCountsByType = {}; - context.validator().getWarnings().forEach(function(warning) { - // deletion count can be derived so don't tag that warning in the changeset - if (warning.type === 'many_deletions') return; - if (!warningCountsByType[warning.type]) warningCountsByType[warning.type] = 0; - warningCountsByType[warning.type] += 1; - }); - for (var warningType in warningCountsByType) { - // tag the counts of warnings ignored by the user - tags['warnings:' + warningType] = warningCountsByType[warningType].toString(); + var warnings = context.validator() + .getIssuesBySeverity({ what: 'edited', where: 'all' }).warning; + var warningsByType = utilArrayGroupBy(warnings, 'type'); + + // deletion count can be derived so don't tag that warning in the changeset + delete warningsByType.many_deletions; + + // add tags for counts of warnings ignored by the user + for (var warningType in warningsByType) { + tags['warnings:' + warningType] = warningsByType[warningType].length.toString(); } + _changeset = _changeset.update({ tags: tags }); var header = selection.selectAll('.header') @@ -354,9 +355,11 @@ export function uiCommit(context) { function getUploadBlockerMessage() { - var errorCount = context.validator().getErrors().length; - if (errorCount > 0) { - return t('commit.outstanding_errors_message', { count: errorCount }); + var errors = context.validator() + .getIssuesBySeverity({ what: 'edited', where: 'all' }).error; + + if (errors.length) { + return t('commit.outstanding_errors_message', { count: errors.length }); } else { var n = d3_select('#preset-input-comment').node(); diff --git a/modules/ui/commit_warnings.js b/modules/ui/commit_warnings.js index 568953b7c..bf75f30c6 100644 --- a/modules/ui/commit_warnings.js +++ b/modules/ui/commit_warnings.js @@ -4,14 +4,12 @@ import { svgIcon } from '../svg'; import { tooltip } from '../util/tooltip'; import { utilEntityOrMemberSelector } from '../util'; + export function uiCommitWarnings(context) { function commitWarnings(selection) { - - var issuesBySeverity = { - warning: context.validator().getWarnings(), - error: context.validator().getErrors() - }; + var issuesBySeverity = context.validator() + .getIssuesBySeverity({ what: 'edited', where: 'all' }); for (var severity in issuesBySeverity) { var issues = issuesBySeverity[severity]; @@ -41,7 +39,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.id; }); items.exit() .remove(); diff --git a/modules/ui/issues.js b/modules/ui/issues.js index d346cd830..aabc86fea 100644 --- a/modules/ui/issues.js +++ b/modules/ui/issues.js @@ -11,7 +11,7 @@ import { uiDisclosure } from './disclosure'; import { uiHelp } from './help'; import { uiMapData } from './map_data'; import { uiTooltipHtml } from './tooltipHtml'; -import { utilArrayGroupBy, utilCallWhenIdle, utilHighlightEntities } from '../util'; +import { utilCallWhenIdle, utilHighlightEntities } from '../util'; export function uiIssues(context) { @@ -264,30 +264,9 @@ export function uiIssues(context) { function update() { - var issues = context.validator().getIssues(); - var changes = context.history().difference().changes(); - var view = context.map().extent(); - - // filter - issues = issues.filter(function(issue) { - if (_options.what === 'edited') { - var entities = issue.entities || []; - var isEdited = entities.some(function(entity) { return changes[entity.id]; }); - if (entities.length && !isEdited) return false; - } - - if (_options.where === 'visible') { - var extent = issue.extent(context.graph()); - if (!view.intersects(extent)) return false; - } - - return true; - }); - - - var groups = utilArrayGroupBy(issues, 'severity'); - _errors = groups.error || []; - _warnings = groups.warning || []; + var issuesBySeverity = context.validator().getIssuesBySeverity(_options); + _errors = issuesBySeverity.error; + _warnings = issuesBySeverity.warning; _toggleButton.selectAll('.icon-badge')