Improvements to save flow

- Attempt fast save first, only perform conflict resolution if necessary (re: #3056)
- Block reentry of save, and dont keep focus on save button (closes #4641)
- Refactor modeSave() for code clarity (avoid shared state in closure variables)

modules/modes/save.js

@@ -1,7 +1,6 @@
 import _clone from 'lodash-es/clone';
 import _difference from 'lodash-es/difference';
 import _filter from 'lodash-es/filter';
-import _each from 'lodash-es/each';
 import _map from 'lodash-es/map';
 import _reduce from 'lodash-es/reduce';
 import _union from 'lodash-es/union';
@@ -44,18 +43,27 @@ import {
 } from '../util';
 
 
+var _isSaving = false;
+
 
 export function modeSave(context) {
-    var mode = {
-        id: 'save'
-    };
-
+    var mode = { id: 'save' };
     var keybinding = d3_keybinding('select');
 
+    var loading = uiLoading(context)
+        .message(t('save.uploading'))
+        .blocking(true);
+
     var commit = uiCommit(context)
         .on('cancel', cancel)
         .on('save', save);
 
+    var _toCheck = [];
+    var _toLoad = [];
+    var _conflicts = [];
+    var _errors = [];
+    var _origChanges;
+
 
     function cancel(selectedID) {
         if (selectedID) {
@@ -66,33 +74,50 @@ export function modeSave(context) {
     }
 
 
-    function save(changeset, tryAgain) {
-        var osm = context.connection();
-        var loading = uiLoading(context).message(t('save.uploading')).blocking(true);
-        var history = context.history();
-        var origChanges = history.changes(actionDiscardTags(history.difference()));
-        var localGraph = context.graph();
-        var remoteGraph = coreGraph(history.base(), true);
-        var modified = _filter(history.difference().summary(), {changeType: 'modified'});
-        var toCheck = _map(_map(modified, 'entity'), 'id');
-        var toLoad = withChildNodes(toCheck, localGraph);
-        var conflicts = [];
-        var errors = [];
+    function save(changeset, tryAgain, checkConflicts) {
+        // Guard against accidentally entering save code twice - #4641
+        if (_isSaving && !tryAgain) return;
 
+        var osm = context.connection();
         if (!osm) return;
 
+        _isSaving = true;
+        context.container().call(loading);  // block input
+
+        var history = context.history();
+        var localGraph = context.graph();
+        var remoteGraph = coreGraph(history.base(), true);
+
+        _conflicts = [];
+        _errors = [];
+
+        // Store original changes, in case user wants to download them as an .osc file
+        _origChanges = history.changes(actionDiscardTags(history.difference()));
+
+        // First time, `history.perform` a no-op action.
+        // Any conflict resolutions will be done as `history.replace`
         if (!tryAgain) {
-            history.perform(actionNoop());  // checkpoint
+            history.perform(actionNoop());
         }
 
-        context.container().call(loading);
+        // Attempt a fast upload.. If there are conflicts, re-enter with `checkConflicts = true`
+        if (!checkConflicts) {
+            upload(changeset);
 
-        if (toCheck.length) {
-            osm.loadMultiple(toLoad, loaded);
+        // Do the full (slow) conflict check..
         } else {
-            upload();
+            var modified = _filter(history.difference().summary(), { changeType: 'modified' });
+            _toCheck = _map(_map(modified, 'entity'), 'id');
+            _toLoad = withChildNodes(_toCheck, localGraph);
+            if (_toCheck.length) {
+                osm.loadMultiple(_toLoad, loaded);
+            } else {
+                upload(changeset);
+            }
         }
 
+        return;
+
 
         function withChildNodes(ids, graph) {
             return _uniq(_reduce(ids, function(result, id) {
@@ -111,13 +136,12 @@ export function modeSave(context) {
             }, _clone(ids)));
         }
 
-
         // Reload modified entities into an alternate graph and check for conflicts..
         function loaded(err, result) {
-            if (errors.length) return;
+            if (_errors.length) return;
 
             if (err) {
-                errors.push({
+                _errors.push({
                     msg: err.responseText,
                     details: [ t('save.status_code', { code: err.status }) ]
                 });
@@ -125,35 +149,35 @@ export function modeSave(context) {
 
             } else {
                 var loadMore = [];
-                _each(result.data, function(entity) {
+                result.data.forEach(function(entity) {
                     remoteGraph.replace(entity);
-                    toLoad = _without(toLoad, entity.id);
+                    _toLoad = _without(_toLoad, entity.id);
 
                     // Because loadMultiple doesn't download /full like loadEntity,
                     // need to also load children that aren't already being checked..
                     if (!entity.visible) return;
                     if (entity.type === 'way') {
                         loadMore.push.apply(loadMore,
-                            _difference(entity.nodes, toCheck, toLoad, loadMore));
+                            _difference(entity.nodes, _toCheck, _toLoad, loadMore));
                     } else if (entity.type === 'relation' && entity.isMultipolygon()) {
                         loadMore.push.apply(loadMore,
-                            _difference(_map(entity.members, 'id'), toCheck, toLoad, loadMore));
+                            _difference(_map(entity.members, 'id'), _toCheck, _toLoad, loadMore));
                     }
                 });
 
                 if (loadMore.length) {
-                    toLoad.push.apply(toLoad, loadMore);
+                    _toLoad.push.apply(_toLoad, loadMore);
                     osm.loadMultiple(loadMore, loaded);
                 }
 
-                if (!toLoad.length) {
-                    checkConflicts();
+                if (!_toLoad.length) {
+                    detectConflicts();
                 }
             }
         }
 
 
-        function checkConflicts() {
+        function detectConflicts() {
             function choice(id, text, action) {
                 return { id: id, text: text, action: function() { history.replace(action); } };
             }
@@ -164,16 +188,14 @@ export function modeSave(context) {
                 return utilDisplayName(entity) || (utilDisplayType(entity.id) + ' ' + entity.id);
             }
 
-            function compareVersions(local, remote) {
+            function sameVersions(local, remote) {
                 if (local.version !== remote.version) return false;
 
                 if (local.type === 'way') {
                     var children = _union(local.nodes, remote.nodes);
-
                     for (var i = 0; i < children.length; i++) {
                         var a = localGraph.hasEntity(children[i]);
                         var b = remoteGraph.hasEntity(children[i]);
-
                         if (a && b && a.version !== b.version) return false;
                     }
                 }
@@ -181,11 +203,11 @@ export function modeSave(context) {
                 return true;
             }
 
-            _each(toCheck, function(id) {
+            _toCheck.forEach(function(id) {
                 var local = localGraph.entity(id);
                 var remote = remoteGraph.entity(id);
 
-                if (compareVersions(local, remote)) return;
+                if (sameVersions(local, remote)) return;
 
                 var action = actionMergeRemoteChanges;
                 var merge = action(id, localGraph, remoteGraph, formatUser);
@@ -200,7 +222,7 @@ export function modeSave(context) {
                 var keepMine = t('save.conflict.' + (remote.visible ? 'keep_local' : 'restore'));
                 var keepTheirs = t('save.conflict.' + (remote.visible ? 'keep_remote' : 'delete'));
 
-                conflicts.push({
+                _conflicts.push({
                     id: id,
                     name: entityName(local),
                     details: mergeConflicts,
@@ -212,163 +234,179 @@ export function modeSave(context) {
                 });
             });
 
-            upload();
+            upload(changeset);
+        }
+    }
+
+
+    function upload(changeset) {
+        var osm = context.connection();
+        if (!osm) {
+            _errors.push({ msg: 'No OSM Service' });
         }
 
+        if (_conflicts.length) {
+            _conflicts.sort(function(a, b) { return b.id.localeCompare(a.id); });
+            showConflicts(changeset);
 
-        function upload() {
-            if (conflicts.length) {
-                conflicts.sort(function(a,b) { return b.id.localeCompare(a.id); });
-                showConflicts();
-            } else if (errors.length) {
-                showErrors();
-            } else {
-                var changes = history.changes(actionDiscardTags(history.difference()));
-                if (changes.modified.length || changes.created.length || changes.deleted.length) {
-                    osm.putChangeset(changeset, changes, uploadCallback);
-                } else {        // changes were insignificant or reverted by user
-                    d3_select('.inspector-wrap *').remove();
-                    loading.close();
-                    context.flush();
-                    cancel();
-                }
+        } else if (_errors.length) {
+            showErrors();
+
+        } else {
+            var history = context.history();
+            var changes = history.changes(actionDiscardTags(history.difference()));
+            if (changes.modified.length || changes.created.length || changes.deleted.length) {
+                osm.putChangeset(changeset, changes, uploadCallback);
+            } else {        // changes were insignificant or reverted by user
+                d3_select('.inspector-wrap *').remove();
+                loading.close();
+                _isSaving = false;
+                context.flush();
+                cancel();
             }
         }
+    }
 
 
-        function uploadCallback(err, changeset) {
-            if (err) {
-                errors.push({
+    function uploadCallback(err, changeset) {
+        if (err) {
+            if (err.status === 409) {          // 409 Conflict
+                save(changeset, true, true);   // tryAgain = true, checkConflicts = true
+            } else {
+                _errors.push({
                     msg: err.responseText,
                     details: [ t('save.status_code', { code: err.status }) ]
                 });
                 showErrors();
-            } else {
-                history.clearSaved();
-                success(changeset);
-                // Add delay to allow for postgres replication #1646 #2678
-                window.setTimeout(function() {
-                    d3_select('.inspector-wrap *').remove();
-                    loading.close();
-                    context.flush();
-                }, 2500);
             }
+
+        } else {
+            context.history().clearSaved();
+            success(changeset);
+            // Add delay to allow for postgres replication #1646 #2678
+            window.setTimeout(function() {
+                d3_select('.inspector-wrap *').remove();
+                loading.close();
+                _isSaving = false;
+                context.flush();
+            }, 2500);
         }
+    }
 
 
-        function showConflicts() {
-            var selection = context.container()
-                .select('#sidebar')
-                .append('div')
-                .attr('class','sidebar-component');
+    function showConflicts(changeset) {
+        var history = context.history();
+        var selection = context.container()
+            .select('#sidebar')
+            .append('div')
+            .attr('class','sidebar-component');
 
-            loading.close();
+        loading.close();
+        _isSaving = false;
 
-            selection.call(uiConflicts(context)
-                .list(conflicts)
-                .origChanges(origChanges)
-                .on('cancel', function() {
-                    history.pop();
-                    selection.remove();
-                })
-                .on('save', function() {
-                    for (var i = 0; i < conflicts.length; i++) {
-                        if (conflicts[i].chosen === 1) {  // user chose "keep theirs"
-                            var entity = context.hasEntity(conflicts[i].id);
-                            if (entity && entity.type === 'way') {
-                                var children = _uniq(entity.nodes);
-                                for (var j = 0; j < children.length; j++) {
-                                    history.replace(actionRevert(children[j]));
-                                }
+        var ui = uiConflicts(context)
+            .conflictList(_conflicts)
+            .origChanges(_origChanges)
+            .on('cancel', function() {
+                history.pop();
+                selection.remove();
+            })
+            .on('save', function() {
+                for (var i = 0; i < _conflicts.length; i++) {
+                    if (_conflicts[i].chosen === 1) {  // user chose "keep theirs"
+                        var entity = context.hasEntity(_conflicts[i].id);
+                        if (entity && entity.type === 'way') {
+                            var children = _uniq(entity.nodes);
+                            for (var j = 0; j < children.length; j++) {
+                                history.replace(actionRevert(children[j]));
                             }
-                            history.replace(actionRevert(conflicts[i].id));
                         }
+                        history.replace(actionRevert(_conflicts[i].id));
                     }
+                }
 
-                    selection.remove();
-                    save(changeset, true);
-                })
-            );
-        }
+                selection.remove();
+                save(changeset, true, false);  // tryAgain = true, checkConflicts = false
+            });
+
+        selection.call(ui);
+    }
 
 
-        function showErrors() {
-            var selection = uiConfirm(context.container());
+    function showErrors() {
+        var selection = uiConfirm(context.container());
 
-            history.pop();
-            loading.close();
+        context.history().pop();
+        loading.close();
+        _isSaving = false;
 
-            selection
-                .select('.modal-section.header')
-                .append('h3')
-                .text(t('save.error'));
+        selection
+            .select('.modal-section.header')
+            .append('h3')
+            .text(t('save.error'));
 
-            addErrors(selection, errors);
-            selection.okButton();
-        }
+        addErrors(selection, _errors);
+        selection.okButton();
+    }
 
 
-        function addErrors(selection, data) {
-            var message = selection
-                .select('.modal-section.message-text');
+    function addErrors(selection, data) {
+        var message = selection
+            .select('.modal-section.message-text');
 
-            var items = message
-                .selectAll('.error-container')
-                .data(data);
+        var items = message
+            .selectAll('.error-container')
+            .data(data);
 
-            var enter = items.enter()
-                .append('div')
-                .attr('class', 'error-container');
+        var enter = items.enter()
+            .append('div')
+            .attr('class', 'error-container');
 
-            enter
-                .append('a')
-                .attr('class', 'error-description')
-                .attr('href', '#')
-                .classed('hide-toggle', true)
-                .text(function(d) { return d.msg || t('save.unknown_error_details'); })
-                .on('click', function() {
-                    var error = d3_select(this);
-                    var detail = d3_select(this.nextElementSibling);
-                    var exp = error.classed('expanded');
+        enter
+            .append('a')
+            .attr('class', 'error-description')
+            .attr('href', '#')
+            .classed('hide-toggle', true)
+            .text(function(d) { return d.msg || t('save.unknown_error_details'); })
+            .on('click', function() {
+                d3_event.preventDefault();
 
-                    detail.style('display', exp ? 'none' : 'block');
-                    error.classed('expanded', !exp);
+                var error = d3_select(this);
+                var detail = d3_select(this.nextElementSibling);
+                var exp = error.classed('expanded');
 
-                    d3_event.preventDefault();
-                });
+                detail.style('display', exp ? 'none' : 'block');
+                error.classed('expanded', !exp);
+            });
 
-            var details = enter
-                .append('div')
-                .attr('class', 'error-detail-container')
-                .style('display', 'none');
+        var details = enter
+            .append('div')
+            .attr('class', 'error-detail-container')
+            .style('display', 'none');
 
-            details
-                .append('ul')
-                .attr('class', 'error-detail-list')
-                .selectAll('li')
-                .data(function(d) { return d.details || []; })
-                .enter()
-                .append('li')
-                .attr('class', 'error-detail-item')
-                .text(function(d) { return d; });
-
-            items.exit()
-                .remove();
-        }
+        details
+            .append('ul')
+            .attr('class', 'error-detail-list')
+            .selectAll('li')
+            .data(function(d) { return d.details || []; })
+            .enter()
+            .append('li')
+            .attr('class', 'error-detail-item')
+            .text(function(d) { return d; });
 
+        items.exit()
+            .remove();
     }
 
 
     function success(changeset) {
         commit.reset();
-        context.enter(modeBrowse(context)
-            .sidebar(uiSuccess(context)
-                .changeset(changeset)
-                .on('cancel', function() {
-                    context.ui().sidebar.hide();
-                })
-            )
-        );
+
+        var ui = uiSuccess(context)
+            .changeset(changeset)
+            .on('cancel', function() { context.ui().sidebar.hide(); });
+
+        context.enter(modeBrowse(context).sidebar(ui));
     }
 
 
@@ -404,6 +442,9 @@ export function modeSave(context) {
 
 
     mode.exit = function() {
+        loading.close();
+        _isSaving = false;
+
         keybinding.off();
 
         context.container().selectAll('#content')

modules/services/osm.js

@@ -349,7 +349,7 @@ export default {
 
     putChangeset: function(changeset, changes, callback) {
         if (_changeset.inflight) {
-            return callback({ message: 'Changeset already inflight', status: -2 });
+            return callback({ message: 'Changeset already inflight', status: -2 }, changeset);
         }
 
         var that = this;
@@ -371,10 +371,10 @@ export default {
             _changeset.inflight = null;
 
             if (err) {
-                return callback(err);
+                return callback(err, changeset);
             }
             if (that.getConnectionId() !== cid) {
-                return callback({ message: 'Connection Switched', status: -1 });
+                return callback({ message: 'Connection Switched', status: -1 }, changeset);
             }
 
             _changeset.open = changesetID;
@@ -393,7 +393,7 @@ export default {
         function uploadedChangeset(err) {
             _changeset.inflight = null;
 
-            if (err) return callback(err);
+            if (err) return callback(err, changeset);
 
             // Upload was successful, safe to call the callback.
             // Add delay to allow for postgres replication #1646 #2678
@@ -680,7 +680,7 @@ export default {
             _rateLimitError = undefined;
             dispatch.call('change');
             if (callback) callback(err, res);
-            that._userChangesets(function() {});  // eagerly load user details/changesets
+            that.userChangesets(function() {});  // eagerly load user details/changesets
         }
 
         return oauth.authenticate(done);

modules/ui/commit.js

@@ -238,6 +238,7 @@ export function uiCommit(context) {
                 return (n && n.value.length) ? null : true;
             })
             .on('click.save', function() {
+                this.blur();    // avoid keeping focus on the button - #4641
                 dispatch.call('save', this, _changeset);
             });
 

modules/ui/confirm.js

@@ -27,7 +27,9 @@ export function uiConfirm(selection) {
             .on('click.confirm', function() {
                 modalSelection.remove();
             })
-            .text(t('confirm.okay'));
+            .text(t('confirm.okay'))
+            .node()
+            .focus();
 
         return modalSelection;
     };

modules/ui/conflicts.js

@@ -16,9 +16,9 @@ import { utilRebind } from '../util/rebind';
 
 
 export function uiConflicts(context) {
-    var dispatch = d3_dispatch('cancel', 'save'),
-        origChanges,
-        conflictList;
+    var dispatch = d3_dispatch('cancel', 'save');
+    var _origChanges;
+    var _conflictList;
 
 
     function conflicts(selection) {
@@ -47,14 +47,14 @@ export function uiConflicts(context) {
 
 
         // Download changes link
-        var detected = utilDetect(),
-            changeset = new osmChangeset();
+        var detected = utilDetect();
+        var changeset = new osmChangeset();
 
-        delete changeset.id;  // Export without chnageset_id
+        delete changeset.id;  // Export without changeset_id
 
-        var data = JXON.stringify(changeset.osmChangeJXON(origChanges)),
-            blob = new Blob([data], {type: 'text/xml;charset=utf-8;'}),
-            fileName = 'changes.osc';
+        var data = JXON.stringify(changeset.osmChangeJXON(_origChanges));
+        var blob = new Blob([data], { type: 'text/xml;charset=utf-8;' });
+        var fileName = 'changes.osc';
 
         var linkEnter = conflictsHelp.selectAll('.download-changes')
             .data([0])
@@ -99,7 +99,7 @@ export function uiConflicts(context) {
 
         buttons
             .append('button')
-            .attr('disabled', conflictList.length > 1)
+            .attr('disabled', _conflictList.length > 1)
             .attr('class', 'action conflicts-button col6')
             .text(t('save.title'))
             .on('click.try_again', function() { dispatch.call('save'); });
@@ -113,12 +113,12 @@ export function uiConflicts(context) {
 
 
     function showConflict(selection, index) {
-        if (index < 0 || index >= conflictList.length) return;
+        if (index < 0 || index >= _conflictList.length) return;
 
         var parent = d3_select(selection.node().parentNode);
 
         // enable save button if this is the last conflict being reviewed..
-        if (index === conflictList.length - 1) {
+        if (index === _conflictList.length - 1) {
             window.setTimeout(function() {
                 parent.select('.conflicts-button')
                     .attr('disabled', null);
@@ -132,7 +132,7 @@ export function uiConflicts(context) {
 
         var item = selection
             .selectAll('.conflict')
-            .data([conflictList[index]]);
+            .data([_conflictList[index]]);
 
         var enter = item.enter()
             .append('div')
@@ -141,7 +141,7 @@ export function uiConflicts(context) {
         enter
             .append('h4')
             .attr('class', 'conflict-count')
-            .text(t('save.conflict.count', { num: index + 1, total: conflictList.length }));
+            .text(t('save.conflict.count', { num: index + 1, total: _conflictList.length }));
 
         enter
             .append('a')
@@ -183,11 +183,11 @@ export function uiConflicts(context) {
             .attr('class', 'conflict-nav-button action col6')
             .attr('disabled', function(d, i) {
                 return (i === 0 && index === 0) ||
-                    (i === 1 && index === conflictList.length - 1) || null;
+                    (i === 1 && index === _conflictList.length - 1) || null;
             })
             .on('click', function(d, i) {
-                var container = parent.select('.conflict-container'),
-                sign = (i === 0 ? -1 : 1);
+                var container = parent.select('.conflict-container');
+                var sign = (i === 0 ? -1 : 1);
 
                 container
                     .selectAll('.conflict')
@@ -249,8 +249,8 @@ export function uiConflicts(context) {
             .selectAll('input')
             .property('checked', function(d) { return d === datum; });
 
-        var extent = geoExtent(),
-            entity;
+        var extent = geoExtent();
+        var entity;
 
         entity = context.graph().hasEntity(datum.id);
         if (entity) extent._extend(entity.extent(context.graph()));
@@ -275,8 +275,7 @@ export function uiConflicts(context) {
             } else {
                 context.map().zoomTo(entity);
             }
-            context.surface().selectAll(
-                utilEntityOrMemberSelector([entity.id], context.graph()))
+            context.surface().selectAll(utilEntityOrMemberSelector([entity.id], context.graph()))
                 .classed('hover', true);
         }
     }
@@ -293,16 +292,16 @@ export function uiConflicts(context) {
     //         choice(id, keepTheirs, forceRemote)
     //     ]
     // }
-    conflicts.list = function(_) {
-        if (!arguments.length) return conflictList;
-        conflictList = _;
+    conflicts.conflictList = function(_) {
+        if (!arguments.length) return _conflictList;
+        _conflictList = _;
         return conflicts;
     };
 
 
     conflicts.origChanges = function(_) {
-        if (!arguments.length) return origChanges;
-        origChanges = _;
+        if (!arguments.length) return _origChanges;
+        _origChanges = _;
         return conflicts;
     };