Modify

Opened 3 months ago

Last modified 4 weeks ago

#20024 new defect

No warning when multipolygon preset is applied to multipolygon

Reported by: GerdP Owned by: team
Priority: normal Milestone: 21.01
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 3 months ago.
20024.patch (1.5 KB) - added by GerdP 3 months ago.
this one should work

Download all attachments as: .zip

Change History (27)

Changed 3 months ago by GerdP

Attachment: preset-with-mp.PNG added

comment:1 Changed 3 months ago by GerdP


comment:2 Changed 3 months ago by GerdP

Description: modified (diff)

comment:3 Changed 3 months ago by GerdP

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 Changed 3 months ago by stoecker

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 Changed 3 months ago by GerdP

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

comment:6 Changed 3 months ago by GerdP

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

comment:7 in reply to:  5 Changed 3 months ago by stoecker

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).

Changed 3 months ago by GerdP

Attachment: 20024.patch added

this one should work

comment:8 Changed 3 months ago by GerdP

Milestone: 20.11
Owner: changed from team to GerdP
Status: newassigned

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 Changed 3 months ago by stoecker

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

comment:10 Changed 3 months ago by GerdP

Right. Would be better.

comment:11 Changed 2 months ago by Don-vip

Milestone: 20.1120.12

Milestone renamed

comment:12 Changed 2 months ago by GerdP

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?

comment:13 in reply to:  12 Changed 2 months ago by skyper

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 Changed 2 months ago by 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?

comment:15 Changed 2 months ago by stoecker

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

comment:16 in reply to:  14 Changed 2 months ago by GerdP

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 Changed 2 months ago by 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.

comment:18 in reply to:  17 Changed 2 months ago by stoecker

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 Changed 2 months ago by GerdP

Similar to the history dialog for tags?

comment:20 Changed 2 months ago by GerdP

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 Changed 2 months ago by skyper

+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.

comment:22 in reply to:  19 Changed 2 months ago by stoecker

Replying to GerdP:

Similar to the history dialog for tags?

Don't know. Is there a screenshot somewhere?

comment:23 Changed 2 months ago by GerdP

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

comment:24 Changed 7 weeks ago by GerdP

Cc: Klumbumbus skyper added
Owner: changed from GerdP to team
Status: assignednew

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 Changed 4 weeks ago by simon04

Milestone: 20.1221.01

Modify Ticket

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

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.