Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolving Changeset already inflight error when editing with 2 layers w/in map #1427

Open
wants to merge 36 commits into
base: hoot2x
Choose a base branch
from

Conversation

jackgrossman18
Copy link
Contributor

ref #1425

@jackgrossman18 jackgrossman18 requested a review from maxgrossman May 8, 2019 17:41
/**
* Get all id's necessary to create new map features
*
* @param {Promise|array} mapId + changesetId, nodeId, wayId, relationId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder, let's update this comment!

return this.request( params )
.then( resp => {

let activeIds = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just return resp.data!

@@ -15,6 +15,7 @@ import {
utilQsString,
utilStringQs
} from '../../../util';
import { diff3MergeIndices } from 'node-diff3';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think we can remove this import!

@@ -73,6 +74,8 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) {
function move(datum) {
context.surface().classed('nope-disabled', d3_event.altKey);

// add one more boolean at end, is activeLayer===mapId
var _activeLayer = _find( Hoot.layers.loadedLayers, function(a, b) { return a.activeLayer; });
Copy link
Contributor

@maxgrossman maxgrossman May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make use of this here, making sure to only apply a target loc if active ids match.

here in iD a question is asked (is this target loc one that allows vertex).

Our question needs to be does the datum's mapId equal the _activeLayer mapId


var _activeLayer = _find(Hoot.layers.loadedLayers, function(a, b) { return a.activeLayer; });
if (_activeLayer && _activeLayer.id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally just a stylistic suggestion, so no need to change, but I almost wonder if we should make a new func for grabbing the ids from the active layer. something like osmEntity.id.hoot that looks a little like this...

function(type, activeLayer) {
   return osmEntity.id.fromOsm(type, activeLayer.activeIds[type]--) + '_' + activeLayer.id;
}

Then here in this if block you just say "do I have an activeLayer'?
Ok I do, use id.hoot function
If I don't use the out of the box id way of doing things.

just a thought!

@jackgrossman18
Copy link
Contributor Author

@maxgrossman please take a look at the most recent changes

let changeActive = new LayerAdd();


if (d3.select('#reference button.select-active-layer').empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if the on click callback can be generalized in a single function? seems like when we click one fo the active layer buttons, we make the one we click's classes all set to active, and the other one's set to false. so a function could have 2 params, one we want to set to active, one we want to disable and just use those accordingly.

so naively, something like

function update(active, inactive) {
   //...update active button class
   //...update inactive button
}

@@ -279,6 +287,8 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) {

// Connect the way to an existing node and continue drawing.
drawWay.addNode = function(node, d) {
console.log(node);
Copy link
Contributor

@maxgrossman maxgrossman May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the stray console logs

@@ -73,7 +75,9 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) {
function move(datum) {
context.surface().classed('nope-disabled', d3_event.altKey);

var _activeLayer = _find( Hoot.layers.loadedLayers, function(a,b) { return a.activeLayer; });
var targetLoc = datum && datum.properties && datum.properties.entity && datum.properties.entity.loc;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to implement the commented out piece here, using the same split stuff we do in the draw.js file.

@@ -140,6 +140,7 @@ export function behaviorHover(context) {
return;
}

// again, boolean, is entity.mapId === activeLayerId...
var suppressed = _altDisables && d3_event && d3_event.altKey;
Copy link
Contributor

@maxgrossman maxgrossman May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to do same is this the active layer check here too

var _activeLayer = _find(Hoot.layers.loadedLayers, function(a, b) { return a.activeLayer; });
let allIds = {};
allIds = _activeLayer.activeIds;
var id = osmEntity.id.fromOSM(type, allIds[type]--);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can call the _activeLayer.activeIds directly inside the fromOsm function _activeLayer.activeIds[type]--

@@ -41,6 +42,7 @@ import {
} from '../util';

import { baseUrl as hootBaseUrl } from '../Hoot/config/apiConfig';
import { all } from 'q';
Copy link
Contributor

@maxgrossman maxgrossman May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import to remove I do believe

@@ -8,6 +8,7 @@ import _flatten from 'lodash-es/flatten';
import _groupBy from 'lodash-es/groupBy';
import _isEmpty from 'lodash-es/isEmpty';
import _map from 'lodash-es/map';
import _matches from 'lodash-es/matches';
Copy link
Contributor

@maxgrossman maxgrossman May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import to remove

@jackgrossman18
Copy link
Contributor Author

@maxgrossman please take a look at the new changes

@maxgrossman
Copy link
Contributor

The changeset upload logic is 👨‍🍳 💋 - made changes to both the secondary and reference datasets, then checked their respective changesets/nodes tables. they had the right changes. Awesome job.

In addition to those comments, I found these bugs.

sometimes I can't double click off when finishing drawing a way.

cannot-double-click-off

I cannot drag nodes that belong to features within the currently active layer

cannot-drag-node

If you notice in the first gif how the icon changes. I think that indicates the mode is changing. For debugging why this is happening, it might be helpful to look at the context's exit and enter methods since they tell you when the modes are being switched.

@jackgrossman18
Copy link
Contributor Author

@maxgrossman @brianhatchl this is ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants