Modify ↓

Opened 4 years ago

Last modified 3 years ago

#20024 new defect

No warning when multipolygon preset is applied to multipolygon

Reported by: GerdP Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: template_report Cc: Klumbumbus, skyper

Description (last modified by GerdP)

What steps will reproduce the problem?

Follow up of #19951

  1. Select a multipolygon relation
  2. Presets -> Relations -> Multipolygon
  3. In popup click "New relation" (for whatever reason)

What is the expected result?

Either a message that selection is invalid or that the new multipolygon relation has the ways of the existing multipolygon when relation editor is openend.

What happens instead?

Relation editor opens with the multipolygon relation as member.

Please provide any additional information below. Attach a screenshot if possible.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-10-03 13:42:38 +0200 (Sat, 03 Oct 2020)
Build-Date:2020-10-04 01:30:47
Revision:17084
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17084 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2004 (19041)
Memory Usage: 1109 MB / 3641 MB (606 MB allocated, but free)
Java version: 1.8.0_221-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920x1080 (scaling 1.0x1.0)
Maximum Screen Size: 1920x1080
Best cursor sizes: 16x16 -> 32x32, 32x32 -> 32x32
VM arguments: [-XX:StartFlightRecording=name=MyRecording2,settings=d:\dbg\gerd.jfc, -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true,dumponexitpath=e:\ld\perf_20201103_091714.jfr]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35579)
+ apache-commons (35524)
+ buildings_tools (35579)
+ continuosDownload (91)
+ ejml (35313)
+ geotools (35169)
+ jaxb (35092)
+ jts (35122)
+ o5m (35248)
+ opendata (35513)
+ pbf (35636)
+ poly (35248)
+ reltoolbox (35602)
+ reverter (35579)
+ undelete (35521)
+ utilsplugin2 (35624)

Validator rules:
+ c:\josm\core\resources\data\validator\geometry.mapcss

Last errors/warnings:
- 00593.190 W: Unsaved changes - <html>The relation has been changed.<br><br>Do you want to save your changes?</html>
- 00594.205 W: Unsaved changes - <html>The relation has been changed.<br><br>Do you want to save your changes?</html>

Attachments (2)

preset-with-mp.PNG​ (77.4 KB ) - added by GerdP 4 years ago.
20024.patch​ (1.5 KB ) - added by GerdP 4 years ago.
this one should work

Download all attachments as: .zip

Change History (30)

by GerdP, 4 years ago

Attachment: preset-with-mp.PNG​ added

comment:2 by GerdP, 4 years ago

Description: modified (diff)

comment:3 by GerdP, 4 years ago

Also with useful selection the result is rather poor compared to Tools->Create multipolygon. Tags from ways are not moved to the relation. It would be easy to change that.

comment:4 by stoecker, 4 years ago

Presets currently only allow new relations as far as I remember. Wrong selection is not given because of

<role key="subarea" text="Sub area" requisite="optional" type="relation" />

comment:5 by GerdP, 4 years ago

You can also apply a preset to an existing relation. subarea is allowed for type=boundary, not for multipolygon.

comment:6 by GerdP, 4 years ago

BTW: With latest there is the same warning as for a node: "Multipolygon is not closed"

in reply to:  5 comment:7 by stoecker, 4 years ago

Replying to GerdP:

You can also apply a preset to an existing relation.

Didn't remember this is possible for relation type presets. Anyway it only sets type=multipolygon ;-)

subarea is allowed for type=boundary, not for multipolygon.

Ah sorry. Right.

For Multipolygon: Questions is why is "New relation" offered, when none of the roles supports a relation. That's a bug.

That there are better methods to create multipolygons is another case, but should not mixed with this function (maybe add a note to the preset).

by GerdP, 4 years ago

Attachment: 20024.patch​ added

this one should work

comment:8 by GerdP, 4 years ago

Milestone: → 20.11
Owner: changed from team to GerdP
Status: new → assigned

For Multipolygon: Questions is why is "New relation" offered, when none of the roles supports a relation. That's a bug.

See attached patch.

comment:9 by stoecker, 4 years ago

That only checks the type. Correct? Properly implemented it also should check the counts ;-)

comment:10 by GerdP, 4 years ago

Right. Would be better.

comment:11 by Don-vip, 4 years ago

Milestone: 20.11 → 20.12

Milestone renamed

comment:12 by GerdP, 4 years ago

Properly implemented it also should check the counts ;-)

I thought we have such a calculation in PropertiesDialog, but even that only checks for the existence of types, not the count.

There is another glitch: When you select a route relation and use the multipolygon preset it might change the type to multipolygon without showing a warning or a dialog.
Not sure what a dialog program should do, I guess the multipolygon preset (and many others) simply shouldn't be enabled, but that probably would slow down JOSM significantly.
So, should I even commit my simple patch?

in reply to:  12 comment:13 by skyper, 4 years ago

Replying to GerdP:

There is another glitch: When you select a route relation and use the multipolygon preset it might change the type to multipolygon without showing a warning or a dialog.

This behavior is normal for <key /> but why is relation a valid type for multipolygon? See defaultpresets.xml line 8551​ 

        <item name="Multipolygon" icon="presets/misc/multipolygon.svg" type="multipolygon,relation" preset_name_label="true">

should be changed to:

        <item name="Multipolygon" icon="presets/misc/multipolygon.svg" type="multipolygon" preset_name_label="true">

I guess.

comment:14 by GerdP, 4 years ago

Do we have a situation where we have a new relation without type and want to be able to change that to type=multipolygon using the preset?

comment:15 by stoecker, 4 years ago

Most of the presets overwrite conflicting keys without asking. Probably that could be solved somehow generic (like displaying the changes you do when applying the preset)?

but why is relation a valid type for multipolygon

If you have following similar workflows:

  • create an untagged way and call highway preset
  • create an untagged relation and call preset multipolygon

in reply to:  14 comment:16 by GerdP, 4 years ago

Replying to GerdP:

Do we have a situation where we have a new relation without type and want to be able to change that to type=multipolygon using the preset?

Yes, if you download relation 7052561(version 1) with members this might be a candidate.

comment:17 by GerdP, 4 years ago

Probably that could be solved somehow generic (like displaying the changes you do when applying the preset)?

I guess we cannot change presets to contain enough information to compute a result like "this preset should not be applied to the selected elements because it makes no sense to combine tag x with tag y".
One of the reasons why I don't use presets is that they somehow hide what's actually changed in the data.
The changes are displayed in the command that is created, but I doubt that those mappers how use presets will look at them in detail.
A possible scenario would be to run validator on the changed objects and display the changes "before - after", but that might make JOSM unusable.

in reply to:  17 comment:18 by stoecker, 4 years ago

Replying to GerdP:

Probably that could be solved somehow generic (like displaying the changes you do when applying the preset)?

I guess we cannot change presets to contain enough information to compute a result like "this preset should not be applied to the selected elements because it makes no sense to combine tag x with tag y".
One of the reasons why I don't use presets is that they somehow hide what's actually changed in the data.
The changes are displayed in the command that is created, but I doubt that those mappers how use presets will look at them in detail.
A possible scenario would be to run validator on the changed objects and display the changes "before - after", but that might make JOSM unusable.

A thought more like a section at the bottom which compares tags before and now and shows something like

  • add x = y
  • change x from y to z
  • remove x = y

Probably only in expert mode?

comment:19 by GerdP, 4 years ago

Similar to the history dialog for tags?

comment:20 by GerdP, 4 years ago

Probably only in expert mode?

Funny, I would do it the other way around. Experts should know what they do, but others might want to learn what tags are involved when a specific preset is used. In case of implementation we should have a preference to switch it on/off.

comment:21 by skyper, 4 years ago

+1 for displaying changes to the user.

For relations without type=* a general preset for relations could be added with a combo box for type=*. Also have a look at #19012 for negative matches to exclude certain keys/tags from matching.

in reply to:  19 comment:22 by stoecker, 4 years ago

Replying to GerdP:

Similar to the history dialog for tags?

Don't know. Is there a screenshot somewhere?

comment:23 by GerdP, 4 years ago

Change tags in an exisitng object and press Ctrl+H to view the history. We could use that showing only the changes.

comment:24 by GerdP, 4 years ago

Cc: Klumbumbus skyper added
Owner: changed from GerdP to team
Status: assigned → new

My small patch only fixes the tip of the iceberg. I am not that familiar with presets, so I leave that those who are experts in this.

comment:25 by simon04, 4 years ago

Milestone: 20.12 → 21.01

comment:26 by Don-vip, 3 years ago

Milestone: 21.01 → 21.02

comment:27 by stoecker, 3 years ago

Milestone: 21.02 → 21.03

Milestone renamed

comment:28 by Don-vip, 3 years ago

Milestone: 21.03

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to GerdP.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment
E-mail address and name can be saved in the Preferences .
Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.