[Patch] Geotagged images dialog doesn't always show expected image

[GerdP]

What steps will reproduce the problem?

1. have three geo image layers, named geo3, geo2 and geo1 from top to bottom
2. click on one image icon for layer geo1
3. click on another image icon for layer geo3
4. note which image is shown (one from geo3 in my case)
5. press y to hide the dialog
6. press y again to show the dialog

What is the expected result?

Same image for geo3 is shown

What happens instead?

The image for geo1 is shown

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

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-01-03 16:22:55 +0100 (Wed, 03 Jan 2024)
Revision:18934
Build-Date:2024-01-04 02:31:00
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18934 de) Windows 10 64-Bit
OS Build number: Windows 10 Home 2009 (19045)
Memory Usage: 495 MB / 1972 MB (353 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: de_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, -DXss50k, -Djpackage.app-path=%UserProfile%\AppData\Local\JOSM\HWConsole.exe]

Last errors/warnings:
- 00000.503 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.506 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00000.902 E: java.security.KeyStoreException: Windows-ROOT not found. Ursache: java.security.NoSuchAlgorithmException: Windows-ROOT KeyStore not available

[GerdP]

I think this is another regression from r18924?
The geo image dialog is a bit unreliable right now, one has to double check each time if the displayed image is really showing the wanted place. I think this started with r18591 and r18924 made it worse.

[GerdP]

This patch fixes also the memory leak reported in #23414, as both are related.

[GerdP]

also make sure that the title is updated when no image is selected

[taylor.smock]

Your patch makes sense to me.

I think this started with r18591 and r18924 made it worse.

Definitely a possibility; prior to that, it kept track of a single image. Post r18591, it kept track of an image per layer.

[GerdP]

Thanks for review. Do you want me to commit it before 23.12 ?

[taylor.smock]

If you feel that it is sufficiently tested, go for it.
Worst case scenario, I get to have fun doing a hotfix release.

[taylor.smock]

Now that I think about it, someone ought to look at extracting most of the logic from ImageViewerDialog so we can actually write non-regression tests. But that is more of a longterm thing.

I'll spend some time doing that if there are more issues found.
Taylor hopes there are no new issues found

[GerdP]

In 18937/josm:

fix #23415 and #23414, patch 23415-v2.patch

• fixes issues where the wrong image is shown
• properly destroys the toggle dialog and the toggle action when no more geotagged image layer is available
• make sure that the title is updated when no image is selected (instead of possibly showing an image name from a closed layer

[GerdP]

Taylor hopes there are no new issues found

Maybe check #22860 and #22806 (same one-line patch for both)

[taylor.smock]

The tests got broken.

I bet readding destroyInstance(); to the destroy() method will fix it.

[GerdP]

Maybe. But that will not work in real life. I don't yer understand how these tests are related.
Better revert r18937 if you want to release today.

[taylor.smock]

It'll be tomorrow -- the latest build gets updated sometime overnight (for me). I think it is a cron job.

[GerdP]

Something with the unit test setup seems wrong. When I execute e.g. ImagesLoaderTest allone it passes.

[taylor.smock]

Here is what happens:
1) An ImageViewerDialog is created and added to the toggle dialog list
2) The test finishes, and the toggle dialogs have destroy called on them
3) The next test starts, and reuses the toggle dialog that was destroyed in (2)
4) The next test finishes, and the destroy method is called again, which is where things fail.

So, when the toggle dialog is destroyed, we must ensure that the static instance is set to null.

[taylor.smock]

In 18938/josm:

See #23415: Fix unit tests from r18937

[GerdP]

This doesn't work. The toggle action isn't destroyed anymore.

[taylor.smock]

What do you mean, it isn't destroyed? It should be; literally the first thing that happens when destroy is called is dialogsPanel = null (after removing itself from the dialogsPanel).

[GerdP]

1. Start JOSM
2. Drag& drop an image to have one image layer
3. delete the image layer

This should disable the toggle action 'y', but it doesn't. If you add another image layer the menu shows two entries with Geotagged Images 'y'.

[taylor.smock]

In 18939/josm:

See #23415/r18938: Use titleBar instead of dialogsPanel

[GerdP]

In 18940/josm:

see #23415/r18939 ( MarkerLayerTest still failed)

• restore most of the old behaviour of destroy() and destroyInstance() as it was before r18937
• don't call destroy inside updateLayers() as this code is also used in unit tests
• check in layerRemoving() if dialog should be destroyed

Hope this now fixes all unit tests. Some of them fail on my machine because the expect to run in English while my machine is configured to German. Have to find out where this can be changed...

[taylor.smock]

Replying to GerdP:

Hope this now fixes all unit tests. Some of them fail on my machine because the expect to run in English while my machine is configured to German. Have to find out where this can be changed...

This is probably an "easy" fix. We probably just have to add the @I18n annotation to the JosmDefaults annotation.

[GerdP]

If you tell me where to put that I can try.

[taylor.smock]

test/unit/org/openstreetmap/josm/testutils/annotations/JosmDefaults.java

@@ -34 +43 @@
  */
 @Target({ElementType.TYPE, ElementType.METHOD})
 @Retention(RetentionPolicy.RUNTIME)
+@I18n
 @ExtendWith(JosmDefaults.DefaultsExtension.class)
 public @interface JosmDefaults {
     /**

[GerdP]

Doesn't help. I've also tried @I18n("en") and both also in MarkerLayerTest which fails with

Testcase: org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayerTest
Test: testNonRegression23316() took 2 sec(s)
Test: testMarkerLayer() took 1 sec(s) FAILED: expected: <0 markers> but was: <0 Wegpunkte>
org.opentest4j.AssertionFailedError: expected: <0 markers> but was: <0 Wegpunkte>
	at org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayerTest.testMarkerLayer(MarkerLayerTest.java:57)

Test: testPlayHeadMarker() took 598 milli sec(s)

New ticket?

[taylor.smock]

Probably. I'm assuming that eclipse is running tests with -Djunit.jupiter.extensions.autodetection.enabled=true (for the JosmDefaults -- you'd probably have a lot more failing tests if it wasn't).

[GerdP]

I run the tests with ant, in Eclipse many tests don't work.

[GerdP]

see #23418