(WIP Patch) First geotagged image not fully selected

[richlv]

What steps will reproduce the problem?

1. Load a few geotagged images.
2. Notice the first being highlighted in red in the map view, but not shown in the image viewer, and also the image controls being disabled.

What is the expected result?

The first image is fully selected - displayed, and sequence can be stepped through, same as when correlating images to GPX.

What happens instead?

As per the reproduction steps.

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

This is additionally cumbersome in a longer editing session, when loading images on top of many other existing layers - it requires finding and clicking an image in the map view, instead of being able to center on the first image from the viewer.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-06-03 18:08:14 +0200 (Mon, 03 Jun 2024)
Revision:19096
Build-Date:2024-06-07 01:31:15
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (19096 en_GB) Mac OS X 13.6.7
OS Build number: macOS 13.6.7 (22G720)
Memory Usage: 732 MB / 4096 MB (296 MB allocated, but free)
Java version: 21+35-2513, Oracle Corporation, OpenJDK 64-Bit Server VM
Look and Feel: com.apple.laf.AquaLookAndFeel
Screen: Display 4128833 1920×1080 (scaling 1.00×1.00) Display 2077750265 1680×1050 (scaling 2.00×2.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→16×16, 32×32→32×32
System property file.encoding: UTF-8
System property sun.jnu.encoding: UTF-8
Locale info: en_GB
Numbers with default locale: 1234567890 -> 1234567890

Plugins:
+ FIT (36248)
+ HouseNumberTaggingTool (36226)
+ InfoMode (36126)
+ Mapillary (3904)
+ PicLayer (1.0.3)
+ apache-commons (36273)
+ apache-http (36273)
+ buildings_tools (36226)
+ dataimport (36066)
+ ejml (36176)
+ geotools (36273)
+ imagery_offset_db (36226)
+ jackson (36273)
+ jaxb (36118)
+ jna (36273)
+ jts (36004)
+ measurement (36256)
+ opendata (36256)
+ pbf (36176)
+ photo_geotagging (36276)
+ reverter (36256)
+ undelete (36226)
+ utilsplugin2 (36241)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1

Last errors/warnings:
- 00003.625 W: Update plug-ins - You updated your JOSM software. To prevent problems the plug-ins should be updated as well.  Update plug-ins now?

[GerdP]

I think the behaviour depends on the availability of geo information in the images.
On my machine the image viewer dialog typically doesn't open when I drag&drop some images.

[richlv]

It somewhat does, yes.
The image viewer always pops up correctly for me, but the first image is selected in it only if none of the images has geotags.
One image having geotags breaks this.

Also notice how the first image is shown as selected in the map view as expected, but this status is disconnected from the image viewer.

[GerdP]

I have problems to reproduce your results and I don't know exactly what to expect. I tested with r19096 and empty preferences.
For me, the image viewer only pops up when I drop one or more images without geotags.
If I do that two popup dialogs are opened, one asking for a gpx file and on top the image viewer loaded with the older image:

No red marker is shown because JOSM doesn't have any position to show. When I load a corresponding GPX track and select that in the popup a red marker is shown. When I click the "next" button in the image viever the next image gets the red marker, so for me the red marker means something "last shown image in the corresponding layer" while you seem to think it should mark the first (oldest?) image.

Anyhow, I think JOSM should open the image viewer dialog with the oldest image when I drag&drop one or more images, no matter if there is geo info or not.

[GerdP]

I also tried with expert mode enabled and got the same results. Your image shows a gray arrow next to the red marker. Is that the (mouse) cursor? Or can I activate that arrow somehow in preferences?

[GerdP]

My first idea was to always open the image viewer when a geoimage layer is created, but there is a problem with this idea: When the user opens a session file with several geoimage layers they surely don't want the image viewer to popup with all first images.

[GerdP]

Work in progress patch

[GerdP]

The patch implements these changes:
1) always open image viewer when one or more images were opened with the file importer
2) removes the code in GeoImageLayer which always opened the ImageViewer when all images are without geo location.
3) adds a new Action "Show first image" to the context menu of the GeoImageLayer.

Maybe the new Action could be changed to just open the image viewer with the last shown image (if known) or first (if not known).

[richlv]

Thank you so much for looking into this. I'll try to cover all the points, please let me know if I miss any.

a) I get image viewer automatically open when I load images with and without geotags. If images have no geotags, correlation window also opens. This is the same on both Mac and Windows.
From plugins that might affect this, common to both is Mapillary.

b) The icon turning red indeed marks the currently selected image, sorry if the initial description was confusing.

c) The grey arrow is a direction marker. It is displayed when the image has the direction (compass) tag.

d) The proposed changes sound great (especially if we can figure out why we get different behaviour for geotagged images). Opening last shown/first sounds great as well.

e) Session file with multiple image layers - there was recently work to make image viewer tabbed (for example, to switch in tabs between local & Mapillary images). Perhaps that can be used here? Although it would likely be a much bigger change, might be out of scope here.

[GerdP]

reg. a) OK, I'll try again with the Mapillary plugin
reg. e) My session file contains +100 image layers (one for each date at which I collected OSM data in the last three years), therefore opening all layers is not an option.

[GerdP]

Even with the Mapillary plugin I cannot reproduce your problems (using tested version r19096). Maybe there is a setting in the plugin which changes the behaviour?

[richlv]

Woah, such a session usage is quite an interesting usecase :)

When you load geotagged images, is the first (oldest) image always selected and displayed, and image navigation works?

[GerdP]

When you load geotagged images, is the first (oldest) image always selected and displayed, and image navigation works?

I think I noticed at least two times now that the image dialog was opened automatically without showing an image. But more often the dialog is not opened.
As before: If two or more images are loaded and non image has geolocation then the two popups are opened as shown in the screen shot.
(The code to force this is removed in my patch. It looks like this:

        if (getInvalidGeoImages().size() == data.size()) {
            this.data.setSelectedImage(this.data.getFirstImage());
            // We do have to wrap the EDT call in a worker call, since layers may be created in the EDT.
            // And the layer must be added to the layer list in order for the dialog to work properly.
            MainApplication.worker.execute(() -> GuiHelper.runInEDT(() -> ImageViewerDialog.getInstance().displayImages(this.getSelection())));
        }

[GerdP]

Changes compared to first version of the patch:

• layer action is now named "Show image" and will either show the first or the last selected image
• check if there is an ImageViever instance before using it in GeoImageLayer.paint(). Maybe this prevents the unexpected empty popup dialogs.

[richlv]

Yep, "image dialog was opened automatically without showing an image" was my primary problem.
Why it does not consistently open for you seems mysterious, but I'd be happy to re-test later to see whether any weird behaviour remains.

[taylor.smock]

I was going to ask if you (GerdP) considered putting showFirstImage in IGeoImageLayer, but now that I've thought about it a bit, I can see the argument for not putting it in IGeoImageLayer (do all GeoImageLayer's have a "first" image?).

Beyond that, slight styling/formatting issue: I know if statements don't need {} in this case (L1003 in GeoImageLayer), but I'd rather have them to avoid hard-to-see bugs in the future. I've fixed some hard-to-see bugs as I fix PMD issues (example​), and I'd rather avoid a case where someone comes along and adds another statement to one of the branches (specifically the else), but doesn't add the {}.

[GerdP]

Reg. formatting: Unfortunately all my JOSM specific settings got lost with the recent change to maven. I'll try to restore my old settings soon. I don't know if it is possible to have no "first" image, but I added a check anyway.
The patch is still WIP, I added you because I am not sure about the change in paint() and I think we still have those sporadic failures in the unit tests?

[taylor.smock]

Unfortunately all my JOSM specific settings got lost with the recent change to maven

Sorry. :(

added you because I am not sure about the change in paint()

I missed the change in paint(). Did you intend to attach another file diff? Or did I miss something in the patch?

I think we still have those sporadic failures in the unit tests

Yep. I think I fixed one source for the sporadic failures. I've customized @JosmDefaults locally to try and catch tests that are polluting the environment. Unfortunately, I suspect that the sporadic failures are caused by race conditions of some sort.

EDIT: To clarify on the race condition: test makes off-thread call to create dialog -> dialog creates _after_ test finishes -> next test starts -> test tries to make new dialog -> dialog fails because it was already created

[GerdP]

reg. Eclipse settings: I've now restored the .settings folder from r19096 and changed only the setting reg. java 1.8.
I have to find out if that still works...

I missed the change in paint(). Did you intend to attach another file diff? Or did I miss something in the patch?

The patch only shows the added lines in paint()

 	510	        if (!ImageViewerDialog.hasInstance())
 	511	            return;
 	512	       

I just tried to find places where ImageViewerDialog.getInstance() is called but possibly not needed.

[GerdP]

@richlv: I think I understand now why you see a different behaviour. My images don't have compass info and there is code in JOSM which might open the image viever dialog without any image when compass info is found in an image. My images never have this info.

[richlv]

Ahhhhh. Seems like a great finding, I guess such inconsistent behaviour should not be there?

[GerdP]

I really don't know yet. The problem occured in the past, see .e.g #10628. It's hard to say what behaviour we actually want because there are so many different use cases and because of multithreading code is not always executed in the same order.

I fear my patch justs adds more complexity...

[GerdP]

I just tried with an older version 18590 from https://josm.openstreetmap.de/download/Archiv/josm-snapshot-18590.jar as that was the last before r18591 which introduced tabs in the image viewer.
This old version behaved very different, e.g. the layer action "Jump to next marker" opens the image viewer, so it did something that my new action "Show Image" would do.
With version 19096 the "Jump to next marker" action doesn't seem to do anything on my machine.

[GerdP]

Next patch which introduces less new code:

• remove code in GeoImageLayer constructor which more or less randomly opens the ImageViewerDialog. The constructor is not the right place to do this as a session import may open many of these layers.
• fix GeoImageLayer.jumpToNextMarker() and GeoImageLayer.jumpToPreviousMarker()' so that they open/update the ImageViewerDialog instead of just modifying the selected image (as before r18591). This also allows to open the ImageViewerDialog` for a geoimage layer, so a new layer action "Show Image" seems obsolete.
• new code to check if a new geoimage layer was added by any open file or drag/drop action and - if so - to open the first image of the topmost new geoimage layer. If ImageViewerDialog is already open a new tab is added.

@Taylor: Reg. the change in OpenFileAction: This also does not seem to be the right place to check for new geoimage layers as other actions like OpenLocationAction or plugins also may add one or more image layers and my understanding is that we always want to show the first image of the topmost new geoimage layer. Maybe a new method MainLayerManager.addLayers(List<Layer> list) would do the trick? It would allow such post processing whenever one or more layers are added.

[GerdP]

I'll commit the 3rd patch tomorrow if nobody suggest a better solution.

[GerdP]

In 19123/josm:

fix #23728: First geotagged image not fully selected

• remove code in GeoImageLayer constructor which more or less randomly opens the ImageViewerDialog
• fix layer actions "Jump to next marker" and "Jump to previous marker" so that they open or update the image viewer dialog
• new code to check if a new geoimage layer was added by any open file or drag/drop action and - if so - to open the first image of the topmost new geoimage layer. If ImageViewerDialog is already open a new tab is added.

[GerdP]

I just noticed a small issue: When I save a session file with geo image layer(s) that shos a specific position JOSM used to open the file and show exactly that position. Now, it will jump to the position of the first image of the topmost layer and I have to use shortcut 8 "Zoom to previous" to see the stored position. New Ticket?

[Adrian]

session file

[Adrian]

I don't know whether this might be relevant. I noticed this issue a long time ago but I don't think I ever created a ticket for it. To reproduce (with r19126):

1. Launch JOSM or delete all layers if JOSM is already running.
2. Open the attached .jos (session) file.
3. Open the .jos file a second time.
4. Note that the map view has changed (use shortcuts 8 and 9 to compare the views). There has been an improvement because the change used to be much larger.

I would expect the map view to be the same each time.

[GerdP]

Not sure if I understand.
If it happened before r19123 and also with r19126 please open a new ticket. With older versions I would expect that JOSM zooms to the position given in the session file:

    <viewport>
        <center lat="43.30103510775562" lon="3.489452090172532"/>
        <scale meter-per-pixel="0.134882"/>
    </viewport>

[Adrian]

Follow-up to ticket:23728#comment:27 and ticket:23728#comment:28 : #23797