From f6708fd84c22b606b407e278194da102a1b8d72a Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 15 Apr 2019 10:20:58 -0400 Subject: [PATCH] Hide feature if _all_ rules hidden (was: if _any_ rule hidden) This matters as we start to match more rules for hybrid features like rail platforms, which now match both path and rail. We want to show them unless the user has hidden all the rules that they match. Also this changes the test code slightly to actually test rule matching. Before it was really just testing hiding. --- modules/renderer/features.js | 15 +++---- modules/ui/preset_list.js | 4 +- test/spec/renderer/features.js | 78 +++++++++++++++------------------- 3 files changed, 42 insertions(+), 55 deletions(-) diff --git a/modules/renderer/features.js b/modules/renderer/features.js index 1640f0ac7..9924db6ed 100644 --- a/modules/renderer/features.js +++ b/modules/renderer/features.js @@ -413,8 +413,10 @@ export function rendererFeatures(context) { features.isHiddenPreset = function(preset, geometry) { if (!_hidden.length) return false; if (!preset.tags) return false; + + var test = preset.setTags({}, geometry); for (var key in _rules) { - if (_rules[key].filter(preset.setTags({}, geometry), geometry)) { + if (_rules[key].filter(test, geometry)) { if (_hidden.indexOf(key) !== -1) { return key; } @@ -429,12 +431,8 @@ export function rendererFeatures(context) { if (!_hidden.length) return false; if (!entity.version) return false; - var matches = features.getMatches(entity, resolver, geometry); - - for (var i = 0; i < _hidden.length; i++) { - if (matches[_hidden[i]]) return true; - } - return false; + var matches = Object.keys(features.getMatches(entity, resolver, geometry)); + return matches.every(function(k) { return features.hidden(k); }); }; @@ -456,8 +454,8 @@ export function rendererFeatures(context) { features.hasHiddenConnections = function(entity, resolver) { if (!_hidden.length) return false; - var childNodes, connections; + var childNodes, connections; if (entity.type === 'midpoint') { childNodes = [resolver.entity(entity.edge[0]), resolver.entity(entity.edge[1])]; connections = []; @@ -503,6 +501,7 @@ export function rendererFeatures(context) { features.forceVisible = function(entityIDs) { if (!arguments.length) return Object.keys(_forceVisible); + _forceVisible = {}; for (var i = 0; i < entityIDs.length; i++) { _forceVisible[entityIDs[i]] = true; diff --git a/modules/ui/preset_list.js b/modules/ui/preset_list.js index 5e025a3c5..e74404b01 100644 --- a/modules/ui/preset_list.js +++ b/modules/ui/preset_list.js @@ -428,19 +428,17 @@ export function uiPresetList(context) { return item; } - function updateForFeatureHiddenState() { + function updateForFeatureHiddenState() { if (!context.hasEntity(_entityID)) return; var geometry = context.geometry(_entityID); - var button = d3_selectAll('.preset-list .preset-list-button'); // remove existing tooltips button.call(tooltip().destroyAny); button.each(function(item, index) { - var hiddenPresetFeaturesId = context.features().isHiddenPreset(item.preset, geometry); var isHiddenPreset = !!hiddenPresetFeaturesId && item.preset !== _currentPreset; diff --git a/test/spec/renderer/features.js b/test/spec/renderer/features.js index 60397c930..2a72385e3 100644 --- a/test/spec/renderer/features.js +++ b/test/spec/renderer/features.js @@ -218,33 +218,34 @@ describe('iD.rendererFeatures', function() { var all = Object.values(graph.base().entities); - function doMatch(ids) { + function doMatch(rule, ids) { ids.forEach(function(id) { - var entity = graph.entity(id), - geometry = entity.geometry(graph); - expect(features.isHidden(entity, graph, geometry), 'doMatch: ' + id).to.be.true; + var entity = graph.entity(id); + var geometry = entity.geometry(graph); + expect(features.getMatches(entity, graph, geometry), 'doMatch: ' + id) + .to.have.property(rule); }); } - function dontMatch(ids) { + function dontMatch(rule, ids) { ids.forEach(function(id) { - var entity = graph.entity(id), - geometry = entity.geometry(graph); - expect(features.isHidden(entity, graph, geometry), 'dontMatch: ' + id).to.be.false; + var entity = graph.entity(id); + var geometry = entity.geometry(graph); + expect(features.getMatches(entity, graph, geometry), 'dontMatch: ' + id) + .not.to.have.property(rule); }); } it('matches points', function () { - features.disable('points'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('points', [ 'point_bar', 'point_dock', 'point_rail_station', 'point_generator', 'point_old_rail_station' ]); - dontMatch([ + dontMatch('points', [ 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -253,17 +254,16 @@ describe('iD.rendererFeatures', function() { it('matches traffic roads', function () { - features.disable('traffic_roads'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('traffic_roads', [ 'motorway', 'motorway_link', 'trunk', 'trunk_link', 'primary', 'primary_link', 'secondary', 'secondary_link', 'tertiary', 'tertiary_link', 'residential', 'living_street', 'unclassified', 'boundary_road', 'inner3' ]); - dontMatch([ + dontMatch('traffic_roads', [ 'point_bar', 'service', 'road', 'track', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -272,14 +272,13 @@ describe('iD.rendererFeatures', function() { it('matches service roads', function () { - features.disable('service_roads'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('service_roads', [ 'service', 'road', 'track' ]); - dontMatch([ + dontMatch('service_roads', [ 'point_bar', 'motorway', 'unclassified', 'living_street', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -288,15 +287,14 @@ describe('iD.rendererFeatures', function() { it('matches paths', function () { - features.disable('paths'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('paths', [ 'path', 'footway', 'cycleway', 'bridleway', 'steps', 'pedestrian', 'corridor' ]); - dontMatch([ + dontMatch('paths', [ 'point_bar', 'motorway', 'service', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -305,15 +303,14 @@ describe('iD.rendererFeatures', function() { it('matches buildings', function () { - features.disable('buildings'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('buildings', [ 'building_yes', 'building_part', 'garage1', 'garage2', 'garage3', 'garage4' ]); - dontMatch([ + dontMatch('buildings', [ 'building_no', 'point_bar', 'motorway', 'service', 'path', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -322,16 +319,15 @@ describe('iD.rendererFeatures', function() { it('matches landuse', function () { - features.disable('landuse'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('landuse', [ 'forest', 'scrub', 'industrial', 'parkinglot', 'building_no', 'rail_landuse', 'landuse_construction', 'retail', 'outer', 'inner1', 'inner2' // non-interesting members of landuse multipolygon ]); - dontMatch([ + dontMatch('landuse', [ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'fence', @@ -341,10 +337,9 @@ describe('iD.rendererFeatures', function() { it('matches boundaries', function () { - features.disable('boundaries'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('boundaries', [ 'boundary', // match ways that are part of boundary relations - #5601 'boundary_member', 'boundary_member2', @@ -352,7 +347,7 @@ describe('iD.rendererFeatures', function() { 'boundary_relation', 'boundary_relation2' ]); - dontMatch([ + dontMatch('boundaries', [ 'boundary_road', // because boundary also used as highway - #4973 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'water', 'railway', 'power_line', @@ -362,15 +357,14 @@ describe('iD.rendererFeatures', function() { it('matches water', function () { - features.disable('water'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('water', [ 'point_dock', 'water', 'coastline', 'bay', 'pond', 'basin', 'reservoir', 'salt_pond', 'river' ]); - dontMatch([ + dontMatch('water', [ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'railway', 'power_line', 'motorway_construction', 'fence' @@ -379,15 +373,14 @@ describe('iD.rendererFeatures', function() { it('matches rail', function () { - features.disable('rail'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('rail', [ 'point_rail_station', 'point_old_rail_station', 'railway', 'rail_landuse', 'rail_disused' ]); - dontMatch([ + dontMatch('rail', [ 'rail_streetcar', 'rail_trail', // because rail also used as highway 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'power_line', @@ -397,14 +390,13 @@ describe('iD.rendererFeatures', function() { it('matches power', function () { - features.disable('power'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('power', [ 'point_generator', 'power_line' ]); - dontMatch([ + dontMatch('power', [ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'motorway_construction', 'fence' @@ -413,15 +405,14 @@ describe('iD.rendererFeatures', function() { it('matches past/future', function () { - features.disable('past_future'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('past_future', [ 'point_old_rail_station', 'rail_disused', 'motorway_construction', 'cycleway_proposed', 'landuse_construction' ]); - dontMatch([ + dontMatch('past_future', [ 'rail_trail', // because rail also used as highway 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'fence' @@ -430,14 +421,13 @@ describe('iD.rendererFeatures', function() { it('matches others', function () { - features.disable('others'); features.gatherStats(all, graph, dimensions); - doMatch([ + doMatch('others', [ 'fence', 'pipeline' ]); - dontMatch([ + dontMatch('others', [ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'boundary_member', 'water', 'railway', 'power_line', 'motorway_construction', 'retail', 'outer', 'inner1', 'inner2', 'inner3'