From 81127d71f3cdc431f292952006dc1751d34b8365 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 10 Apr 2019 10:19:23 -0400 Subject: [PATCH] Cache disabled() results in straighten action for consistent response What could happen was: - user could right click on a line - this would trigger `disabled()` checks for each operation buttons - the line was not fully downloaded, so would return `disabled()` 'not_downloaded' (and also start download of the missing tiles) - then the tooltip would pop into existence, calling `tooltip()` - which calls `disabled()` again - but this time it's fine and the `disabled()` is false - so you'd see a greyed out button but the tooltip said everyting is ok and you can click the button anyway I fixed this by just caching the disabled test. This is probably ok anyway because these tests can be expensive, and then the user will see a consistent message like "The line is not yet fully downloaded". If the user clicks off the line and back onto it, iD will reenter select mode, rebuild the menu, redo the disabled test, and they will see the button enabled. --- modules/core/context.js | 51 +++++++++++++++----------------- modules/modes/select.js | 18 +++-------- modules/operations/straighten.js | 18 +++++++---- modules/ui/edit_menu.js | 37 ++++++++++++----------- 4 files changed, 59 insertions(+), 65 deletions(-) diff --git a/modules/core/context.js b/modules/core/context.js index d4e7e62724..08e9b02dcf 100644 --- a/modules/core/context.js +++ b/modules/core/context.js @@ -108,62 +108,59 @@ export function coreContext() { }; - function wrapcb(callback, cid) { + function afterLoad(cid, callback) { return function(err, result) { if (err) { // 400 Bad Request, 401 Unauthorized, 403 Forbidden.. if (err.status === 400 || err.status === 401 || err.status === 403) { - connection.logout(); + if (connection) { + connection.logout(); + } } - return callback.call(context, err); + if (typeof callback === 'function') { + callback(err); + } + return; - } else if (connection.getConnectionId() !== cid) { - return callback.call(context, { message: 'Connection Switched', status: -1 }); + } else if (connection && connection.getConnectionId() !== cid) { + if (typeof callback === 'function') { + callback({ message: 'Connection Switched', status: -1 }); + } + return; } else { - return callback.call(context, err, result); + history.merge(result.data, result.extent); + if (typeof callback === 'function') { + callback(err, result); + } + return; } }; } context.loadTiles = function(projection, callback) { - var cid; - function done(err, result) { - if (!err) history.merge(result.data, result.extent); - if (callback) callback(err, result); - } if (connection && context.editable()) { - cid = connection.getConnectionId(); + var cid = connection.getConnectionId(); utilCallWhenIdle(function() { - connection.loadTiles(projection, wrapcb(done, cid)); + connection.loadTiles(projection, afterLoad(cid, callback)); })(); } }; context.loadTileAtLoc = function(loc, callback) { - var cid; - function done(err, result) { - if (!err) history.merge(result.data, result.extent); - if (callback) callback(err, result); - } if (connection && context.editable()) { - cid = connection.getConnectionId(); + var cid = connection.getConnectionId(); utilCallWhenIdle(function() { - connection.loadTileAtLoc(loc, wrapcb(done, cid)); + connection.loadTileAtLoc(loc, afterLoad(cid, callback)); })(); } }; context.loadEntity = function(entityID, callback) { - var cid; - function done(err, result) { - if (!err) history.merge(result.data, result.extent); - if (callback) callback(err, result); - } if (connection) { - cid = connection.getConnectionId(); - connection.loadEntity(entityID, wrapcb(done, cid)); + var cid = connection.getConnectionId(); + connection.loadEntity(entityID, afterLoad(cid, callback)); } }; diff --git a/modules/modes/select.js b/modules/modes/select.js index 117cb7210b..7db14f3019 100644 --- a/modules/modes/select.js +++ b/modules/modes/select.js @@ -1,21 +1,12 @@ -import { - event as d3_event, - select as d3_select -} from 'd3-selection'; +import { event as d3_event, select as d3_select } from 'd3-selection'; import { t } from '../util/locale'; import { actionAddMidpoint } from '../actions'; - import { - behaviorBreathe, - behaviorCopy, - behaviorHover, - behaviorLasso, - behaviorPaste, - behaviorSelect + behaviorBreathe, behaviorCopy, behaviorHover, + behaviorLasso, behaviorPaste, behaviorSelect } from '../behavior'; - import { geoExtent, geoChooseEdge, geoPointInPolygon } from '../geo'; import { modeBrowse } from './browse'; import { modeDragNode } from './drag_node'; @@ -24,7 +15,6 @@ import { osmNode, osmWay } from '../osm'; import * as Operations from '../operations/index'; import { uiEditMenu, uiSelectionList } from '../ui'; import { uiCmd } from '../ui/cmd'; - import { utilArrayIntersection, utilEntityOrMemberSelector, utilEntitySelector, utilKeybinding @@ -148,7 +138,7 @@ export function modeSelect(context, selectedIDs) { function positionMenu() { - if (!editMenu) { return; } + if (!editMenu) return; var entity = singular(); if (entity && context.geometry(entity.id) === 'relation') { diff --git a/modules/operations/straighten.js b/modules/operations/straighten.js index da7c2bd513..548701339a 100644 --- a/modules/operations/straighten.js +++ b/modules/operations/straighten.js @@ -5,6 +5,7 @@ import { utilArrayDifference, utilGetAllNodes } from '../util/index'; export function operationStraighten(selectedIDs, context) { + var _disabled; var action = actionStraighten(selectedIDs, context.projection); var wayIDs = selectedIDs.filter(function(id) { return id.charAt(0) === 'w'; }); var nodes = utilGetAllNodes(wayIDs, context.graph()); @@ -63,16 +64,21 @@ export function operationStraighten(selectedIDs, context) { operation.disabled = function() { - var reason = action.disabled(context.graph()); - if (reason) { - return reason; + if (_disabled !== undefined) return _disabled; + + _disabled = action.disabled(context.graph()); + if (_disabled) { + return _disabled; } else if (someMissing()) { - return 'not_downloaded'; + _disabled = 'not_downloaded'; + return _disabled; } else if (selectedIDs.some(context.hasHiddenConnections)) { - return 'connected_to_hidden'; + _disabled = 'connected_to_hidden'; + return _disabled; } - return false; + _disabled = false; + return _disabled; function someMissing() { diff --git a/modules/ui/edit_menu.js b/modules/ui/edit_menu.js index f742729649..b8acb3ce6d 100644 --- a/modules/ui/edit_menu.js +++ b/modules/ui/edit_menu.js @@ -1,9 +1,6 @@ -import { - event as d3_event, - select as d3_select -} from 'd3-selection'; +import { event as d3_event, select as d3_select } from 'd3-selection'; -import { geoVecFloor } from '../geo'; +import { geoVecAdd, geoVecFloor } from '../geo'; import { textDirection } from '../util/locale'; import { uiTooltipHtml } from './tooltipHtml'; @@ -51,7 +48,7 @@ export function uiEditMenu(context, operations) { offset[1] = -1 * (center[1] + menuHeight - viewport.height + vpBottomMargin); } - var origin = [ center[0] + offset[0], center[1] + offset[1] ]; + var origin = geoVecAdd(center, offset); menu = selection .append('g') @@ -75,19 +72,17 @@ export function uiEditMenu(context, operations) { var button = menu.selectAll('.edit-menu-item') - .data(operations) - .enter() + .data(operations); + + // enter + var buttonEnter = button.enter() .append('g') .attr('class', function (d) { return 'edit-menu-item edit-menu-item-' + d.id; }) - .classed('disabled', function (d) { return d.disabled(); }) - .attr('transform', function (d, i) { - return 'translate(' + geoVecFloor([ - 0, - m + i * buttonHeight - ]).join(',') + ')'; + .attr('transform', function(d, i) { + return 'translate(' + geoVecFloor([0, m + i * buttonHeight]).join(',') + ')'; }); - button + buttonEnter .append('rect') .attr('x', 4) .attr('width', buttonWidth) @@ -97,13 +92,19 @@ export function uiEditMenu(context, operations) { .on('mouseover', mouseover) .on('mouseout', mouseout); - button + buttonEnter .append('use') .attr('width', '20') .attr('height', '20') .attr('transform', function () { return 'translate(' + [2 * p, 5] + ')'; }) .attr('xlink:href', function (d) { return '#iD-operation-' + d.id; }); + // update + button = buttonEnter + .merge(button) + .classed('disabled', function(d) { return d.disabled(); }); + + tooltip = d3_select('#id-container') .append('div') .attr('class', 'tooltip-inner edit-menu-tooltip'); @@ -171,9 +172,9 @@ export function uiEditMenu(context, operations) { }; - editMenu.center = function (_) { + editMenu.center = function(val) { if (!arguments.length) return center; - center = _; + center = val; return editMenu; };