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

Preset field inheritance #5712

Merged
merged 6 commits into from
Jan 15, 2019
Merged

Preset field inheritance #5712

merged 6 commits into from
Jan 15, 2019

Conversation

quincylvania
Copy link
Collaborator

@quincylvania quincylvania commented Jan 14, 2019

Closes #5710.

The biggest user-facing implication of this change is that, by default, specified presets will retain the generic fields that define them. So the shop field will be present on essentially all shop features. I actually don't mind this behavior. It let's users quickly fix mistagged or accidentally-tagged features (see #5611), and more clearly indicates how the tagging/preset system works. If we do go this route I think it'd be wise to rename fields named Type with more specific labels.

screen shot 2019-01-15 at 10 31 13 am

Todo

  • Update the documentation

@quincylvania quincylvania added preset An issue with an OpenStreetMap preset or tag field An issue with a field in the user interface wip Work in progress labels Jan 14, 2019
@quincylvania quincylvania self-assigned this Jan 14, 2019
@bhousel
Copy link
Member

bhousel commented Jan 15, 2019

Hmm, can we resolve these in preset.init() instead of at build time? I imagine that people who are replacing them with a custom set of presets would want to use this feature too..

@quincylvania
Copy link
Collaborator Author

Hmm, can we resolve these in preset.init() instead of at build time? I imagine that people who are replacing them with a custom set of presets would want to use this feature too..

Oh that's a good idea, thanks! Let me know if anything else about this needs work.

…initialization

Removed the data build check for duplicate values between fields and moreFields
Renamed the shop field from Type to Shop Type
Renamed the beauty field from Shop Type to Beauty Specialty
Added the brand field to the shop preset under moreFields
@quincylvania quincylvania removed the wip Work in progress label Jan 15, 2019
@quincylvania quincylvania requested a review from bhousel January 15, 2019 15:24
@quincylvania
Copy link
Collaborator Author

@bhousel This should be ready to merge. The data changes are fairly large though so it'd be nice to get your thoughts.

@bhousel
Copy link
Member

bhousel commented Jan 15, 2019

The biggest user-facing implication of this change is that, by default, specified presets will retain the generic fields that define them. So the shop field will be present on essentially all shop features. I actually don't mind this behavior.

Hmm I kind of don't like this part, but mainly because the tags themselves are arbitrary and messy and we can't easily translate them. In the example shown above, showing "Shop Type" of doityourself would be confusing to both non-English and English speakers. I'm imagining that people who don't know any better might try to replace that with "do it yourself", thinking that they are helping.

I see the main purpose of iD's preset system as: hiding the ugliness of OSM tagging from most users.

I really like the idea of preset field inheritance though! It would make a lot of the preset definitions much simpler.

You said in #5710:

An alternative solution would be to allow targeting a specific preset, but I think "parent" inheritance fits most cases.

I wanted to think about this a bit before responding, and I really like this idea of targeting a specific preset a bit better than the {inherit} keyword. The reason is because of how arbitrary OSM tags are. We want to inherit the "building like" fields (address, opening hours) for shop=* but also lots of amenity=*, tourism=*, leisure=*..

I don't see OSM tagging as having a clear inheritance where, e.g. all the amenities get some fields and all the leisure ones get different fields. It's all mixed up.

Would you be ok with - instead of {inherit} - allowing a preset id to appear in there?
The preset ids are automatically generated from the file path and name, so for example, all vending machines could inherit fields from {amenity/vending}.

@quincylvania
Copy link
Collaborator Author

@bhousel Thanks for the feedback!

I definitely see your point about the messy tags, I'll try to come up with a fix. I think eventually we'll need to figure out a way to give translatable labels to the common values of combo fields without limiting them to a predetermined list. As you said, doityourself is confusing but a user could still encounter that option in the shop field before this change.

Would you be ok with - instead of {inherit} - allowing a preset id to appear in there?
The preset ids are automatically generated from the file path and name, so for example, all vending machines could inherit fields from {amenity/vending}

For some reason I thought this would be difficult or confusing but now that you write it out it looks really cool and more obvious. Allowing the same preset to inherit from multiple others would be pretty powerful. I'll look into this more.

@quincylvania
Copy link
Collaborator Author

@bhousel Like you suggested, I changed it so specific presets must be targeted instead of {inherit}. This can work with multiple targets.

I kept it so the parent preset's fields are still used if the field properties are omitted. I think this behavior is helpful for quickly adding new presets and it can easily be overridden.

I'm not sure what the best method is for ignoring the "Type" fields of target presets. I might strip the target's fields that map to the tags defining the source preset, but I'm not sure that's desirable in all cases.

@quincylvania
Copy link
Collaborator Author

I'm not sure what the best method is for ignoring the "Type" fields of target presets. I might strip the target's fields that map to the tags defining the source preset, but I'm not sure that's desirable in all cases.

I ended up doing this. I'd consider changing it back once we can figure out translatable labels for common combo field values. It's not exactly intuitive that some fields are inherited but not all. For now I think it works alright. This PR is ready to merge on my end.

screen shot 2019-01-15 at 1 55 29 pm

Copy link
Member

@bhousel bhousel left a comment

Choose a reason for hiding this comment

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

Looks great! OK to merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
field An issue with a field in the user interface preset An issue with an OpenStreetMap preset or tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preset field inheritance
2 participants