-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Disable merge operation when it would damage relations #8675
Conversation
bfe06f2
to
d3388bf
Compare
d3388bf
to
a14cf49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this will be quite useful, especially since there isn’t currently anything in the UI to inform the user upfront that the multiple selection has mixed relation membership.
modules/actions/join.js
Outdated
// this prohibits, and prohibits some cases this allows. | ||
var sortedParentRelations = function (id) { | ||
return graph.parentRelations(graph.entity(id)) | ||
.filter((rel) => !rel.isRestriction()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is this code that already handles restriction relations?
Line 162 in 1ee45ee
if (parent.isRestriction() && parent.members.some(function(m) { return nodeIds.indexOf(m.id) >= 0; })) { |
I’m not sure if it’s a problem, but connectivity relations are intended to be modeled the same way as restriction relations. (Connectivity relations have a preset but not an editor, so currently you have to create one manually or morph a restriction relation into one using the raw tag editor.) Maybe hasFromViaTo
would cover both relation types reasonably well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, but to do that right would require more changes than I want to get into in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate a bit: I want to avoid getting into modifying the code you linked to so that it prohibits joining connectivity relations in the same situations as restriction relations, due to i18n nuances. But it would be easy enough to just exempt them from the code I added. That would preserve the existing behavior for connectivity relations, while disabling problematic merges for the much more common relation types like route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next @1ec5 is going to ask for support for type=manoeuvre relations. 😆
@@ -316,6 +316,7 @@ en: | |||
relation: These features can't be merged because they have conflicting relation roles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string could be renamed to conflicting_relation_roles
since there’s now another one about overall relation membership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to touch connect.js or invalidate existing translations with this PR.
Thank you! |
The operation is disabled when attempting to join ways which don't belong to identical sets of relations. Restriction relations are excluded, because they are already handled with slightly different logic.
Fixes #8674
Fixes #8645
Fixes #3825
Fixes #1512