Opened 11 months ago
Closed 11 months ago
#23440 closed defect (fixed)
[Patch] Validation results may not be refreshed on upload
Reported by: | GerdP | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | 24.01 |
Component: | Core validator | Version: | |
Keywords: | template_report | Cc: | taylor.smock |
Description
What steps will reproduce the problem?
- Load attached file
- run validator, note the "Unnamed ways (1)" warning
- try to fix it, e.g. add noname=yes
- 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
Attachments (2)
Change History (13)
by , 11 months ago
Attachment: | 23440.osm.gz added |
---|
comment:1 by , 11 months ago
comment:2 by , 11 months ago
Component: | Core → Core validator |
---|
follow-up: 6 comment:3 by , 11 months ago
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.
Not sure if this test can done outside of the EDT task?
comment:4 by , 11 months ago
Cc: | added |
---|---|
Milestone: | → 24.01 |
Summary: | Validation results may not be refreshed on upload → [Patch] Validation results may not be refreshed on upload |
comment:5 by , 11 months ago
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)
comment:6 by , 11 months ago
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.
follow-up: 8 comment:7 by , 11 months ago
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?
comment:8 by , 11 months ago
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. }
comment:9 by , 11 months ago
So, this would be better, right?
-
src/org/openstreetmap/josm/data/validation/ValidationTask.java
c:\josm\core>svn diff src
135 135 this.errors.removeIf(error -> error.getSeverity().getLevel() >= Severity.OTHER.getLevel()); 136 136 } 137 137 138 if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null && (!beforeUpload || !errors.isEmpty())) { 138 if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null) { 139 MapFrame map = MainApplication.getMap(); 139 140 // update GUI on Swing EDT 140 141 GuiHelper.runInEDT(() -> { 141 MapFrame map = MainApplication.getMap(); 142 if (!map.validatorDialog.isShowing() && errors.isEmpty() && beforeUpload) 143 return; 142 144 map.validatorDialog.unfurlDialog(); 143 145 map.validatorDialog.tree.setErrors(errors); 144 146 //FIXME: nicer way to find / invalidate the corresponding error layer
comment:10 by , 11 months ago
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()); 136 136 } 137 137 138 if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null && (!beforeUpload || !errors.isEmpty())) { 138 MapFrame map = MainApplication.getMap(); // This is effectively a field access 139 if (!GraphicsEnvironment.isHeadless() && map != null) { 139 140 // update GUI on Swing EDT 140 141 GuiHelper.runInEDT(() -> { 141 MapFrame map = MainApplication.getMap(); 142 // 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. 143 if (!map.validatorDialog.isShowing() && errors.isEmpty() && beforeUpload) 144 return; 142 145 map.validatorDialog.unfurlDialog(); 143 146 map.validatorDialog.tree.setErrors(errors); 144 147 //FIXME: nicer way to find / invalidate the corresponding error layer
This works because
regression of r18850