From 00c8ff4f6911c48250889cd47b79e4893ce93386 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 1 Jul 2018 22:50:28 -0400 Subject: [PATCH 1/4] Don't add notes to the _seenEntity cache re: https://github.com/openstreetmap/iD/commit/229484a940eeb1d8ef08495c39642276c599c323#commitcomment-29560519 --- modules/services/osm.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/modules/services/osm.js b/modules/services/osm.js index 037d23003..d0a9b933b 100644 --- a/modules/services/osm.js +++ b/modules/services/osm.js @@ -237,11 +237,11 @@ function parse(xml, callback, options) { uid = child.getElementsByTagName('id')[0].textContent; } else { uid = osmEntity.id.fromOSM(child.nodeName, child.attributes.id.value); + if (options.cache && _seenEntity[uid]) { + return null; // avoid reparsing a "seen" entity + } } - if (options.cache && _seenEntity[uid]) { - return null; - } return parser(child, uid); } @@ -326,7 +326,7 @@ export default { // Logout and retry the request.. if (isAuthenticated && err && (err.status === 400 || err.status === 401 || err.status === 403)) { that.logout(); - that.loadFromAPI(path, callback); + that.loadFromAPI(path, callback, options); // else, no retry.. } else { @@ -343,7 +343,7 @@ export default { parse(xml, function (entities) { if (options.cache) { for (var i in entities) { - _seenEntity[entities[i].id] = true; + _seenEntity[entities[i].id] = true; // avoid re-parsing again later } } callback(null, entities); @@ -707,6 +707,7 @@ export default { dispatch.call('loading'); // start the spinner } + var options = { cache: !loadingNotes }; cache.inflight[id] = that.loadFromAPI( path + tile.extent.toParam(), function(err, parsed) { @@ -726,8 +727,8 @@ export default { dispatch.call('loaded'); // stop the spinner } } - - } + }, + options ); }); }, From 535208beb5c6500d3cb8ed6d490aa54fc48c2ad6 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 1 Jul 2018 23:35:10 -0400 Subject: [PATCH 2/4] Move tests to spec/services/osm.js and remove old notes.js files --- modules/services/index.js | 3 - modules/services/notes.js | 298 ------------------------------------ test/index.html | 1 - test/spec/services/notes.js | 90 ----------- test/spec/services/osm.js | 62 +++++++- 5 files changed, 55 insertions(+), 399 deletions(-) delete mode 100644 modules/services/notes.js delete mode 100644 test/spec/services/notes.js diff --git a/modules/services/index.js b/modules/services/index.js index cd38a0890..789628ed5 100644 --- a/modules/services/index.js +++ b/modules/services/index.js @@ -1,6 +1,5 @@ import serviceMapillary from './mapillary'; import serviceNominatim from './nominatim'; -import serviceNotes from './notes'; import serviceOpenstreetcam from './openstreetcam'; import serviceOsm from './osm'; import serviceStreetside from './streetside'; @@ -11,7 +10,6 @@ import serviceWikipedia from './wikipedia'; export var services = { geocoder: serviceNominatim, mapillary: serviceMapillary, - notes: serviceNotes, openstreetcam: serviceOpenstreetcam, osm: serviceOsm, streetside: serviceStreetside, @@ -23,7 +21,6 @@ export var services = { export { serviceMapillary, serviceNominatim, - serviceNotes, serviceOpenstreetcam, serviceOsm, serviceStreetside, diff --git a/modules/services/notes.js b/modules/services/notes.js deleted file mode 100644 index a78c8e9ef..000000000 --- a/modules/services/notes.js +++ /dev/null @@ -1,298 +0,0 @@ -import _extend from 'lodash-es/extend'; -import _filter from 'lodash-es/filter'; -import _find from 'lodash-es/find'; -import _forEach from 'lodash-es/forEach'; -import _isEmpty from 'lodash-es/isEmpty'; - -import osmAuth from 'osm-auth'; - -import rbush from 'rbush'; - -var _entityCache = {}; - -import { dispatch as d3_dispatch } from 'd3-dispatch'; -import { xml as d3_xml } from 'd3-request'; - -import { d3geoTile as d3_geoTile } from '../lib/d3.geo.tile'; -import { geoExtent } from '../geo'; - -import { - utilRebind, - utilIdleWorker -} from '../util'; - -import { - osmNote -} from '../osm'; -import { actionRestrictTurn } from '../actions'; - -var urlroot = 'https://api.openstreetmap.org', - _notesCache, - dispatch = d3_dispatch('loadedNotes', 'loading'), - tileZoom = 14; - -// TODO: complete authentication -var oauth = osmAuth({ - url: urlroot, - oauth_consumer_key: '', - oauth_secret: '', - loading: authLoading, - done: authDone -}); - -function authLoading() { - dispatch.call('authLoading'); -} - -function authDone() { - dispatch.call('authDone'); -} - -function authenticated() { - return oauth.authenticated(); -} - -function abortRequest(i) { - i.abort(); -} - -function getTiles(projection) { - var s = projection.scale() * 2 * Math.PI, - z = Math.max(Math.log(s) / Math.log(2) - 8, 0), - ts = 256 * Math.pow(2, z - tileZoom), - origin = [ - s / 2 - projection.translate()[0], - s / 2 - projection.translate()[1]]; - - var tiles = d3_geoTile() - .scaleExtent([tileZoom, tileZoom]) - .scale(s) - .size(projection.clipExtent()[1]) - .translate(projection.translate())() - .map(function(tile) { - var x = tile[0] * ts - origin[0], - y = tile[1] * ts - origin[1]; - - return { - id: tile.toString(), - xyz: tile, - extent: geoExtent( - projection.invert([x, y + ts]), - projection.invert([x + ts, y]) - ) - }; - }); - - return tiles; -} - -function getLoc(attrs) { - var lon = attrs.lon && attrs.lon.value; - var lat = attrs.lat && attrs.lat.value; - return [parseFloat(lon), parseFloat(lat)]; -} - -function parseComments(comments) { - var parsedComments = []; - - // for each comment - _forEach(comments, function(comment) { - if (comment.nodeName === 'comment') { - var childNodes = comment.childNodes; - var parsedComment = {}; - - _forEach(childNodes, function(node) { - if (node.nodeName !== '#text') { - var nodeName = node.nodeName; - parsedComment[nodeName] = node.innerHTML; - } - }); - if (parsedComment) { parsedComments.push(parsedComment); } - } - }); - return parsedComments; -} - -var parsers = { - note: function parseNote(obj, uid) { - var attrs = obj.attributes; - var childNodes = obj.childNodes; - var parsedNote = {}; - - parsedNote.loc = getLoc(attrs); - - _forEach(childNodes, function(node) { - if (node.nodeName !== '#text') { - var nodeName = node.nodeName; - // if the element is comments, parse the comments - if (nodeName === 'comments') { - parsedNote[nodeName] = parseComments(node.childNodes); - } else { - parsedNote[nodeName] = node.innerHTML; - } - } - }); - - parsedNote.id = uid; - parsedNote.type = 'note'; - - return { - minX: parsedNote.loc[0], - minY: parsedNote.loc[1], - maxX: parsedNote.loc[0], - maxY: parsedNote.loc[1], - data: new osmNote(parsedNote) - }; - } -}; - -function parse(xml, callback, options) { - options = _extend({ cache: true }, options); - if (!xml || !xml.childNodes) return; - - var root = xml.childNodes[0]; - var children = root.childNodes; - - function parseChild(child) { - var parser = parsers[child.nodeName]; - if (parser) { - - var childNodes = child.childNodes; - - var uid; - _forEach(childNodes, function(node) { - if (node.nodeName === 'id') { - uid = child.nodeName + node.innerHTML; - } - }); - - if (options.cache && _entityCache[uid]) { - return null; - } - return parser(child, uid); - } - } - utilIdleWorker(children, parseChild, callback); -} - -export default { - - init: function() { - if (!_notesCache) { - this.reset(); - } - - this.event = utilRebind(this, dispatch, 'on'); - }, - - reset: function() { - var cache = _notesCache; - - if (cache) { - if (cache.notes && cache.notes.inflight) { - _forEach(cache.notes.inflight, abortRequest); - } - } - - _notesCache = { notes: { inflight: {}, loaded: {}, rtree: rbush() } }; - }, - - loadFromAPI: function(path, callback, options) { - options = _extend({ cache: true }, options); - - function done(err, xml) { - if (err) { - callback(err, xml); - } - parse( - xml, - function(entities) { - if (options.cache) { - for (var i in entities) { - _entityCache[entities[i].id] = true; - } - } - callback(null, entities); - }, - options - ); - } - - if (authenticated()) { - return oauth.xhr({ method: 'GET', path: path }, done); - } else { - return d3_xml(path).get(done); - } - }, - - // TODO: refactor /services for consistency by splitting or joining loadTiles & loadTile - loadTile: function(which, currZoom, url, tile) { - var that = this; - var cache = _notesCache[which]; - var bbox = tile.extent.toParam(); - var fullUrl = url + bbox; - - var id = tile.id; - - if (cache.loaded[id] || cache.inflight[id]) return; - - if (_isEmpty(cache.inflight)) { - dispatch.call('loading'); - } - - cache.inflight[id] = that.loadFromAPI( - fullUrl, - function (err, parsed) { - delete cache.inflight[id]; - if (!err) { - cache.loaded[id] = true; - } - - cache.rtree.load(parsed); - - if (_isEmpty(cache.inflight)) { - dispatch.call('loadedNotes'); - } - }, - [] - ); - }, - - loadTiles: function(which, url, projection) { - var that = this; - var s = projection.scale() * 2 * Math.PI, - currZoom = Math.floor(Math.max(Math.log(s) / Math.log(2) - 8, 0)); - - var tiles = getTiles(projection); - - _filter(which.inflight, function(v, k) { - var wanted = _find(tiles, function(tile) { return k === (tile.id + ',0'); }); - if (!wanted) delete which.inflight[k]; - return !wanted; - }).map(abortRequest); - - tiles.forEach(function(tile) { - that.loadTile(which, currZoom, url, tile); - }); - }, - - loadNotes: function(projection) { - var that = this; - var url = urlroot + '/api/0.6/notes?bbox='; - that.loadTiles('notes', url, projection); - }, - - notes: function(projection) { - var viewport = projection.clipExtent(); - var min = [viewport[0][0], viewport[1][1]]; - var max = [viewport[1][0], viewport[0][1]]; - var bbox = geoExtent(projection.invert(min), projection.invert(max)).bbox(); - - return _notesCache.notes.rtree.search(bbox) - .map(function(d) { return d.data; }); - }, - - cache: function() { - return _notesCache; - } -}; \ No newline at end of file diff --git a/test/index.html b/test/index.html index e31966fd8..188b9084c 100644 --- a/test/index.html +++ b/test/index.html @@ -104,7 +104,6 @@ - diff --git a/test/spec/services/notes.js b/test/spec/services/notes.js deleted file mode 100644 index 72d342a0b..000000000 --- a/test/spec/services/notes.js +++ /dev/null @@ -1,90 +0,0 @@ -describe('iD.serviceNotes', function () { - var dimensions = [64, 64], - context, server, notes; - - - before(function() { - iD.services.notes = iD.serviceNotes; - }); - - after(function() { - delete iD.services.notes; - }); - - beforeEach(function() { - context = iD.Context().assetPath('../dist/'); - context.projection - .scale(667544.214430109) // z14 - .translate([-116508, 0]) // 10,0 - .clipExtent([[0,0], dimensions]); - - server = sinon.fakeServer.create(); - notes = iD.services.notes; - notes.reset(); - }); - - afterEach(function() { - server.restore(); - }); - - describe('#init', function () { - it('Initializes cache one time', function () { - var cache = notes.cache(); - expect(cache).to.have.property('notes'); - - notes.init(); - var cache2 = notes.cache(); - expect(cache).to.equal(cache2); - }); - }); - - describe('#reset', function () { - it('resets cache', function () { - notes.cache.foo = 'bar'; - notes.reset(); - expect(notes.cache()).to.not.have.property('foo'); - }); - }); - - describe('#loadFromAPI', function () { - var path = '/api/0.6/notes?bbox=-0.65094,51.312159,0.374908,51.3125', - response = '' + - '' + - '' + - '814798' + - 'https://api.openstreetmap.org/api/0.6/notes/814798' + - 'https://api.openstreetmap.org/api/0.6/notes/814798/comment' + - 'https://api.openstreetmap.org/api/0.6/notes/814798/close' + - '2016-12-13 11:02:44 UTC' + - 'open' + - '' + - '' + - '2016-12-13 11:02:44 UTC' + - 'opened' + - 'Otford Scout Hut' + - '<p>Otford Scout Hut</p>' + - '' + - '' + - '' + - ''; - - it('returns an object', function (done) { - var result = notes.loadFromAPI( - 'https://www.openstreetmap.org' + path, - function (err, xml) { - expect(err).to.not.be.ok; - expect(typeof xml).to.eql('object'); - done(); - }, - []); - - // TODO: clarify why this throws an error - // server.respondWith('GET', 'http://www.openstreetmap.org' + path, - // [200, { 'Content-Type': 'text/xml' }, response]); - // server.respond(); - - done(); - }); - }); - -}); \ No newline at end of file diff --git a/test/spec/services/osm.js b/test/spec/services/osm.js index 3f51916f1..9e2832b92 100644 --- a/test/spec/services/osm.js +++ b/test/spec/services/osm.js @@ -137,8 +137,8 @@ describe('iD.serviceOsm', function () { }); describe('#loadFromAPI', function () { - var path = '/api/0.6/map?bbox=-74.542,40.655,-74.541,40.656', - response = '' + + var path = '/api/0.6/map?bbox=-74.542,40.655,-74.541,40.656'; + var response = '' + '' + ' ' + @@ -293,11 +293,58 @@ describe('iD.serviceOsm', function () { }); + + describe('#loadNotes', function () { + var path = '/api/0.6/notes?bbox=-0.65094,51.312159,0.374908,51.3125'; + var response = '' + + '' + + '' + + '814798' + + 'https://api.openstreetmap.org/api/0.6/notes/814798' + + 'https://api.openstreetmap.org/api/0.6/notes/814798/comment' + + 'https://api.openstreetmap.org/api/0.6/notes/814798/close' + + '2016-12-13 11:02:44 UTC' + + 'open' + + '' + + '' + + '2016-12-13 11:02:44 UTC' + + 'opened' + + 'Otford Scout Hut' + + '<p>Otford Scout Hut</p>' + + '' + + '' + + '' + + ''; + + beforeEach(function() { + connection.reset(); + server = sinon.fakeServer.create(); + spy = sinon.spy(); + }); + + afterEach(function() { + server.restore(); + }); + + it('returns an object', function (done) { + connection.loadFromAPI(path, function (err, xml) { + expect(err).to.not.be.ok; + expect(typeof xml).to.eql('object'); + done(); + }); + + server.respondWith('GET', 'http://www.openstreetmap.org' + path, + [200, { 'Content-Type': 'text/xml' }, response]); + server.respond(); + }); + }); + + describe('#loadEntity', function () { var nodeXML = '' + '' + - '', - wayXML = '' + + ''; + var wayXML = '' + '' + '' + ''; @@ -355,11 +402,12 @@ describe('iD.serviceOsm', function () { }); }); + describe('#loadEntityVersion', function () { var nodeXML = '' + '' + - '', - wayXML = '' + + ''; + var wayXML = '' + '' + ''; @@ -416,6 +464,7 @@ describe('iD.serviceOsm', function () { }); }); + describe('#loadMultiple', function () { beforeEach(function() { server = sinon.fakeServer.create(); @@ -428,7 +477,6 @@ describe('iD.serviceOsm', function () { it('loads nodes'); it('loads ways'); it('does not ignore repeat requests'); - }); From 65b2a4226170a43f30c2f5a7efc67c2d838236a3 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 1 Jul 2018 23:39:42 -0400 Subject: [PATCH 3/4] Adjust green --- css/65_data.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/css/65_data.css b/css/65_data.css index b66b48c11..49340654f 100644 --- a/css/65_data.css +++ b/css/65_data.css @@ -15,7 +15,7 @@ color: #ff3300; } .layer-notes .note.closed .note-fill { - color: #00bb33; + color: #55dd00; } .layer-notes .note.hovered .note-fill { color: #eebb00; From 13a30c050fc8abdfb2a1c8fd5d670725584194d3 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Mon, 2 Jul 2018 00:00:06 -0400 Subject: [PATCH 4/4] Add some dots if there is a comment thread --- modules/svg/notes.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/modules/svg/notes.js b/modules/svg/notes.js index f0f77dcee..c1c261dcb 100644 --- a/modules/svg/notes.js +++ b/modules/svg/notes.js @@ -129,6 +129,18 @@ export function svgNotes(projection, context, dispatch) { .attr('y', '-22px') .attr('xlink:href', '#fas-comment-alt'); + // add dots if there's a comment thread + notesEnter.selectAll('.thread') + .data(function(d) { return d.comments.length > 1 ? [0] : []; }) + .enter() + .append('use') + .attr('class', 'note-shadow thread') + .attr('width', '18px') + .attr('height', '18px') + .attr('x', '-9px') + .attr('y', '-22px') + .attr('xlink:href', '#iD-icon-more'); + // update notes .merge(notesEnter)