-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improved rendering/handling of relation members #11701
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
base: develop
Are you sure you want to change the base?
Changes from all commits
e25e6f0
e69bf43
0468b52
8539405
c910931
003143d
aca85ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,8 @@ export function actionJoin(ids) { | |
| isFinite(tagsB[key])); | ||
| } | ||
|
|
||
| action.id = 'join'; | ||
|
|
||
|
|
||
| return action; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { actionDeleteMembers } from './delete_members'; | ||
|
|
||
|
|
||
| // `actionMembersRemove` removes selected members from a single relation | ||
| // | ||
| // * there must be only one relation in the selection | ||
| // * all other selected entities are members of the relation | ||
| // * the operation removes all occurrences of the features from | ||
| // being a member of the relation | ||
|
|
||
| export function actionMembersRemove(entityIDs) { | ||
|
|
||
| // export function actionDeleteMembers(relationId, memberIndexes) { | ||
|
|
||
| var action = function(graph) { | ||
| let relation; | ||
| for (const entityID of entityIDs) { | ||
| var entity = graph.entity(entityID); | ||
| if (entity.type === 'relation') { | ||
| relation = entity; | ||
| } | ||
| } | ||
| let memberIndices = []; | ||
| for (let i = 0; i < relation.members.length; i++) { | ||
| if (entityIDs.indexOf(relation.members[i].id) > 0) { | ||
| memberIndices.push(i); | ||
| } | ||
| } | ||
|
|
||
| graph = actionDeleteMembers(relation.id, memberIndices)(graph); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a feature is a member of the relation multiple times, this operation completely removes all its memberships from the relation. This makes it less useful for dealing with incorrect redundant memberships, though it would make more sense if the operation were called something like Detach. |
||
|
|
||
| // only keep relation in new selection (see operation/merge.js) | ||
| entityIDs.splice(0, entityIDs.indexOf(relation.id)); | ||
| entityIDs.splice(1, entityIDs.length - 1); | ||
|
|
||
| return graph; | ||
| }; | ||
|
|
||
|
|
||
| action.disabled = function(graph) { | ||
| let relation; | ||
| for (const entityID of entityIDs) { | ||
| var entity = graph.entity(entityID); | ||
| if (entity.type === 'relation') { | ||
| if (relation !== undefined) return 'not_eligible'; | ||
| relation = entity; | ||
| } | ||
| } | ||
| if (relation === undefined) { | ||
| return 'not_eligible'; | ||
| } | ||
| if (entityIDs.some(entityID => entityID !== relation.id && | ||
| !relation.members.find(member => member.id === entityID))) { | ||
| return 'not_eligible'; | ||
| } | ||
| }; | ||
|
|
||
| return action; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,8 @@ export function actionMerge(ids) { | |
| } | ||
| }; | ||
|
|
||
| action.id = 'merge'; | ||
|
|
||
|
|
||
| return action; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { actionAddMember } from './add_member'; | ||
|
|
||
|
|
||
| // `actionMergeMembers` adds new members to a single relation | ||
| // | ||
| // * there must be only one relation in the selection | ||
| // * all other selected entities are added as new members to the relation | ||
| // * sorting is done "automagically" when applicable (e.g. connecting to | ||
| // existing members of a route), otherwise they will be appended at the | ||
| // end of the members list | ||
| // * members are added using an empty role | ||
|
|
||
| export function actionMergeMembers(entityIDs) { | ||
|
|
||
| var action = function(graph) { | ||
| let relationID; | ||
| let newMembers = []; | ||
| for (const entityID of entityIDs) { | ||
| var entity = graph.entity(entityID); | ||
| if (entity.type === 'relation') { | ||
| relationID = entityID; | ||
| } else { | ||
| newMembers.push({ | ||
| id: entity.id, | ||
| type: entity.type, | ||
| role: '' | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| for (const member of newMembers) { | ||
| graph = actionAddMember(relationID, member)(graph); | ||
| } | ||
|
|
||
| // only keep relation in new selection (see operation/merge.js) | ||
| entityIDs.splice(0, entityIDs.indexOf(relationID)); | ||
| entityIDs.splice(1, entityIDs.length - 1); | ||
|
|
||
| return graph; | ||
| }; | ||
|
|
||
|
|
||
| action.disabled = function(graph) { | ||
| const relationCount = entityIDs.filter(entityID => | ||
| graph.entity(entityID).type === 'relation') | ||
| .length; | ||
| if (relationCount !== 1) return 'not_eligible'; | ||
|
Comment on lines
+44
to
+47
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Merge action remains enabled even when the Remove Members action is enabled, so the context menu starts out with ➖ followed by ➕. Do we need to have both actions enabled at the same time? Users shouldn’t need to add redundant memberships very often, but when they do have to, they can still add it the old-fashioned way with the way selected. |
||
| }; | ||
|
|
||
| action.id = 'merge_members'; | ||
|
|
||
| return action; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,6 +153,8 @@ export function actionMergePolygon(ids, newRelationId) { | |
| } | ||
| }; | ||
|
|
||
| action.id = 'merge_polygon'; | ||
|
|
||
|
|
||
| return action; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { t } from '../core/localizer'; | ||
|
|
||
| import { actionMembersRemove } from '../actions/members_remove'; | ||
|
|
||
| import { behaviorOperation } from '../behavior/operation'; | ||
| import { modeSelect } from '../modes/select'; | ||
|
|
||
| export function operationMembersRemove(context, selectedIDs) { | ||
|
|
||
| var _action = actionMembersRemove(selectedIDs); | ||
|
|
||
| var operation = function() { | ||
|
|
||
| if (operation.disabled()) return; | ||
|
|
||
| context.perform(_action, operation.annotation()); | ||
|
|
||
| context.validator().validate(); | ||
|
|
||
| var resultIDs = selectedIDs.filter(context.hasEntity); | ||
| if (resultIDs.length > 1) { | ||
| var interestingIDs = resultIDs.filter(function(id) { | ||
| return context.entity(id).hasInterestingTags(); | ||
| }); | ||
| if (interestingIDs.length) resultIDs = interestingIDs; | ||
| } | ||
| context.enter(modeSelect(context, resultIDs)); | ||
| }; | ||
|
|
||
| operation.available = function() { | ||
| return !_action.disabled(context.graph()); | ||
| }; | ||
|
|
||
| operation.disabled = function() { | ||
| return _action.disabled(context.graph()); | ||
| }; | ||
|
|
||
| operation.tooltip = function() { | ||
| return t.append('operations.members_remove.description'); | ||
| }; | ||
|
|
||
| operation.annotation = function() { | ||
| return t('operations.members_remove.annotation', { n: selectedIDs.length }); | ||
| }; | ||
|
|
||
| operation.id = 'members_remove'; | ||
| operation.keys = []; | ||
| operation.title = t.append('operations.members_remove.title'); | ||
| operation.behavior = behaviorOperation(context).which(operation); | ||
|
|
||
| return operation; | ||
| } |
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.
Remove Members doesn’t look like an operation that undoes what Merge does. How about Unmerge? Or maybe make it look like a version of the Detach operation.