Modify

Opened 4 weeks ago

Closed 4 weeks ago

#19508 closed defect (fixed)

[PATCH] Don't suggest to change incline=0% to -0% when reversing or combining ways

Reported by: Klumbumbus Owned by: team
Priority: normal Milestone: 20.07
Component: Core Version:
Keywords: template_report reverse switch Cc:

Description

What steps will reproduce the problem?

  1. have a way highway=secondary incline=0%
  2. reverse the way

What is the expected result?

reversed way

What happens instead?

JOSM asks to change incline=0% to incline=-0%

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

from ticket:19485#comment:10

same for incline=0° and incline=0

there are about 25k such combinations

https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/corrector/ReverseWayTagCorrector.java?rev=16438#L145

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-07-07 09:21:17 +0200 (Tue, 07 Jul 2020)
Build-Date:2020-07-08 01:30:50
Revision:16739
Relative:URL: ^/trunk

Identification: JOSM/1.5 (16739 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1909 (18363)
Memory Usage: 784 MB / 1820 MB (334 MB allocated, but free)
Java version: 1.8.0_251-b08, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680x1050 (scaling 1.0x1.0)
Maximum Screen Size: 1680x1050
Best cursor sizes: 16x16 -> 32x32, 32x32 -> 32x32
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=C:\Program Files (x86)\josm-latest.jnlp, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Djnlpx.splashport=61109, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Attachments (4)

19508v1.patch (2.8 KB) - added by gaben 4 weeks ago.
The patch is based on the modified regex, plus added some test cases. The test cases were swapped I fixed that as well.
19508v2.patch (3.2 KB) - added by gaben 4 weeks ago.
Suggestions applied, improved test cases
19508v3.patch (3.3 KB) - added by gaben 4 weeks ago.
Also remove sign from null value, test case updated
19508v4.patch (3.8 KB) - added by gaben 4 weeks ago.
Exception handling added

Download all attachments as: .zip

Change History (14)

Changed 4 weeks ago by gaben

Attachment: 19508v1.patch added

The patch is based on the modified regex, plus added some test cases. The test cases were swapped I fixed that as well.

comment:2 Changed 4 weeks ago by gaben

Summary: Don't suggest to change incline=0% to -0% when reversing or combining ways[PATCH] Don't suggest to change incline=0% to -0% when reversing or combining ways

comment:3 Changed 4 weeks ago by gaben

Please review and test :)

comment:4 Changed 4 weeks ago by stoecker

  • (\\d*[,.]?\\d*)(.*) should be be ([0-9,.]+) or you will loose .1 which is valid.
  • Why do you add the sign to the number parser?
  • The reversed number should not use a "," anymore if the original had one. Use fixedNum there as well.
  • Any reason why you reverse the arguments in the test?
  • parseDouble probably throws exceptions which should be catched.
Last edited 4 weeks ago by stoecker (previous) (diff)

comment:5 in reply to:  4 ; Changed 4 weeks ago by gaben

Thanks @stoecker for the quick review!

Replying to stoecker:

  • (\\d*[,.]?\\d*)(.*) should be be ([0-9,.]+) or you will loose .1 which is valid.
  • Why do you add the sign to the number parser?
  • The reversed number should not use a "," anymore if the original had one. Use fixedNum there as well.
  • Any reason why you reverse the arguments in the test?
  • parseDouble probably throws exceptions which should be caught.
  • Try it out :) .1 will become -.1. Just noticed, the current version (wasn't modified since 2009) will leave this as is.
  • It doesn't affect the output, but thanks for noticing.
  • I didn't want any behaviour change. If you think it should be fixed here, instead of the validator, I can modify the patch.
  • Yes. The first parameter is the expected one, the second is the actual, so what the function returns. (Causes confusion in the IDE.)
  • Yeah probably when someone gives a really big number, I wanted to ask what to do here :) So what to do? Log somewhere something, or suppress (return the original) or something else?
Last edited 4 weeks ago by gaben (previous) (diff)

comment:6 Changed 4 weeks ago by simon04

Keywords: reverse switch added
Milestone: 20.07

comment:7 in reply to:  5 Changed 4 weeks ago by stoecker

  • Try it out :) .1 will become -.1. Just noticed, the current version (wasn't modified since 2009) will leave this as is.

Ah sorry. Thought there is a + instead of * for the first numbers.

  • It doesn't affect the output, but thanks for noticing.
  • I didn't want any behaviour change. If you think it should be fixed here, instead of the validator, I can modify the patch.

Not instead, but also. When we anyway modify values we can also fix them. E.g. boolean values are always converted to yes/no regardless of the original (which I believe is a major reason than true/false and 0/1 vanished from the database).

  • Yeah probably when someone gives a really big number, I wanted to ask what to do here :) So what to do? Log somewhere something, or suppress (return the original) or something else?

I'd simply handle it as != 0, i.e. change the sign.

Changed 4 weeks ago by gaben

Attachment: 19508v2.patch added

Suggestions applied, improved test cases

Changed 4 weeks ago by gaben

Attachment: 19508v3.patch added

Also remove sign from null value, test case updated

comment:8 Changed 4 weeks ago by stoecker

I'd still add a try{} around the parse otherwise it's dangerous.

Changed 4 weeks ago by gaben

Attachment: 19508v4.patch added

Exception handling added

comment:9 Changed 4 weeks ago by gaben

I hope this time everything looks good :) If you find anything, please feel free to modify the patch.

comment:10 Changed 4 weeks ago by stoecker

Resolution: fixed
Status: newclosed

In 16771/josm:

fix #19508 - better reversal function for number, patch by gaben

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.