[Patch] Validation results may not be refreshed on upload

[GerdP]

What steps will reproduce the problem?

1. Load attached file
2. run validator, note the "Unnamed ways (1)" warning
3. try to fix it, e.g. add noname=yes
4. hit upload

What is the expected result?

pre-upload validation is executed and the warning disappears

What happens instead?

pre-upload validation seems to be executed but warning is still shown

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

I think this always happens when the pre-upload validation doesn't find anything to warn about.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-01-16 08:06:33 +0100 (Tue, 16 Jan 2024)
Revision:18940
Build-Date:2024-01-17 02:31:01
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18940 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19045)
Memory Usage: 273 MB / 1888 MB (151 MB allocated, but free)
Java version: 17.0.8+7-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: en_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djpackage.app-version=1.5.18789, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Djpackage.app-path=%UserProfile%\AppData\Local\JOSM\HWConsole.exe]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (36196)
+ RoadSigns (36196)
+ apache-commons (36176)
+ buildings_tools (36200)
+ comfort0 (36200)
+ o5m (36126)
+ pbf (36176)
+ poly (36126)
+ reltoolbox (36200)
+ reverter (36196)
+ undelete (36126)
+ utilsplugin2 (36200)

Validator rules:
+ d:\java_tools\JOSM\mygeometry.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1
+ c:\josm\core\resources\data\validator\geometry.mapcss

Last errors/warnings:
- 00000.499 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.502 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00000.830 E: java.security.KeyStoreException: Windows-ROOT not found. Cause: java.security.NoSuchAlgorithmException: Windows-ROOT KeyStore not available
- 00007.168 W: Failed to add c:\josm\core\resources\data\validator\geometry.mapcss to tag checker
- 00007.169 W: java.lang.IllegalArgumentException: Unknown MapCSS base selector o

[GerdP]

regression of r18850

[GerdP]

possible fix

[GerdP]

The patch checks if the validator dialog is already showing. If yes, it is always updated.
An empty dialog is only suppressed when there was no dialog showing and there are no errors and the validation was triggered by the upload action.

[GerdP]

Not sure if this test can done outside of the EDT task?

I asked that because I was afraid about performance issues in unit tests, but we don't reach that code in batch, right? I guess it could be done but that would require an additional if block.
So, I am happy with the patch despite the wrong naming (23340.patch instead of 23440.patch)

[taylor.smock]

Not sure if this test can done outside of the EDT task?

I think it can be done outside of the EDT -- I don't believe we are waiting on anything in the EDT from the tests anyway.

[GerdP]

My problem is that I never fully understand why some things have to be done in EDT, so I am always afraid to break something.
So I don't want lots of code in the GuiHelper.runInEDT(() block which doesn't have to be threre.
Do you want to suggest a different approach?

[taylor.smock]

Replying to GerdP:

My problem is that I never fully understand why some things have to be done in EDT, so I am always afraid to break something.

The only things that need to be done on the EDT is stuff directly related to the UI (example: telling a dialog that it needs to show itself needs to be done on the EDT).
Otherwise another thread should be used -- we don't do this consistently though. This is so that the UI isn't "bogged down" or otherwise appears unresponsive/frozen.

So I don't want lots of code in the GuiHelper.runInEDT(() block which doesn't have to be threre.

Totally fair. In this case, the runInEDT block is about as minimal as it could be; we could pull the MainApplication.getMap() call out, but it probably wouldn't be worth it as a "standalone" change.

                MapFrame map = MainApplication.getMap(); // This could be pulled out
                map.validatorDialog.unfurlDialog(); // This modifies the UI state of a dialog
                map.validatorDialog.tree.setErrors(errors); // As does this
                MainApplication.getLayerManager().getLayersOfType(ValidatorLayer.class).forEach(ValidatorLayer::invalidate); // This also ''should'' be called from the EDT, although I think it might be a bit more permissive.
                if (!errors.isEmpty()) {
                    OsmValidator.initializeErrorLayer(); // IIRC, this does some stuff on the EDT, so it needs to be called on the EDT.
                }

[GerdP]

So, this would be better, right?

src/org/openstreetmap/josm/data/validation/ValidationTask.java

@@ -135 +135 @@
             this.errors.removeIf(error -> error.getSeverity().getLevel() >= Severity.OTHER.getLevel());
         }
 
-        if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null && (!beforeUpload || !errors.isEmpty())) {
+        if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null) {
+            MapFrame map = MainApplication.getMap();
             // update GUI on Swing EDT
             GuiHelper.runInEDT(() -> {
-                MapFrame map = MainApplication.getMap();
+                if (!map.validatorDialog.isShowing() && errors.isEmpty() && beforeUpload)
+                    return;
                 map.validatorDialog.unfurlDialog();
                 map.validatorDialog.tree.setErrors(errors);
                 //FIXME: nicer way to find / invalidate the corresponding error layer

[taylor.smock]

Technically yes, that is better; realistically, no. The MainApplication.getMap() call is very cheap. It could also be

src/org/openstreetmap/josm/data/validation/ValidationTask.java

@@ -135 +135 @@
             this.errors.removeIf(error -> error.getSeverity().getLevel() >= Severity.OTHER.getLevel());
         }
 
-        if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null && (!beforeUpload || !errors.isEmpty())) {
+        MapFrame map = MainApplication.getMap(); // This is effectively a field access
+        if (!GraphicsEnvironment.isHeadless() && map != null) {
             // update GUI on Swing EDT
             GuiHelper.runInEDT(() -> {
-                MapFrame map = MainApplication.getMap();
+                // I ''believe'' that the isShowing() method is safe to use outside of the EDT, but we probably ought to keep it in the EDT anyway. Both of the other statements are "cheap", so it doesn't matter if they are inside or outside the EDT.
+                if (!map.validatorDialog.isShowing() && errors.isEmpty() && beforeUpload)
+                    return;
                 map.validatorDialog.unfurlDialog();
                 map.validatorDialog.tree.setErrors(errors);
                 //FIXME: nicer way to find / invalidate the corresponding error layer

This works because

[GerdP]

In 18964/josm:

fix #23440: Validation results may not be refreshed on upload
The patch checks if the validator dialog is already showing. If yes, it is always updated.
An empty dialog is only suppressed when there was no dialog showing and there are no errors and the validation was triggered by the upload action.