From 01d2e3eaf30f90f6d87f849736bdb6dfdf90ae09 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 16 Apr 2019 16:56:50 -0400 Subject: [PATCH] Replace validator `tooltip` with `reference` function, add tag diff --- css/80_app.css | 26 ++++++- data/core.yaml | 1 + dist/locales/en.json | 1 + modules/core/validator.js | 2 +- modules/ui/entity_issues.js | 5 +- modules/ui/issues.js | 9 +-- modules/ui/map_in_map.js | 2 +- modules/validations/almost_junction.js | 11 ++- modules/validations/crossing_ways.js | 14 +++- modules/validations/disconnected_way.js | 12 +++- modules/validations/generic_name.js | 12 +++- modules/validations/incompatible_source.js | 15 +++- modules/validations/many_deletions.js | 13 +++- modules/validations/missing_role.js | 14 +++- modules/validations/missing_tag.js | 13 +++- modules/validations/old_multipolygon.js | 14 +++- modules/validations/outdated_tags.js | 39 ++++++++-- modules/validations/private_data.js | 82 +++++++++++++++------- modules/validations/tag_suggests_area.js | 12 +++- modules/validations/unknown_road.js | 12 +++- 20 files changed, 255 insertions(+), 54 deletions(-) diff --git a/css/80_app.css b/css/80_app.css index 03bcf28d6..85c09cb12 100644 --- a/css/80_app.css +++ b/css/80_app.css @@ -3360,12 +3360,36 @@ li.issue-fix-item:not(.actionable) .fix-icon { width: 100%; overflow: hidden; display: none; - padding: 10px; + padding: 10px 0; } .issue-info.expanded { display: inline-block; } +.issue-info .issue-reference { + margin-bottom: 10px; +} +.issue-info .tagDiff-table { + min-width: 60%; + width: unset; + border: 1px solid #ccc; +} +.issue-info .tagDiff-row { + border: 1px solid #ccc; +} +.issue-info .tagDiff-cell { + padding: 2px 5px; + font-family: monospace; + font-size: 10px; + border: 1px solid #ccc; +} +.issue-info .tagDiff-cell-add { + background: #dfd; +} +.issue-info .tagDiff-cell-remove { + background: #fdd; +} + /* Background - Display Options Sliders ------------------------------------------------------- */ diff --git a/data/core.yaml b/data/core.yaml index 8be40d3e8..f0988cd67 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -1269,6 +1269,7 @@ en: title: "Where:" visible: "In View" all: "Everywhere" + suggested: "Suggested updates:" almost_junction: title: Almost Junctions message: "{feature} is very close but not connected to {feature2}" diff --git a/dist/locales/en.json b/dist/locales/en.json index 8b7ffb9f4..89c866465 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -1548,6 +1548,7 @@ "all": "Everywhere" } }, + "suggested": "Suggested updates:", "almost_junction": { "title": "Almost Junctions", "message": "{feature} is very close but not connected to {feature2}", diff --git a/modules/core/validator.js b/modules/core/validator.js index 8c335a66b..d5be80a78 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -334,7 +334,7 @@ export function validationIssue(attrs) { this.type = attrs.type; // required this.severity = attrs.severity; // required - 'warning' or 'error' this.message = attrs.message; // required - localized string - this.tooltip = attrs.tooltip; // required - localized string + this.reference = attrs.reference; // required - function(selection) to render reference info this.entities = attrs.entities; // optional - array of entities this.loc = attrs.loc; // optional - expect a [lon, lat] array this.data = attrs.data; // optional - object containing extra data for the fixes diff --git a/modules/ui/entity_issues.js b/modules/ui/entity_issues.js index b49cdf5ca..352f21da9 100644 --- a/modules/ui/entity_issues.js +++ b/modules/ui/entity_issues.js @@ -159,7 +159,10 @@ export function uiEntityIssues(context) { .attr('class', 'issue-info') .style('max-height', '0') .style('opacity', '0') - .html(function(d) { return d.tooltip; }); + .each(function(d) { + d3_select(this) + .call(d.reference); + }); // Update diff --git a/modules/ui/issues.js b/modules/ui/issues.js index ae51570e5..146181c09 100644 --- a/modules/ui/issues.js +++ b/modules/ui/issues.js @@ -93,11 +93,8 @@ export function uiIssues(context) { .on('click', function(d) { var extent = d.extent(context.graph()); if (extent) { - var extent = d.extent(context.graph()); - if (extent) { - var setZoom = Math.max(context.map().zoom(), 19); - context.map().centerZoomEase(extent.center(), setZoom); - } + var setZoom = Math.max(context.map().zoom(), 19); + context.map().centerZoomEase(extent.center(), setZoom); // select the first entity if (d.entities && d.entities.length) { @@ -134,7 +131,7 @@ export function uiIssues(context) { var iconName = '#iD-icon-' + (d.severity === 'warning' ? 'alert' : 'error'); d3_select(this) .call(svgIcon(iconName)); - }) + }); textEnter .append('span') diff --git a/modules/ui/map_in_map.js b/modules/ui/map_in_map.js index 29919f0c7..7d88928f9 100644 --- a/modules/ui/map_in_map.js +++ b/modules/ui/map_in_map.js @@ -7,7 +7,7 @@ import { geoRawMercator, geoScaleToZoom, geoVecSubtract, geoVecScale, geoZoomToS import { rendererTileLayer } from '../renderer'; import { svgDebug, svgData } from '../svg'; import { utilSetTransform } from '../util'; -import { utilGetDimensions } from '../util/dimensions'; +// import { utilGetDimensions } from '../util/dimensions'; export function uiMapInMap(context) { diff --git a/modules/validations/almost_junction.js b/modules/validations/almost_junction.js index 7d0bf618c..6045498ff 100644 --- a/modules/validations/almost_junction.js +++ b/modules/validations/almost_junction.js @@ -91,7 +91,7 @@ export function validationAlmostJunction() { feature: utilDisplayLabel(entity, context), feature2: utilDisplayLabel(edgeHighway, context) }), - tooltip: t('issues.almost_junction.highway-highway.tip'), + reference: showReference, entities: [entity, node, edgeHighway], loc: extendableNodeInfo.node.loc, data: { @@ -105,6 +105,15 @@ export function validationAlmostJunction() { return issues; + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.almost_junction.highway-highway.tip')); + } + function isExtendableCandidate(node, way) { // can not accurately test vertices on tiles not downloaded from osm - #5938 diff --git a/modules/validations/crossing_ways.js b/modules/validations/crossing_ways.js index 68f6ce295..720a5e5c1 100644 --- a/modules/validations/crossing_ways.js +++ b/modules/validations/crossing_ways.js @@ -224,7 +224,6 @@ export function validationCrossingWays() { function findCrossingsByWay(way1, graph, tree) { - var edgeCrossInfos = []; if (way1.type !== 'way') return edgeCrossInfos; @@ -467,6 +466,7 @@ export function validationCrossingWays() { } })); } + var useFixIcon = 'iD-icon-layers'; var useFixID; if (isCrossingIndoors) { @@ -496,11 +496,12 @@ export function validationCrossingWays() { icon: 'iD-operation-move', title: t('issues.fix.reposition_features.title') })); + return new validationIssue({ type: type, severity: 'warning', message: t('issues.crossing_ways.message', messageDict), - tooltip: t('issues.crossing_ways.' + crossingTypeID + '.tip'), + reference: showReference, entities: entities, data: { edges: crossing.edges, @@ -509,6 +510,15 @@ export function validationCrossingWays() { loc: crossing.crossPoint, fixes: fixes }); + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.crossing_ways.' + crossingTypeID + '.tip')); + } } function makeChangeLayerFix(higherOrLower, context) { diff --git a/modules/validations/disconnected_way.js b/modules/validations/disconnected_way.js index 0e425321a..ceda3020d 100644 --- a/modules/validations/disconnected_way.js +++ b/modules/validations/disconnected_way.js @@ -77,12 +77,22 @@ export function validationDisconnectedWay() { type: type, severity: 'warning', message: t('issues.disconnected_way.highway.message', { highway: entityLabel }), - tooltip: t('issues.disconnected_way.highway.tip'), + reference: showReference, entities: [entity], fixes: fixes })]; + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.disconnected_way.highway.tip')); + } + + function vertexIsDisconnected(way, vertex, relation) { // can not accurately test vertices on tiles not downloaded from osm - #5938 var osm = context.connection(); diff --git a/modules/validations/generic_name.js b/modules/validations/generic_name.js index 536b1027d..af9cee9a9 100644 --- a/modules/validations/generic_name.js +++ b/modules/validations/generic_name.js @@ -58,7 +58,7 @@ export function validationGenericName() { type: type, severity: 'warning', message: t('issues.generic_name.message', {feature: preset.name(), name: generic}), - tooltip: t('issues.generic_name.tip'), + reference: showReference, entities: [entity], fixes: [ new validationIssueFix({ @@ -76,6 +76,16 @@ export function validationGenericName() { }) ] })]; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.generic_name.tip')); + } }; validation.type = type; diff --git a/modules/validations/incompatible_source.js b/modules/validations/incompatible_source.js index e26ded2aa..f1996566e 100644 --- a/modules/validations/incompatible_source.js +++ b/modules/validations/incompatible_source.js @@ -21,7 +21,7 @@ export function validationIncompatibleSource() { message: t('issues.incompatible_source.' + invalidSource.id + '.feature.message', { feature: utilDisplayLabel(entity, context), }), - tooltip: t('issues.incompatible_source.' + invalidSource.id + '.tip'), + reference: getReference(invalidSource.id), entities: [entity], fixes: [ new validationIssueFix({ @@ -32,7 +32,20 @@ export function validationIncompatibleSource() { } }); } + return issues; + + + function getReference(id) { + return function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.incompatible_source.' + id + '.tip')); + }; + } }; validation.type = type; diff --git a/modules/validations/many_deletions.js b/modules/validations/many_deletions.js index a42f3521b..95f9a77d3 100644 --- a/modules/validations/many_deletions.js +++ b/modules/validations/many_deletions.js @@ -43,14 +43,25 @@ export function validationManyDeletions() { 'issues.many_deletions.'+messageType+'.message', { n: totalFeatures, p: points, l: lines, a:areas, r: relations } ), - tooltip: t('issues.many_deletions.tip'), + reference: showReference, hash: [points, lines, areas, relations].join() })]; } return []; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.many_deletions.tip')); + } }; + validation.type = type; validation.inputType = 'changes'; diff --git a/modules/validations/missing_role.js b/modules/validations/missing_role.js index ec6ff08bd..e848575c7 100644 --- a/modules/validations/missing_role.js +++ b/modules/validations/missing_role.js @@ -35,6 +35,7 @@ export function validationMissingRole() { return !member.role || !member.role.trim().length; } + function makeIssue(way, relation, member, context) { return new validationIssue({ type: type, @@ -43,7 +44,7 @@ export function validationMissingRole() { member: utilDisplayLabel(way, context), relation: utilDisplayLabel(relation, context), }), - tooltip: t('issues.missing_role.multipolygon.tip'), + reference: showReference, entities: [relation, way], data: { member: member @@ -63,8 +64,19 @@ export function validationMissingRole() { }) ] }); + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.missing_role.multipolygon.tip')); + } } + function makeAddRoleFix(role, context) { return new validationIssueFix({ title: t('issues.fix.set_as_' + role + '.title'), diff --git a/modules/validations/missing_tag.js b/modules/validations/missing_tag.js index 9842aef5b..8b0a6fa05 100644 --- a/modules/validations/missing_tag.js +++ b/modules/validations/missing_tag.js @@ -87,12 +87,23 @@ export function validationMissingTag() { type: type, severity: isError ? 'error' : 'warning', message: t('issues.missing_tag.' + missingTagType + '.message', messageObj), - tooltip: t('issues.missing_tag.tip'), + reference: showReference, entities: [entity], fixes: fixes })]; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.missing_tag.tip')); + } }; + validation.type = type; return validation; diff --git a/modules/validations/old_multipolygon.js b/modules/validations/old_multipolygon.js index c66dbc10e..e87683996 100644 --- a/modules/validations/old_multipolygon.js +++ b/modules/validations/old_multipolygon.js @@ -24,12 +24,13 @@ export function validationOldMultipolygon() { } if (!multipolygon || !outerWay) return []; + var multipolygonLabel = utilDisplayLabel(multipolygon, context); return [new validationIssue({ type: type, severity: 'warning', message: t('issues.old_multipolygon.message', { multipolygon: multipolygonLabel }), - tooltip: t('issues.old_multipolygon.tip'), + reference: showReference, entities: [outerWay, multipolygon], fixes: [ new validationIssueFix({ @@ -51,8 +52,19 @@ export function validationOldMultipolygon() { }) ] })]; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.old_multipolygon.tip')); + } }; + validation.type = type; return validation; diff --git a/modules/validations/outdated_tags.js b/modules/validations/outdated_tags.js index 85878a61c..2e8ae5ec3 100644 --- a/modules/validations/outdated_tags.js +++ b/modules/validations/outdated_tags.js @@ -57,19 +57,13 @@ export function validationOutdatedTags() { if (!tagDiff.length) return []; - - // debugging (sorta) - var tooltip = '
\n' + tagDiff.join('\n') + '
'; - - return [new validationIssue({ type: type, severity: 'warning', message: t('issues.outdated_tags.message', { feature: utilDisplayLabel(entity, context) }), - tooltip: tooltip, // t('issues.outdated_tags.tip'), + reference: showReference, entities: [entity], data: { - tagDiff: tagDiff, newTags: newTags }, fixes: [ @@ -87,6 +81,37 @@ export function validationOutdatedTags() { }) ] })]; + + + function showReference(selection) { + var enter = selection.selectAll('.issue-reference') + .data([0]) + .enter(); + + enter + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.outdated_tags.tip')); + + enter + .append('strong') + .text(t('issues.suggested')); + + enter + .append('table') + .attr('class', 'tagDiff-table') + .selectAll('.tagDiff-row') + .data(tagDiff) + .enter() + .append('tr') + .attr('class', 'tagDiff-row') + .append('td') + .attr('class', function(d) { + var klass = d.charAt(0) === '+' ? 'add' : 'remove'; + return 'tagDiff-cell tagDiff-cell-' + klass; + }) + .text(function(d) { return d; }); + } }; diff --git a/modules/validations/private_data.js b/modules/validations/private_data.js index 2f4150a61..ee24cd55d 100644 --- a/modules/validations/private_data.js +++ b/modules/validations/private_data.js @@ -18,7 +18,7 @@ export function validationPrivateData() { }; // but they might be public if they have one of these other tags - var okayModifierKeys = { + var publicKeys = { amenity: true, craft: true, historic: true, @@ -39,55 +39,87 @@ export function validationPrivateData() { website: true }; - function privateDataKeys(entity) { - var tags = entity.tags; - if (!tags.building || !privateBuildingValues[tags.building]) return []; - var privateKeys = []; - for (var key in tags) { - if (okayModifierKeys[key]) return []; - if (personalTags[key]) privateKeys.push(key); - } - return privateKeys; - } var validation = function checkPrivateData(entity, context) { - var privateKeys = privateDataKeys(entity); - if (privateKeys.length === 0) return []; + var tags = entity.tags; + var keepTags = {}; + var tagDiff = []; + if (!tags.building || !privateBuildingValues[tags.building]) return []; + + for (var k in tags) { + if (publicKeys[k]) return []; // probably a public feature + + if (personalTags[k]) { + tagDiff.push('- ' + k + '=' + tags[k]); + } else { + keepTags[k] = tags[k]; + } + } + + if (!tagDiff.length) return []; + + var fixID = tagDiff.length === 1 ? 'remove_tag' : 'remove_tags'; - var fixID = privateKeys.length === 1 ? 'remove_tag' : 'remove_tags'; return [new validationIssue({ type: type, severity: 'warning', message: t('issues.private_data.contact.message', { feature: utilDisplayLabel(entity, context), }), - tooltip: t('issues.private_data.tip'), + reference: showReference, entities: [entity], data: { - privateKeys: privateKeys + newTags: keepTags }, fixes: [ new validationIssueFix({ - auto: true, icon: 'iD-operation-delete', title: t('issues.fix.' + fixID + '.title'), onClick: function() { - var entity = this.issue.entities[0]; - var tags = Object.assign({}, entity.tags); // shallow copy - var privateKeys = this.issue.data.privateKeys; - for (var index in privateKeys) { - delete tags[privateKeys[index]]; - } + var entityID = this.issue.entities[0].id; + var newTags = this.issue.data.newTags; context.perform( - actionChangeTags(entity.id, tags), - t('issues.fix.remove_private_info.annotation') + actionChangeTags(entityID, newTags), + t('issues.fix.upgrade_tags.annotation') ); } }) ] })]; + + + function showReference(selection) { + var enter = selection.selectAll('.issue-reference') + .data([0]) + .enter(); + + enter + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.private_data.tip')); + + enter + .append('strong') + .text(t('issues.suggested')); + + enter + .append('table') + .attr('class', 'tagDiff-table') + .selectAll('.tagDiff-row') + .data(tagDiff) + .enter() + .append('tr') + .attr('class', 'tagDiff-row') + .append('td') + .attr('class', function(d) { + var klass = d.charAt(0) === '+' ? 'add' : 'remove'; + return 'tagDiff-cell tagDiff-cell-' + klass; + }) + .text(function(d) { return d; }); + } }; + validation.type = type; return validation; diff --git a/modules/validations/tag_suggests_area.js b/modules/validations/tag_suggests_area.js index 6ed0378f6..dc30aaddb 100644 --- a/modules/validations/tag_suggests_area.js +++ b/modules/validations/tag_suggests_area.js @@ -95,10 +95,20 @@ export function validationTagSuggestsArea() { type: type, severity: 'warning', message: t('issues.tag_suggests_area.message', { feature: featureLabel, tag: tagText }), - tooltip: t('issues.tag_suggests_area.tip'), + reference: showReference, entities: [entity], fixes: fixes })]; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.tag_suggests_area.tip')); + } }; validation.type = type; diff --git a/modules/validations/unknown_road.js b/modules/validations/unknown_road.js index 17bd32263..169c7dc50 100644 --- a/modules/validations/unknown_road.js +++ b/modules/validations/unknown_road.js @@ -42,10 +42,20 @@ export function validationUnknownRoad() { message: t('issues.unknown_road.message', { feature: utilDisplayLabel(entity, context), }), - tooltip: t('issues.unknown_road.tip'), + reference: showReference, entities: [entity], fixes: fixes })]; + + + function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.unknown_road.tip')); + } }; validation.type = type;