[WIP Patch] Double check if error still exists before executing autofix

[GerdP]

What steps will reproduce the problem?

1. load attached file with duplicate relations
2. run validator, should produce error "Duplicated relations (1)"
3. double click on the error entry so that both relations are selected
4. select one of the two relations
5. change name from "same" to "other"
6. click on Fix button without running validator again

What is the expected result?

No further change, error was fixed manually

What happens instead?

One relation is deleted

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: 1144 MB / 3641 MB (661 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_20201017_154712.jfr]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35579)
+ PolygonCutOut (v0.7)
+ apache-commons (35524)
+ buildings_tools (35579)
+ continuosDownload (91)
+ ejml (35313)
+ geotools (35169)
+ jaxb (35092)
+ jts (35122)
+ merge-overlap (35583)
+ o5m (35248)
+ opendata (35513)
+ pbf (35446)
+ poly (35248)
+ reverter (35579)
+ undelete (35521)
+ utilsplugin2 (35580)

[GerdP]

Similar problem with other tests like DuplicateWay. Both don't re-evaluate the problem with the primitives in their current state.

[GerdP]

Patch contains a few minor changes, this is the important part:

        // see #19956 check again
        Test test2 = new DuplicateRelation();
        test2.startTest(NullProgressMonitor.INSTANCE);
        toFix.forEach(test2::visit);
        test2.endTest();
        boolean errorStillExists = false;
        for (TestError e : test2.getErrors()) {
            if (e.getCode() == testError.getCode() && e.getPrimitives().size() >= 2) {
                // refresh primitives, might be fewer than before
                toFix = e.primitives(Relation.class).collect(Collectors.toSet());
                errorStillExists = true;
                break;
            }
        }
        if (!errorStillExists)
            return null;

I don't like the idea to implement such a double check in each test that offers an autofix (11 java tests in core) and I think the problem also exists in the mapcss tests.

I'll try again to code this in double check in ValidatorDialog.

[GerdP]

implement double check in class TestError, should work for all java tests, but cannot double check fixable mapcss errors

[GerdP]

In 17252/josm:

fix #19956 Double check if error still exists before executing autofix

• let ValidatorDialog do this so that different manual changes or autofixes don't disturb each other

[simon04]

This has caused a few compilation/error-prone warnings:

    [javac] Compiling 7 source files to /home/simon/src/josm.svngit/build
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/data/validation/TestError.java:562: warning: [deprecation] newInstance() in Class has been deprecated
    [javac]             tester2 = getTester().getClass().newInstance();
    [javac]                                             ^
    [javac]   where T is a type-variable:
    [javac]     T extends Object declared in class Class
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/data/validation/TestError.java:562: warning: [ClassNewInstance] Class.newInstance() bypasses exception checking; prefer getDeclaredConstructor().newInstance()
    [javac]             tester2 = getTester().getClass().newInstance();
    [javac]                                                         ^
    [javac]     (see https://errorprone.info/bugpattern/ClassNewInstance)
    [javac]   Did you mean 'tester2 = getTester().getClass().getDeclaredConstructor().newInstance();'?
    [javac] 3 warnings

Have you noticed any performance implications when fixing a large number of primitives (say, 100 opening_hours warnings)?

[GerdP]

This has caused a few compilation/error-prone warnings:

Yes, found that yesterday but I found no better solution with Java 8. The suggested change doesn't work with JDK 8 (1.8.0_201)

Have you noticed any performance implications when fixing a large number of primitives (say, 100 opening_hours warnings)?

I didn't try but I would not be surprised. Do you have test data for me? If opening_hours test does the double check on its own - or doesn't have to - we could introudce a flag in class Test. Or I might limit the double check to a fixed list of tests.

[GerdP]

Replying to GerdP:

This has caused a few compilation/error-prone warnings:

Yes, found that yesterday but I found no better solution with Java 8. The suggested change doesn't work with JDK 8 (1.8.0_201)

Forget that. Seems I was mislead by Eclipse. I just have to add more code to the catch blocks.

[GerdP]

Seems I have to exclude OpeningHoursTest, it produces EDT violations with this code.

[GerdP]

In 17261/josm:

see #19956 Double check if error still exists before executing autofix

• revert r17252. There are more tests that don't work with this. Have to double check my patch first ;)

[Don-vip]

Milestone renamed

[stoecker]

Milestone renamed