Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12255 closed enhancement (fixed)

[patch] GeoImageLayer: ImageEntry enhancements for image property dialog

Reported by: holgermappt Owned by: team
Priority: normal Milestone: 16.01
Component: Core image mapping Version:
Keywords: regression Cc:

Description

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.

Attachments (4)

GeoImageLayer_ImageDialog.patch (19.9 KB ) - added by holgermappt 8 years ago.
12255-v2.patch (24.7 KB ) - added by simon04 8 years ago.
12255-v3.patch (24.7 KB ) - added by simon04 8 years ago.
12255-v4.patch (28.7 KB ) - added by holgermappt 8 years ago.

Download all attachments as: .zip

Change History (31)

by holgermappt, 8 years ago

comment:1 by Don-vip, 8 years ago

Milestone: 16.01

in reply to:  description ; comment:2 by simon04, 8 years ago

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;
    }

by simon04, 8 years ago

Attachment: 12255-v2.patch added

in reply to:  2 ; comment:3 by holgermappt, 8 years ago

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.

in reply to:  3 comment:4 by Don-vip, 8 years ago

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 :)

in reply to:  3 ; comment:5 by simon04, 8 years ago

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;
    }

by simon04, 8 years ago

Attachment: 12255-v3.patch added

in reply to:  5 comment:6 by holgermappt, 8 years ago

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.

by holgermappt, 8 years ago

Attachment: 12255-v4.patch added

comment:7 by simon04, 8 years ago

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. :)

comment:8 by simon04, 8 years ago

In 9270/josm:

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

in reply to:  7 comment:9 by holgermappt, 8 years ago

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.

comment:10 by simon04, 8 years ago

In 9277/josm:

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

comment:11 by simon04, 8 years ago

Resolution: fixed
Status: newclosed

I generalized your patch for ImageEntry#loadThumbnail a bit …

comment:12 by tomi@…, 8 years ago

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.

comment:13 by Don-vip, 8 years ago

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

comment:14 by bartosomail@…, 8 years ago

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.

in reply to:  13 comment:15 by tomi@…, 8 years ago

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

comment:16 by Don-vip, 8 years ago

Keywords: regression added
Resolution: fixed
Status: closedreopened

Confirmed. r9253 works, r9272 doesn't.

comment:17 by Don-vip, 8 years ago

Bug found, fix in progress.

comment:18 by Don-vip, 8 years ago

Resolution: fixed
Status: reopenedclosed

In 9329/josm:

fix #12255 - fix EXIF time parsing regression

comment:19 by tomi@…, 8 years ago

Wow, that was quick, thanks. :-)

in reply to:  14 comment:20 by Don-vip, 8 years ago

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.

in reply to:  19 comment:21 by Don-vip, 8 years ago

Replying to tomi@…:

Wow, that was quick, thanks. :-)

new tested on the way. Thanks for quick feedback!

in reply to:  17 comment:22 by holgermappt, 8 years ago

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?

Last edited 8 years ago by holgermappt (previous) (diff)

comment:23 by tomi@…, 8 years ago

I think the problem was that it was below this:

477             if (dirGps == null) {
478                 setExifCoor(null);
479                 setPos(null);
480                 return;
481             }
Last edited 8 years ago by Don-vip (previous) (diff)

comment:24 by Don-vip, 8 years ago

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

comment:25 by holgermappt, 8 years ago

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.

in reply to:  25 comment:26 by Don-vip, 8 years ago

Replying to holgermappt:

In that case setExifTime() should be moved further up

Yep, you're right.

comment:27 by Don-vip, 8 years ago

In 9331/josm:

see #12255 - add robustness

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.