[patch] GeoImageLayer: ImageEntry enhancements for image property dialog

[holgermappt]

This patch adds functionality needed for an image property dialog (in the PhotoAdjust plugin).

List of changes:

• ImageEntry was changed from a data container into a real Class. E.g. constructors added, extractExif() added.
• Method extractExif() was moved from class GeoImageLayer to class ImageEntry.
• Consider temporary variable in method ImageEntry.getExifImgDir().
• Method ImageEntry.getThumbnail() added.
• ImageEntry.setPos(): Allow to clear the position (by setting it to null).
• ImageEntry.setExifImgDir() changed type of argument exifDir from double to Double to be able to clear the image direction (by setting it to null). This requires a re-compilation of other code that calls this method. The original setExifImgDir(double exifDir) is commented out in case the re-compilation is a problem. Otherwise it can be removed from the code. The only code with that issue that I found is the PhotoAdjust plugin.
• ImageEntry: Added methods getTmp() and discardTmp(). This allows to access the temporary variable from plugins.
• ImageViewerDialog: Added the possibility to disable the center view button with setCentreEnabled().
  • Please check the synchronized section. The idea is that the instance that disables the button also enables it. In case two threads try to disable the button simultaneously the one that actually disables it gets the return value true, the other thread gets a false because it didn't disable the button.
• ImageViewerDialog: Round the elevation.

[simon04]

Replying to holgermappt:

• Method extractExif() was moved from class GeoImageLayer to class ImageEntry.

I would use the constants in SystemOfMeasurement for converting to km/h.

• Method ImageEntry.getThumbnail() added.

I would also add setThumbnail and make the field private.

• ImageEntry.setExifImgDir() changed type of argument exifDir from double to Double to be able to clear the image direction (by setting it to null). This requires a re-compilation of other code that calls this method. The original setExifImgDir(double exifDir) is commented out in case the re-compilation is a problem. Otherwise it can be removed from the code. The only code with that issue that I found is the PhotoAdjust plugin.

Alright. No need to modify GeoImageSessionImporter.

• ImageEntry: Added methods getTmp() and discardTmp(). This allows to access the temporary variable from plugins.

Can getTmp() be implemented using cleanTmp()?

• ImageViewerDialog: Added the possibility to disable the center view button with setCentreEnabled().
  • Please check the synchronized section. The idea is that the instance that disables the button also enables it. In case two threads try to disable the button simultaneously the one that actually disables it gets the return value true, the other thread gets a false because it didn't disable the button.

I think this function can be simplified to:

    public static synchronized boolean setCentreEnabled(boolean value) {
        final ImageViewerDialog instance = getInstance();
        final boolean wasEnabled = instance.tbCentre.isEnabled();
        instance.tbCentre.setEnabled(value);
        instance.tbCentre.getAction().actionPerformed(new ActionEvent(instance.tbCentre, 0, null));
        return wasEnabled;
    }

[holgermappt]

Replying to simon04:

Replying to holgermappt:
Alright. No need to modify GeoImageSessionImporter.

I changed the type in GeoImageSessionImporter to be consistent with the other Double fields like speed and elevation.

Can getTmp() be implemented using cleanTmp()?

getTmp() returns the current tmp and creates a new one if it doesn't exist. cleanTmp() always creates a new tmp and sets the tmp position. I don't see how one method can use the other or be replaced with the other one.

• ImageViewerDialog: Added the possibility to disable the center view button with setCentreEnabled().
  • Please check the synchronized section.

I think this function can be simplified to:

Looks good.

The documentation of ImageEntry.getPos() starts with /* instead of /** in 12255-v2.patch​.

The other modifications are fine.

The Returns the cached temporary ... from [9243#file47] is not right. It uses the temporary version only if it exists and the regular one otherwise.

[Don-vip]

Replying to holgermappt:

The Returns the cached temporary ... from [9243#file47] is not right. It uses the temporary version only if it exists and the regular one otherwise.

Feel free to fix documentation also, you have a better knowledge than me of these parts of code :)

[simon04]

Replying to holgermappt:

Can getTmp() be implemented using cleanTmp()?

getTmp() returns the current tmp and creates a new one if it doesn't exist. cleanTmp() always creates a new tmp and sets the tmp position. I don't see how one method can use the other or be replaced with the other one.

I meant:

    public ImageEntry getTmp() {
        if (tmp == null) {
            cleanTmp();
        }
        return tmp;
    }

[holgermappt]

Replying to simon04:

Can getTmp() be implemented using cleanTmp()?

I meant:

    public ImageEntry getTmp() {
        if (tmp == null) {
            cleanTmp();
        }
        return tmp;
    }

No, that doesn't work because it clears the position. I replaced cleanTmp() with createTmp() and modified CorrelateGpxWithImages.StatusBarUpdater.statusText() accordingly. It should make more sense now.

Other changes in v4 compared to v3:

• Use ImageEntry.discardTmp() in CorrelateGpxWithImages.
• Update of the Returns the cached temporary ... documentation strings.
• Added ImageEntry.loadThumbnail(). This was something I wanted before, but did not know how to implement it.
  • Extended class ThumbsLoader to have a version that operates on a single image entry.
• Fixed the "Do not load thumbnails that were loaded before" condition in class ThumbsLoader.
• Added and updated documentation strings.

[simon04]

The problem I see w/ ImageEntry#loadThumbnail is that it fetches the thumbnail synchronously, i.e., it blocks. Maybe we can move/generalize GeoImageLayer#startLoadThumbs somehow. Or call the ThumbsLoader from the plugin?

I'll commit the other parts of this patch for now since the patch is already complex enough. :)

[simon04]

In 9270/josm:

see #12255 - GeoImageLayer: ImageEntry enhancements for image property dialog (patch by holgermappt, modified)

[holgermappt]

Replying to simon04:

The problem I see w/ ImageEntry#loadThumbnail is that it fetches the thumbnail synchronously, i.e., it blocks. Maybe we can move/generalize GeoImageLayer#startLoadThumbs somehow. Or call the ThumbsLoader from the plugin?

I think on the image entry level it should be blocking. I load the thumbnail because I want to use it. Same like extractExif(). If I don't want it to block, then something has to be done on a higher level. In my case I need a single thumbnail for a dialog where some of the ImageEntry properties can be modified. It would take too long to load all thumbnails with GeoImageLayer#startLoadThumbs. It's too much overhead to build a temporary GeoImageLayer with just that one ImageEntry and then call GeoImageLayer#startLoadThumbs on that layer. I can move ImageEntry#loadThumbnail into my plugin, but then I need the ThumbsLoader constructor that operates on a single image entry.

In my opinion ThumbsLoader.loadThumb() should be part of ImageEntry to keep all knowledge about an image at one place. At least the part that calculates the thumbnail. But so far I didn't understand the role of MediaTracker and whether it is a good idea to create a new tracker for each image. Same for the cache code.

I'll commit the other parts of this patch for now since the patch is already complex enough. :)

Thank you for your time to review my changes.

[simon04]

In 9277/josm:

see #12255 - Add ImageEntry#loadThumbnail (based on patch by holgermappt)

[simon04]

I generalized your patch for ImageEntry#loadThumbnail a bit …

[tomi@…]

I think this change broke reading of exif timestamps for me. I was trying to correlate images to gpx today and current josm couldn't do it, it didn't see gps timestamps at all. When I downgraded to 9229, everything works again. This change seems to be the only relevant one between 9229 and 9327. Can someone please check if they can reproduce this? If not, I'll be happy to provide a .jpg that josm fails to read. Thanks.

[Don-vip]

thanks for feedback. Can you please post a jpg and a gpx file?

[bartosomail@…]

Since I updated to photoadjust plugin to Version 31953, I sometimes find myself with the "Center view" button disabled (greyed) in the Geotagged Images pane. It happened a few times, while I worke with the image postitioning. I still haven't find a way to reproduce the problem. Restarting JOSM gets the button enabled again.

[tomi@…]

Replying to Don-vip:

thanks for feedback. Can you please post a jpg and a gpx file?

​http://store.lisk.in/tmp/josm-gps-correlation-bug.zip

[Don-vip]

Confirmed. r9253 works, r9272 doesn't.

[Don-vip]

Bug found, fix in progress.

[Don-vip]

In 9329/josm:

fix #12255 - fix EXIF time parsing regression

[tomi@…]

Wow, that was quick, thanks. :-)

[Don-vip]

Replying to bartosomail@…:

Since I updated to photoadjust plugin to Version 31953, I sometimes find myself with the "Center view" button disabled (greyed) in the Geotagged Images pane. It happened a few times, while I worke with the image postitioning. I still haven't find a way to reproduce the problem. Restarting JOSM gets the button enabled again.

this ticket is for core features, please create a new one for issues with plugins.

[Don-vip]

Replying to tomi@…:

Wow, that was quick, thanks. :-)

new tested on the way. Thanks for quick feedback!

[holgermappt]

Replying to Don-vip:

Bug found, fix in progress.

The fix is to move setExifTime(ExifReader.readTime(file)); a few lines up? Can you give a hint what caused the problem?

[tomi@…]

I think the problem was that it was below this:

477             if (dirGps == null) {
478                 setExifCoor(null);
479                 setPos(null);
480                 return;
481             }

[Don-vip]

dirGps is null with this image. Previously this case was handled by setExifTime(ExifReader.readTime(file)) being duplicated before the call to extractExif().

[holgermappt]

I see. In that case setExifTime() should be moved further up right after:

        if (file == null) {
            return;
        }

in case catch (CompoundException | IOException p) is triggered and the function returns.
The original code called setExifTime() before extractExif() was called. It was not duplicated before, there was no setExifTime() in the original extractExif(). I moved it into extractExif() but didn't consider all cases that let the function return.

[Don-vip]

Replying to holgermappt:

In that case setExifTime() should be moved further up

Yep, you're right.

[Don-vip]

In 9331/josm:

see #12255 - add robustness