Prevent node drags that can damage relations

[abhisheksaikia]

When remodelling roads in iD, nodes are dragged and small segments may be deleted. This causes the relations in that segment to be modified/deleted and the mapper uploads it without knowledge. A warning box saying a relation has been affected will prevent relation breakage or unknowingly adding or modifying a relation. Particularly helpful in case of turn restrictions

An example where a node was dragged and the no_u_turn got affected:from role was deleted without any warning to the mapper 👇

[1ec5]

Part of the problem is that relation membership isn’t very visible – not on the map (#2946), and way below the fold in the feature editor. So even a mapper who’s familiar with turn restriction relations may be prone to accidentally deleting them.

[bhousel]

I hope that's just an example edit and not something that people are actually doing. That node drag probably moved the crosswalk tag onto the traffic signals node also.

We do have some situations where an action will be disabled if it will affect a relation, and we should add node drags to this check also.

[bhousel]

I hacked together something today that should prevent this and warn the user:

[bhousel]

The change I pushed does fix the issue but also prevent certain harmless drags, so I'm reopening this improve it.

[bhousel]

Ok this is really fixed this time.. It turned out to be a lot more complicated than I expected, but I added a lot of test coverage to connect.js to handle a lot of different ways that connecting nodes around a turn restriction can break things.

[slhh]

@bhousel

Ok this is really fixed this time..

At least not without generating new issues. I think it will turn out to be much more complicated than you have expected again.

I had a short look at the test cases, and realized that it does still prevent certain harmless drags, especially ones fixing a broken relation. I have been able to verify this using the master deployment. Overiding the prevention using the Alt key doesn't really work, because the Alt key prevents connecting by itself. This makes the situation even worse.

Connecting members of the same relation with different roles is not invalid in general, and even in case of turn restrictions this can be quite valid. In addition the checks need to be relation type specific.

I can offer to work on this issue, but it will take some time. When do you intend to release the next version of iD?
I would likely combine this with my work already done for #4769 and #4771.

[bhousel]

I had a short look at the test cases, and realized that it does still prevent certain harmless drags, especially ones fixing a broken relation. I have been able to verify this using the master deployment.

Can you share some more details - or a screen capture? I find it's much easier to view a gif created with LICEcap or ScreenToGif than to try to explain something in words.

I'm not immediately worried about the situation where a user is trying to repair an already broken relation.

Connecting members of the same relation with different roles is not invalid in general..

I will check but I think the code mostly allows this. The current code is supposed to prevent connecting node-node with conflicting roles (but node-way should be ok, except for the specific turn restriction checks), and I don't know any relation types which this would affect (site relations maybe?). It might prevent someone from dragging a PTv2 stop onto a PTv2 platform (which is good? should test).

When do you intend to release the next version of iD?

Tomorrow 🚀 So if you have a specific case that the current code gets wrong, let me know!

[slhh]

@bhousel

So if you have a specific case that the current code gets wrong, let me know!

1. nodes A,B,C,D,E
   way A-B: from member
   way B-C: via member
   way D-E: to member
   connecting C and D should not be prevented.
2. nodes A,B,C,D,E
   way A-B: from member
   way C-D: via member
   way D-E: to member
   connecting B and C should not be prevented.

The current code is supposed to prevent connecting node-node with conflicting roles

Preventing node-node conflicts seems to be fine. I assumed way-way role conflicts are also prevented.
The test description
it('returns falsy when connecting members of different relation and different roles',
might be read in this way, but I think I looked at a cached outdated version of the tests anyway. The current code looks quite different from what I remember to have seen.

[bhousel]

Interesting - in situation 1, the via and to are not connected, and in situation 2, the via and from are not connected.. and in both situations the user is trying to connect them to fix the issue?

The test description
it('returns falsy when connecting members of different relation and different roles',
might be read in this way, but I think I looked at a cached outdated version of the tests anyway. The current code looks quite different from what I remember to have seen.

Yes, I did change this since last Friday or so.. You might have seen the original version where the tests were much too strict..