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

prevent survey markers from being moved #10122

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

k-yle
Copy link
Contributor

@k-yle k-yle commented Feb 22, 2024

Closes #9255, Closes #9867

survey_point: true
},
latitude: true,
longitude: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if encouraging these two keys is a good idea, since most appear to be introduced by bodged imports

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I could see this legitimizing the two keys as a “one weird trick” for locking a feature. Imagine someone pranking other mappers by adding a longitude to all the POIs they map. It would never show up in the fields, only the raw tags, hidden by default.

Copy link
Collaborator

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Should we apply any styling to the point on the map, so that it looks locked before the user tries to futz with it?

iD/css/20_map.css

Lines 296 to 304 in b5f1f78

/* Wikidata-tagged */
g.point.tag-wikidata path.stroke {
stroke-width: 2px;
stroke: #666;
fill: #eee;
}
g.point.tag-wikidata .icon {
color: #666;
}

Will mappers get annoyed that they can’t move the point and delete it instead? A wikidata=* tag locks a feature from getting deleted too.

@@ -232,6 +232,9 @@ export function modeDragNode(context) {
isInvalid = hasInvalidGeometry(entity, context.graph());
}

if (entity.type === 'node' && entity.isLocked()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be feasible to allow the drag when some modifier key is held down, like Alt/Option, when the mapper is sure they know what they’re doing? If not, another approach would be to create a filter for things like survey points and hide it by default, as we do with boundaries.

/ref #9255 (comment)

survey_point: true
},
latitude: true,
longitude: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I could see this legitimizing the two keys as a “one weird trick” for locking a feature. Imagine someone pranking other mappers by adding a longitude to all the POIs they map. It would never show up in the fields, only the raw tags, hidden by default.

@k-yle
Copy link
Contributor Author

k-yle commented Feb 23, 2024

All your suggestions seem like good ideas, I'll wait and see if there are any other comments before making changes.

Would it be feasible to allow the drag when some modifier key is held down, like Alt/Option, when the mapper is sure they know what they’re doing?

I'm pretty sure it's possible since we have access to MouseEvent#altKey in both contexts

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable behavior for iD. Yes, not every survey point mapped in OSM was imported from a source with authoritative precision, but I would say that accidentally moved survey points are a bigger issue than misplaced survey points which are not correctable easily in iD. An override for advanced users to still edit the location of such points would be nice to have, but a relatively easy workaround is to just change the feature temporarily to a different preset while moving.

I'd also exclude latitude/longitude from the tags for this feature. 👍

As far as I see, it does not fully fix the issue reported in #9255, as the validation warning is still shown. Perhaps the validation should not be triggered when all of the affected entities are locked.

@tyrasd tyrasd added the usability An issue with ease-of-use or design label Mar 7, 2024
@1ec5
Copy link
Collaborator

1ec5 commented Mar 7, 2024

Yes, not every survey point mapped in OSM was imported from a source with authoritative precision, but I would say that accidentally moved survey points are a bigger issue than misplaced survey points which are not correctable easily in iD.

True, it isn’t a big inconvenience in the grand scheme of things. However, the lock design seems superficially like the Wikidata lock, except that it locks something other than a field that can actually get in the way when mapping – and it doesn’t protect the feature from getting deleted. Will we consider using the same affordance for anything else?

Conceptually, I don’t see a difference between these survey points and other things that non-experts should think twice before touching, such as boundaries and (takes a deep breath) abandoned railways. A filter would be a more robust solution to this problem, because the user has an opportunity to keep immovable features from cluttering the map as they map other types of features around them. A filter wasn’t an option for Wikidata-tagged features because that’s a secondary attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability An issue with ease-of-use or design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide/lock survey points survey_point shouldn't be moved
3 participants