openstreetmap / merkaartor

Home of Merkaartor, an openstreetmap mapping program.
http://merkaartor.be/
Other
292 stars 79 forks source link

Trim string values before uploading #232

Open norbertwenzel opened 3 years ago

norbertwenzel commented 3 years ago

Initially, let me say that I am not sure if this is an issue at all. I recognized this from an OSM Map Note complaining about a "missing" way. The problem was that the way was tagged as highway=␣unclassified (with a space in front of "unclassified"). This change was introduced in version 7 by a user of Merkaartor 18.2 (see corresponding changeset). The result was that none of the map styles available on the OSM page rendered the way at all.

I don't know whether it is still possible to carry out such uploads with the current version 18.4. At least I couldn't find a matching ticket description so I assume this has not been reported yet. If this has already been changed between 18.2 and 18.4, I'm sorry for the noise.

Assuming this has not been changed and it is still possible to upload data with leading or trailing spaces, I would suggest that newly edited fields be trimmed either as they are entered or before the changes are uploaded to OSM. Frankly I was assuming that the trimming would be done by the server before accepting changes, but obviously it is not. I am not aware of any formalization of the OSM key/value format, but I think these are only intended as trimmed keys/values.

Would you agree these values should be trimmed on your side? If so, do you have any preference where to apply the trimming (and maybe some hints where to find the corresponding code)?

Krakonos commented 3 years ago

Thanks for the report. As you noted, the root problem is that there are no verifiable key/value rules. But you are also right we could apply some basic checks.

I can see a few possible entrypoints: 1) On data entry, in TagModel class. Function TagModel::setData() already checks for some things (duplicity tags), it could also check leading/traling whitespace and act accordingly. 2) On data upload time, perhaps showing warnings in the upload dialog and offering corrections. There is currently no framework to do this. 3) Passively, in the renderer and Tags panel. We could detect this in already existing data and mark appropriately. There is currently no framework to do this.

Unless you want to dig deep, I'd suggest option 1. The action taken should not be forceful - I really hate the unwanted autocorrect behavior, and though there probably is no legitimate reason for having leading/trailing white spaces, I think there is potential to build framework for future checks.

I can see this implemented as a list of functions for each known tag, and one list for all tags (like this one or empty string), where the function returns a tuple with enum showing the result (OK/Warning/Perhaps even error) and an optional suggestion. The code can then easily loop through all the validating functions and run them in sequence, asking the user to apply corrections if applicable.

Do you want to have a stab at this?

norbertwenzel commented 3 years ago

Yes, I would like to give it a try if you don't mind.

Krakonos commented 3 years ago

Sure, I appreciate it! Don't hesitate to ask for assistance if you get stuck. Perhaps upload your incomplete code and open a PR early so we can cooperate if necessary.

Recoil016 commented 3 years ago

Possibly related: apparently Merkaartor is uploading tags that have empty strings as their value such as the following: https://www.openstreetmap.org/way/946464896 https://www.openstreetmap.org/way/943575472 https://www.openstreetmap.org/way/946482590