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

[editor] Removed superfluous name=* restrictions #8036

Merged
merged 1 commit into from
May 3, 2024

Conversation

map-per
Copy link
Member

@map-per map-per commented Apr 30, 2024

closes #1481

  • Removed code that prevents users from adding a multilingual name in case it is the same as name=*. In uni lingual countries the OSM convention is that e.g. name:de should be the same as name. (see: [Android] Editor ignores name in russian if it's the same as default name #1481)

  • Allowed users to remove names / submit an empty name. This is e.g. helpful when the name of a POI (e.g. restaurant) changes. OM users can then update the multilingual names in languages they know and remove the wrong names in languages they don't know (no multilingual name is better than a wrong one) (see: Deleting tags not possible in the editor #6292)

@map-per map-per mentioned this pull request Apr 30, 2024
5 tasks
@map-per
Copy link
Member Author

map-per commented Apr 30, 2024

Tested it here: https://pewu.github.io/osm-history/#/node/11073340805

  • ❌ Deleting names results in a blank name instead of deleting the name tag

@biodranik
Copy link
Member

@map-per that's a bug, there is no need to push an empty tag. It should be cleared properly in the core, ideally even before upload/merging with osm starts, right at the moment of saving it.

@kirylkaveryn You've recently touched this code in #8032 , it would be great if you cooperate to finish these changes.


void EditableMapObject::RemoveNeedlessNames()
{
RemoveBlankAndDuplicationsForDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Removing blank names is still needed, right? So the code above with !name.empty() should stay? Or can it be done more efficiently in another way?
CC @kirylkaveryn

Copy link
Contributor

Choose a reason for hiding this comment

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

This method is used in ios to clean up empty names and prepare the list of languages that can be added to make a translation.

Maybe it would be better to set nil to the names that shouldn't be added to the list of changes (when a user adds a name but decides to skip this change later and delete translation from the list) and empty string when a user cleans up names manually?

Copy link
Member

Choose a reason for hiding this comment

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

This is a good question: how tag removal is encoded now in the OM editor core? Is it supported at all? If not, should empty values signal that these keys should be removed from the OSM at the tag merging stage, when there are local tags, and remote tags?

Signed-off-by: map-per <map-per@gmx.de>
@map-per
Copy link
Member Author

map-per commented May 3, 2024

I limited the PR to only fix the identical names problem.

Test: ✔️ Works as intended (https://pewu.github.io/osm-history/#/node/5433124122)

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! @kirylkaveryn can you please confirm that the behavior on iOS is exactly the same?

@biodranik biodranik merged commit 60f6db5 into organicmaps:master May 3, 2024
15 checks passed
@map-per map-per deleted the name2 branch May 4, 2024 06:33
@biodranik
Copy link
Member

@map-per Strange, I was sure that I had invited you to our closed chat for active contributors... Ping me (biodranik) if you are interested and have a Telegram or Matrix account.

@map-per
Copy link
Member Author

map-per commented May 12, 2024

You did ask, I'm just slow, but I created a Matrix account now (and wrote you).

@biodranik
Copy link
Member

@map-per for some reason, I don't see any messages in Elements matrix client (it s%cks btw...), can you please ping me again, so I can re-invite you?

@map-per
Copy link
Member Author

map-per commented May 12, 2024

I tried again, but both times the request was instantly rejected. Maybe it works the other way round, my account is: -

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Editor ignores name in russian if it's the same as default name
3 participants