From a98c16de2842164006f3df1e8275d8461f2d9785 Mon Sep 17 00:00:00 2001 From: Dennis Guse Date: Sun, 29 Mar 2020 13:07:40 +0200 Subject: [PATCH] Removed TrackPointFactory. New TrackPoints are now always created and old ones not reused. --- .../CustomContentProviderUtilsTest.java | 60 ++++++------------- .../opentracks/content/TrackDataHub.java | 3 +- .../opentracks/content/data/TrackPoint.java | 5 -- .../provider/ContentProviderUtils.java | 20 +++---- .../content/provider/TrackPointFactory.java | 22 ------- .../content/provider/TrackPointIterator.java | 12 +--- .../io/file/exporter/FileTrackExporter.java | 3 +- .../importer/AbstractFileTrackImporter.java | 3 +- .../services/TrackRecordingService.java | 3 +- 9 files changed, 32 insertions(+), 99 deletions(-) delete mode 100644 src/main/java/de/dennisguse/opentracks/content/provider/TrackPointFactory.java diff --git a/src/androidTest/java/de/dennisguse/opentracks/content/provider/CustomContentProviderUtilsTest.java b/src/androidTest/java/de/dennisguse/opentracks/content/provider/CustomContentProviderUtilsTest.java index df6cb72c9..f4da07570 100644 --- a/src/androidTest/java/de/dennisguse/opentracks/content/provider/CustomContentProviderUtilsTest.java +++ b/src/androidTest/java/de/dennisguse/opentracks/content/provider/CustomContentProviderUtilsTest.java @@ -33,7 +33,6 @@ import org.mockito.junit.MockitoJUnitRunner; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; import de.dennisguse.opentracks.content.data.TestDataUtil; import de.dennisguse.opentracks.content.data.Track; @@ -76,68 +75,43 @@ public class CustomContentProviderUtilsTest { @Test public void testLocationIterator_noPoints() { - testIterator(1, 0, 1, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); - } - - @Test - public void testLocationIterator_customFactory() { - final TrackPoint location = new TrackPoint(new Location("test_location")); - final AtomicInteger counter = new AtomicInteger(); - testIterator(1, 15, 4, false, new TrackPointFactory() { - @Override - public TrackPoint create() { - counter.incrementAndGet(); - return location; - } - }); - // Make sure we were called exactly as many times as we had track points. - Assert.assertEquals(15, counter.get()); - } - - @Test - public void testLocationIterator_nullFactory() { - try { - testIterator(1, 15, 4, false, null); - Assert.fail("Expecting IllegalArgumentException"); - } catch (IllegalArgumentException e) { - // Expected. - } + testIterator(1, 0, 1, false); } @Test public void testLocationIterator_noBatchAscending() { - testIterator(1, 50, 100, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); - testIterator(2, 50, 50, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + testIterator(1, 50, 100, false); + testIterator(2, 50, 50, false); } @Test public void testLocationIterator_noBatchDescending() { - testIterator(1, 50, 100, true, TrackPointFactory.DEFAULT_LOCATION_FACTORY); - testIterator(2, 50, 50, true, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + testIterator(1, 50, 100, true); + testIterator(2, 50, 50, true); } @Test public void testLocationIterator_batchAscending() { - testIterator(1, 50, 11, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); - testIterator(2, 50, 25, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + testIterator(1, 50, 11, false); + testIterator(2, 50, 25, false); } @Test public void testLocationIterator_batchDescending() { - testIterator(1, 50, 11, true, TrackPointFactory.DEFAULT_LOCATION_FACTORY); - testIterator(2, 50, 25, true, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + testIterator(1, 50, 11, true); + testIterator(2, 50, 25, true); } @Test public void testLocationIterator_largeTrack() { - testIterator(1, 20000, 2000, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + testIterator(1, 20000, 2000, false); } - private List testIterator(long trackId, int numPoints, int batchSize, boolean descending, TrackPointFactory trackPointFactory) { + private List testIterator(long trackId, int numPoints, int batchSize, boolean descending) { long lastPointId = initializeTrack(trackId, numPoints); contentProviderUtils.setDefaultCursorBatchSize(batchSize); List locations = new ArrayList<>(numPoints); - try (TrackPointIterator it = contentProviderUtils.getTrackPointLocationIterator(trackId, -1L, descending, trackPointFactory)) { + try (TrackPointIterator it = contentProviderUtils.getTrackPointLocationIterator(trackId, -1L, descending)) { while (it.hasNext()) { TrackPoint loc = it.next(); Assert.assertNotNull(loc); @@ -173,7 +147,7 @@ public class CustomContentProviderUtilsTest { // Load all inserted trackPoints. long lastPointId = -1; int counter = 0; - try (TrackPointIterator it = contentProviderUtils.getTrackPointLocationIterator(id, -1L, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY)) { + try (TrackPointIterator it = contentProviderUtils.getTrackPointLocationIterator(id, -1L, false)) { while (it.hasNext()) { it.next(); lastPointId = it.getTrackPointId(); @@ -688,7 +662,7 @@ public class CustomContentProviderUtilsTest { } /** - * Tests the method {@link ContentProviderUtils#getTrackPointLocationIterator(long, long, boolean, TrackPointFactory)} in descending. + * Tests the method {@link ContentProviderUtils#getTrackPointLocationIterator(long, long, boolean)} in descending. */ @Test public void testGetTrackPointLocationIterator_desc() { @@ -704,7 +678,7 @@ public class CustomContentProviderUtilsTest { long startTrackPointId = trackpointIds[9]; - TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(trackId, startTrackPointId, true, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(trackId, startTrackPointId, true); for (int i = 0; i < trackpointIds.length; i++) { Assert.assertTrue(trackPointIterator.hasNext()); TrackPoint trackPoint = trackPointIterator.next(); @@ -715,7 +689,7 @@ public class CustomContentProviderUtilsTest { } /** - * Tests the method {@link ContentProviderUtils#getTrackPointLocationIterator(long, long, boolean, TrackPointFactory)} in ascending. + * Tests the method {@link ContentProviderUtils#getTrackPointLocationIterator(long, long, boolean)} in ascending. */ @Test public void testGetTrackPointLocationIterator_asc() { @@ -731,7 +705,7 @@ public class CustomContentProviderUtilsTest { long startTrackPointId = trackpointIds[0]; - TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(trackId, startTrackPointId, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY); + TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(trackId, startTrackPointId, false); for (int i = 0; i < trackpointIds.length; i++) { Assert.assertTrue(locationIterator.hasNext()); TrackPoint trackPoint = locationIterator.next(); diff --git a/src/main/java/de/dennisguse/opentracks/content/TrackDataHub.java b/src/main/java/de/dennisguse/opentracks/content/TrackDataHub.java index 36eb61fad..994b02f57 100644 --- a/src/main/java/de/dennisguse/opentracks/content/TrackDataHub.java +++ b/src/main/java/de/dennisguse/opentracks/content/TrackDataHub.java @@ -34,7 +34,6 @@ import de.dennisguse.opentracks.content.data.Track; import de.dennisguse.opentracks.content.data.TrackPoint; import de.dennisguse.opentracks.content.data.Waypoint; import de.dennisguse.opentracks.content.provider.ContentProviderUtils; -import de.dennisguse.opentracks.content.provider.TrackPointFactory; import de.dennisguse.opentracks.content.provider.TrackPointIterator; import de.dennisguse.opentracks.util.LocationUtils; import de.dennisguse.opentracks.util.PreferencesUtils; @@ -392,7 +391,7 @@ public class TrackDataHub implements DataSourceListener, SharedPreferences.OnSha int samplingFrequency = -1; boolean includeNextPoint = false; - try (TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(selectedTrackId, localLastSeenLocationId + 1, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY)) { + try (TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(selectedTrackId, localLastSeenLocationId + 1, false)) { while (locationIterator.hasNext()) { TrackPoint trackPoint = locationIterator.next(); diff --git a/src/main/java/de/dennisguse/opentracks/content/data/TrackPoint.java b/src/main/java/de/dennisguse/opentracks/content/data/TrackPoint.java index 311509032..96e9f56ee 100644 --- a/src/main/java/de/dennisguse/opentracks/content/data/TrackPoint.java +++ b/src/main/java/de/dennisguse/opentracks/content/data/TrackPoint.java @@ -185,9 +185,4 @@ public class TrackPoint { public float bearingTo(@NonNull Location dest) { return location.bearingTo(dest); } - - public void reset() { - location.reset(); - sensorDataSet = null; - } } diff --git a/src/main/java/de/dennisguse/opentracks/content/provider/ContentProviderUtils.java b/src/main/java/de/dennisguse/opentracks/content/provider/ContentProviderUtils.java index 2a1a8bd82..0653dc57c 100644 --- a/src/main/java/de/dennisguse/opentracks/content/provider/ContentProviderUtils.java +++ b/src/main/java/de/dennisguse/opentracks/content/provider/ContentProviderUtils.java @@ -626,10 +626,9 @@ public class ContentProviderUtils { * * @param cursor the cursor pointing to a trackPoint. * @param indexes the cached track points indexes - * @param trackPoint the track point */ - static void fillTrackPoint(Cursor cursor, CachedTrackPointsIndexes indexes, TrackPoint trackPoint) { - trackPoint.reset(); + static TrackPoint fillTrackPoint(Cursor cursor, CachedTrackPointsIndexes indexes) { + TrackPoint trackPoint = new TrackPoint(); if (!cursor.isNull(indexes.longitudeIndex)) { trackPoint.setLongitude(((double) cursor.getInt(indexes.longitudeIndex)) / 1E6); @@ -658,6 +657,8 @@ public class ContentProviderUtils { float power = cursor.isNull(indexes.sensorPowerIndex) ? SensorDataSet.DATA_UNAVAILABLE : cursor.getFloat(indexes.sensorPowerIndex); trackPoint.setSensorDataSet(new SensorDataSet(heartRate, cadence, power, SensorDataSet.DATA_UNAVAILABLE, trackPoint.getTime())); + + return trackPoint; } /** @@ -750,14 +751,12 @@ public class ContentProviderUtils { } /** - * Creates a location object from a cursor. + * Creates a {@link TrackPoint} object from a cursor. * * @param cursor the cursor pointing to the location */ public TrackPoint createTrackPoint(Cursor cursor) { - TrackPoint location = new TrackPoint(); - fillTrackPoint(cursor, new CachedTrackPointsIndexes(cursor), location); - return location; + return fillTrackPoint(cursor, new CachedTrackPointsIndexes(cursor)); } /** @@ -868,16 +867,15 @@ public class ContentProviderUtils { * Creates a new read-only iterator over a given track's points. * It provides a lightweight way of iterating over long tracks without failing due to the underlying cursor limitations. * Since it's a read-only iterator, {@link Iterator#remove()} always throws {@link UnsupportedOperationException}. - * Each call to {@link TrackPointIterator#next()} may advance to the next DB record, and if so, the iterator calls {@link TrackPointFactory#create()} and populates it with information retrieved from the record. + * Each call to {@link TrackPointIterator#next()} may advance to the next DB record. * When done with iteration, {@link TrackPointIterator#close()} must be called. * * @param trackId the track id * @param startTrackPointId the starting track point id. -1L to ignore * @param descending true to sort the result in descending order (latest location first) - * @param trackPointFactory the location factory */ - public TrackPointIterator getTrackPointLocationIterator(final long trackId, final long startTrackPointId, final boolean descending, final TrackPointFactory trackPointFactory) { - return new TrackPointIterator(this, trackId, startTrackPointId, descending, trackPointFactory); + public TrackPointIterator getTrackPointLocationIterator(final long trackId, final long startTrackPointId, final boolean descending) { + return new TrackPointIterator(this, trackId, startTrackPointId, descending); } private TrackPoint findTrackPointBy(String selection, String[] selectionArgs) { diff --git a/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointFactory.java b/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointFactory.java deleted file mode 100644 index 6798a2444..000000000 --- a/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointFactory.java +++ /dev/null @@ -1,22 +0,0 @@ -package de.dennisguse.opentracks.content.provider; - -import android.location.Location; -import android.location.LocationManager; - -import de.dennisguse.opentracks.content.data.TrackPoint; - -/** - * Creates a new {@link TrackPoint}. - * An implementation can create new instances or reuse existing instances for optimization. - */ -public class TrackPointFactory { - - /** - * The default {@link TrackPointFactory} which creates a location each time. - */ - public static final TrackPointFactory DEFAULT_LOCATION_FACTORY = new TrackPointFactory(); - - public TrackPoint create() { - return new TrackPoint(new Location(LocationManager.GPS_PROVIDER)); - } -} diff --git a/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointIterator.java b/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointIterator.java index 1db6582c8..d944ae6ce 100644 --- a/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointIterator.java +++ b/src/main/java/de/dennisguse/opentracks/content/provider/TrackPointIterator.java @@ -18,21 +18,15 @@ public class TrackPointIterator implements Iterator, AutoCloseable { private final ContentProviderUtils contentProviderUtils; private final long trackId; private final boolean descending; - private final TrackPointFactory trackPointFactory; //TODO Remove; seems to be an old performance optimization. private final CachedTrackPointsIndexes indexes; private long lastTrackPointId = -1L; private Cursor cursor; - public TrackPointIterator(ContentProviderUtils contentProviderUtils, long trackId, long startTrackPointId, boolean descending, TrackPointFactory trackPointFactory) { - if (trackPointFactory == null) { - throw new IllegalArgumentException("trackPointFactory is null"); - } - + public TrackPointIterator(ContentProviderUtils contentProviderUtils, long trackId, long startTrackPointId, boolean descending) { this.contentProviderUtils = contentProviderUtils; this.trackId = trackId; this.descending = descending; - this.trackPointFactory = trackPointFactory; cursor = getCursor(startTrackPointId); indexes = cursor != null ? new CachedTrackPointsIndexes(cursor) @@ -91,9 +85,7 @@ public class TrackPointIterator implements Iterator, AutoCloseable { } } lastTrackPointId = cursor.getLong(indexes.idIndex); - TrackPoint trackPoint = trackPointFactory.create(); - ContentProviderUtils.fillTrackPoint(cursor, indexes, trackPoint); - return trackPoint; + return ContentProviderUtils.fillTrackPoint(cursor, indexes); } @Override diff --git a/src/main/java/de/dennisguse/opentracks/io/file/exporter/FileTrackExporter.java b/src/main/java/de/dennisguse/opentracks/io/file/exporter/FileTrackExporter.java index 54dca39a6..7d744303d 100644 --- a/src/main/java/de/dennisguse/opentracks/io/file/exporter/FileTrackExporter.java +++ b/src/main/java/de/dennisguse/opentracks/io/file/exporter/FileTrackExporter.java @@ -28,7 +28,6 @@ import de.dennisguse.opentracks.content.data.Track; import de.dennisguse.opentracks.content.data.TrackPoint; import de.dennisguse.opentracks.content.data.Waypoint; import de.dennisguse.opentracks.content.provider.ContentProviderUtils; -import de.dennisguse.opentracks.content.provider.TrackPointFactory; import de.dennisguse.opentracks.content.provider.TrackPointIterator; import de.dennisguse.opentracks.util.LocationUtils; @@ -128,7 +127,7 @@ public class FileTrackExporter implements TrackExporter { int locationNumber = 0; TrackPoint lastTrackPoint = null; - try (TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY)) { + try (TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false)) { while (trackPointIterator.hasNext()) { if (Thread.interrupted()) { diff --git a/src/main/java/de/dennisguse/opentracks/io/file/importer/AbstractFileTrackImporter.java b/src/main/java/de/dennisguse/opentracks/io/file/importer/AbstractFileTrackImporter.java index ef6cde14f..45fb4dec0 100644 --- a/src/main/java/de/dennisguse/opentracks/io/file/importer/AbstractFileTrackImporter.java +++ b/src/main/java/de/dennisguse/opentracks/io/file/importer/AbstractFileTrackImporter.java @@ -40,7 +40,6 @@ import de.dennisguse.opentracks.content.data.Track; import de.dennisguse.opentracks.content.data.TrackPoint; import de.dennisguse.opentracks.content.data.Waypoint; import de.dennisguse.opentracks.content.provider.ContentProviderUtils; -import de.dennisguse.opentracks.content.provider.TrackPointFactory; import de.dennisguse.opentracks.content.provider.TrackPointIterator; import de.dennisguse.opentracks.stats.TripStatisticsUpdater; import de.dennisguse.opentracks.util.FileUtils; @@ -161,7 +160,7 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track @Deprecated // TODO Should not be necessary anymore? TripStatisticsUpdater markerTripStatisticsUpdater = new TripStatisticsUpdater(track.getTripStatistics().getStartTime()); - try (TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY)) { + try (TrackPointIterator trackPointIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false)) { while (true) { if (waypoint == null) { diff --git a/src/main/java/de/dennisguse/opentracks/services/TrackRecordingService.java b/src/main/java/de/dennisguse/opentracks/services/TrackRecordingService.java index cdcbee0af..5532a2e39 100644 --- a/src/main/java/de/dennisguse/opentracks/services/TrackRecordingService.java +++ b/src/main/java/de/dennisguse/opentracks/services/TrackRecordingService.java @@ -46,7 +46,6 @@ import de.dennisguse.opentracks.content.data.TrackPoint; import de.dennisguse.opentracks.content.data.Waypoint; import de.dennisguse.opentracks.content.provider.ContentProviderUtils; import de.dennisguse.opentracks.content.provider.CustomContentProvider; -import de.dennisguse.opentracks.content.provider.TrackPointFactory; import de.dennisguse.opentracks.content.provider.TrackPointIterator; import de.dennisguse.opentracks.content.sensor.SensorDataSet; import de.dennisguse.opentracks.services.sensors.BluetoothRemoteSensorManager; @@ -378,7 +377,7 @@ public class TrackRecordingService extends Service { TripStatistics tripStatistics = track.getTripStatistics(); trackTripStatisticsUpdater = new TripStatisticsUpdater(tripStatistics.getStartTime()); - try (TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false, TrackPointFactory.DEFAULT_LOCATION_FACTORY)) { + try (TrackPointIterator locationIterator = contentProviderUtils.getTrackPointLocationIterator(track.getId(), -1L, false)) { trackTripStatisticsUpdater.addTrackPoint(locationIterator, recordingDistanceInterval); } catch (RuntimeException e) { Log.e(TAG, "RuntimeException", e);