search

openstreetmap / id-tagging-schema

🆔🏷 The presets and other tagging data used by the iD editor
ISC License
137 stars 148 forks source link

Empty undo entry from preset change #951

Open jleedev opened 1 year ago

jleedev commented 1 year ago

URL

No response

How to reproduce the issue?

  1. Draw an area, type in 'solar', see that there are two presets "Solar Farm" and "Solar Power Plant".
  2. Try to learn the difference between the two. I see that Solar Farm gives plant:method=photovoltaic, plant:output:electricity=yes, plant:source=solar, power=plant.
  3. Switch from Solar Farm to Solar Power Plant to see what that preset contains.

Two weird things happen!

  1. The tags don't change. From a blank slate, Solar Power Plant gives only plant:source=solar, power=plant, but it doesn't remove the additional tags when switching from Solar Farm. This is confusing but I'm not sure I can describe it as a bug.
  2. A no-op undo entry is created, saying "Changed tags", but no tags were changed.

I think that if switching presets doesn't change anything, then an undo entry should not be created.

Screenshot(s) or anything else?

Screen recording 2023-04-12 09.45.45.webm

Which deployed environments do you see the issue in?

Development version at ideditor.netlify.app

What version numbers does this issue effect?

2.26.0-dev

Which browsers are you seeing this problem on?

Chrome

nickrsan commented 1 year ago

I suspect this is a bug that's related to one that @bhousel tracked down as described in openstreetmap/iD#9707. I'm leaving both open right now since they each contain valuable information, but at some point they may need to be merged.

1ec5 commented 1 year ago

There was some discussion in OSMUS Slack about this issue at the time, but I forgot to copy it here:

Solar Power Plant and Solar Farm are currently defined as two distinct presets. The latter is more specific in that it also includes plant:method=photovoltaic, while the former is intended to remove that tag. I guess that’s because a solar farm is assumed to always consist of photovoltaic solar panels, but there are plant:method=thermal solar farms too.

At the time, I suspected this might be a regression from 96bac0 along the lines of openstreetmap/iD#9372. As far as I can tell, 21e54f only fixes openstreetmap/iD#9372 if the old preset’s ID starts with the new preset’s ID. But in this case, power/plant/source/method/photovoltaic does not start with power/plant/source/solar.

https://github.com/openstreetmap/iD/blob/b1121b11753cfe8919e5665df315060d2ec04139/modules/actions/change_preset.js#L15

Anyhow, openstreetmap/iD#9707 looks like the more serious issue that should be fixed before considering whether this special case needs to be corrected.

nickrsan commented 1 year ago

OK, sounds like this may be a separate preset issue then. Tagging it and marking that we're waiting on this one for openstreetmap/iD#9707

tyrasd commented 1 year ago

@1ec5 is right about the cause of this bug. it should be easily fixed by renaming/moving the Solar Farm preset such that it can be correctly recognized as a sub-preset of the Solar Power Plant preset.