[patch] To pictures are saving all coordinates, this saved earlier too

[anonymous]

What steps will reproduce the problem?

1. Add pictures 1,2,3 on map using photoadjust plugin
2. Save coordinates for 1,2,3 to EXIF using option from right click on photo layer (photo_geotagging plugin)
3. Step 1 for pictures 4,5,6
4. Step 2, but now are saving all 1,2,3,4,5,6

What is the expected result?

In step 4 should be saved only 4,5,6

What happens instead?

In step 4 are saving all pictures, this saved earlier too

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

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2015-01-27 20:36:55
Last Changed Author: Don-vip
Revision: 7989
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Relative URL: ^/trunk
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2015-01-27 21:33:31 +0100 (Tue, 27 Jan 2015)
Last Changed Rev: 7989

Identification: JOSM/1.5 (7989 pl) Linux Ubuntu 14.04.1 LTS
Memory Usage: 204 MB / 869 MB (45 MB allocated, but free)
Java version: 1.7.0_75, Oracle Corporation, OpenJDK 64-Bit Server VM
Java package: openjdk-7-jre:amd64-7u75-2.5.4-1~trusty1
Dataset consistency test: No problems found

Plugins:
- CADTools (1002)
- InfoMode (30892)
- OpeningHoursEditor (30892)
- PicLayer (30892)
- editgpx (30892)
- photo_geotagging (30892)
- photoadjust (30905)
- reverter (30892)

Last errors/warnings:
- E: java.net.SocketTimeoutException: Read timed out. Przyczyna: java.net.SocketTimeoutException: Read timed out
- E: java.net.SocketTimeoutException: Read timed out. Przyczyna: java.net.SocketTimeoutException: Read timed out
- E: java.net.SocketTimeoutException: Read timed out. Przyczyna: java.net.SocketTimeoutException: Read timed out
- E: java.net.SocketTimeoutException: Read timed out. Przyczyna: java.net.SocketTimeoutException: Read timed out
- E: java.net.SocketTimeoutException: Read timed out. Przyczyna: java.net.SocketTimeoutException: Read timed out

[holgermappt]

OK, we do not clear the new-GPS-data flag, so it is saved again and again. There was never a concept to clear it, but it's probably a good idea to add that. I can look at this. JOSM core needs to be changed (ImageEntry, unflagNewGpsData()), then photo_geotagging needs to call that after the GPS data was written. Problem is that other components might still need that information.

Is there a reason why you want to save the GPS information after three pictures? I position all photos first and save the coordinates when I'm done with that. Works fine up to 100 photos. You can even save the session and continue later, JOSM will remember what pictures you touched in the previous session.

[bastiK]

Replying to holgermappt:

OK, we do not clear the new-GPS-data flag, so it is saved again and again. There was never a concept to clear it, but it's probably a good idea to add that. I can look at this. JOSM core needs to be changed (ImageEntry, unflagNewGpsData()), then photo_geotagging needs to call that after the GPS data was written.

Sounds good.

Problem is that other components might still need that information.

Not at the moment, as far as I'm aware.

[anonymous]

Replying to holgermappt:

Is there a reason why you want to save the GPS information after three pictures? I position all photos first and save the coordinates when I'm done with that. Works fine up to 100 photos. You can even save the session and continue later, JOSM will remember what pictures you touched in the previous session.

I have photos from many places in country taken few years ago, is not easy to find his place. I position 100 photos, save (don't want lost when system crash), position next 100 photos, etc. I saw this (too many pictures are saving), so I reported. I don't have another reason for this.

[holgermappt]

New method unflagNewGpsData(), modified infoText.

[holgermappt]

Remove new GPS data flag, do not use gpsTime.

[holgermappt]

Here are two patches, one for JOSM core (GeoImageLayer_unflagNewGpsData.patch) and one for the photo_geotagging plugin (photo_geotagging_unflagNewGpsData.patch). The core patch must be applied first and then the minimum JOSM version needs to be set in photo_geotagging/build.xml (now set to 9999).

I had to change GeoImageLayer.getImages() to return a list with the actual images instead of a list of image clones. Is there a reason to clone the images?

I also added the number of images with modified GPS data to the GeoImageLayer infoText.

photo_geotagging shows a list of files with modified GPS data that will be written to the image files. I can select one or more of the file paths, but all are written. Was the idea to write only the files that were selected? If not it would be good if it is not possible to select files. Otherwise the user will wonder why the unselected files were modified too.

[skyper]

@holgermappt:
Thanks for the patches. Please always add [patch] at the beginning of the ticket's summary when attaching a patch. Otherwise it will not be listed under report/8 and might get forgotten.

[holgermappt]

@skyper: I didn't add [patch] because there is an open question. For me patch means something that is ready to be used. Right now the patch files are more work in progress with pending review and test.

[bastiK]

In 8041/josm:

applied #11059 - New method unflagNewGpsData(), modified infoText (patch by holgermappt)

[bastiK]

In [o30967]: applied ​#josm11059 - Remove new GPS data flag, do not use gpsTime (patch by holgermappt)

[bastiK]

Replying to holgermappt:

Here are two patches, one for JOSM core (GeoImageLayer_unflagNewGpsData.patch) and one for the photo_geotagging plugin (photo_geotagging_unflagNewGpsData.patch). The core patch must be applied first and then the minimum JOSM version needs to be set in photo_geotagging/build.xml (now set to 9999).

Thanks for the patches!

I had to change GeoImageLayer.getImages() to return a list with the actual images instead of a list of image clones. Is there a reason to clone the images?

I think it is general defensive programming style. But if you need access to the actual images, this has to be adapted of course.

I also added the number of images with modified GPS data to the GeoImageLayer infoText.

photo_geotagging shows a list of files with modified GPS data that will be written to the image files. I can select one or more of the file paths, but all are written. Was the idea to write only the files that were selected? If not it would be good if it is not possible to select files. Otherwise the user will wonder why the unselected files were modified too.

Yes, this is a problem and should be fixed. Either not allow to select files or change it to a table with checkboxes in front (like in a browser email website).

[bastiK]

Replying to holgermappt:

@skyper: I didn't add [patch] because there is an open question. For me patch means something that is ready to be used. Right now the patch files are more work in progress with pending review and test.

For patches of this scope, I will only do a review by reading the diff and expect you have done some basic tests.

Anyway, from my perspective, there was nothing wrong with the patches so the marker would have been correct.

[bastiK]

Replying to bastiK:

Replying to holgermappt:

photo_geotagging shows a list of files with modified GPS data that will be written to the image files. I can select one or more of the file paths, but all are written. Was the idea to write only the files that were selected? If not it would be good if it is not possible to select files. Otherwise the user will wonder why the unselected files were modified too.

Yes, this is a problem and should be fixed. Either not allow to select files or change it to a table with checkboxes in front (like in a browser email website).

Now that I think about it, checkboxes aren't so great for block selection with Shift & Ctrl. Maybe a dynamic text below the list would be better, showing the selected vs. total images. Or a confirmation checkbox.

[holgermappt]

Replying to bastiK:

Now that I think about it, checkboxes aren't so great for block selection with Shift & Ctrl. Maybe a dynamic text below the list would be better, showing the selected vs. total images. Or a confirmation checkbox.

I think the user interface is okay as it is. Just the function to update only the selected files is missing. All should be selected by default. For me this is a nice-to-have feature with low priority.