Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display distance, average and maximum speed #58

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Display distance, average and maximum speed #58

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b90b29a
Display distance, average and maximum speed
Jan 5, 2016
0b3ad70
Reorganized distance calculations
Jan 11, 2016
1314844
Sharing track statistics data between TrackManager and TrackDetails
Feb 3, 2016
6c9ed79
cleaned up useless imports
Feb 3, 2016
d5958d8
Code improvement/cleanup
Feb 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/src/main/java/me/guillaumin/android/osmtracker/activity/TrackDetail.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@

import me.guillaumin.android.osmtracker.OSMTracker;
import me.guillaumin.android.osmtracker.R;
import me.guillaumin.android.osmtracker.db.DataHelper;
import me.guillaumin.android.osmtracker.db.TrackContentProvider;
import me.guillaumin.android.osmtracker.db.TrackContentProvider.Schema;
import me.guillaumin.android.osmtracker.db.TracklistAdapter;
import me.guillaumin.android.osmtracker.db.model.Track;
import me.guillaumin.android.osmtracker.db.model.TrackStatistics;
import me.guillaumin.android.osmtracker.gpx.ExportToStorageTask;
import me.guillaumin.android.osmtracker.util.MercatorProjection;
import android.content.ContentResolver;
Expand Down Expand Up @@ -125,6 +128,7 @@ protected void onResume() {
Track t = Track.build(trackId, cursor, cr, true);

bindTrack(t);
TrackStatistics stat = DataHelper.getTrackStatistics(trackId, cr);

String from[] = new String[]{ITEM_KEY, ITEM_VALUE};
int[] to = new int[] {R.id.trackdetail_item_key, R.id.trackdetail_item_value};
Expand All @@ -144,6 +148,21 @@ protected void onResume() {
map.put(ITEM_VALUE, Integer.toString(t.getTpCount()));
data.add(map);

// Distance
map = new HashMap<String, String>();
map.put(ITEM_KEY, getResources().getString(R.string.trackmgr_distance));
map.put(ITEM_VALUE, TracklistAdapter.distanceToString(stat.totalLength(), getResources()));
data.add(map);

// Speed
map = new HashMap<String, String>();
map.put(ITEM_KEY, getResources().getString(R.string.trackdetail_speed));
map.put(ITEM_VALUE, TracklistAdapter.speedToString(stat.averageSpeed(), getResources()) + " " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those it would be preferable to use a placeholder as we do here because in some languages that may not be expressed as "12 km" but perhaps "km 12"...

getResources().getString(R.string.trackdetail_speed_average) + ", " +
TracklistAdapter.speedToString(stat.maximumSpeed(), getResources()) + " " +
getResources().getString(R.string.trackdetail_speed_max));
data.add(map);

// Start date
map = new HashMap<String, String>();
map.put(ITEM_KEY, getResources().getString(R.string.trackdetail_startdate));
Expand Down
3 changes: 3 additions & 0 deletions app/src/main/java/me/guillaumin/android/osmtracker/activity/TrackManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,9 @@ private void deleteTrack(long id) {
if (trackStorageDirectory.exists()) {
FileSystemUtils.delete(trackStorageDirectory, true);
}

// Delete the statistics
DataHelper.removeTrackStatistics(id);
}

/**
Expand Down
23 changes: 23 additions & 0 deletions app/src/main/java/me/guillaumin/android/osmtracker/db/DataHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

import java.io.File;
import java.text.SimpleDateFormat;
import java.util.TreeMap;
import java.lang.RuntimeException;

import me.guillaumin.android.osmtracker.OSMTracker;
import me.guillaumin.android.osmtracker.db.TrackContentProvider.Schema;
import me.guillaumin.android.osmtracker.db.model.TrackStatistics;
import android.content.ContentResolver;
import android.content.ContentUris;
import android.content.ContentValues;
Expand Down Expand Up @@ -65,6 +68,9 @@ public class DataHelper {
*/
private Context context;

/** Statistics for all existing tracks */
private static TreeMap<Long, TrackStatistics> tracksStatistics = new TreeMap<Long, TrackStatistics> ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you need to store something here? This class is supposed to be a stateless utility class so that doesn't sound right. Is that for caching purposes? In which cas that should be a field stored on the class that need this data, not this utility class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to store it because creating a new instance of TrackStatistics iterates throigh all points in the track, an I don't want that to happen every second when the active track is updated. Thing is, four different classes (including this one) need to share this data, so if it should be a field of a class, I have to ask: which one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see it only in 2 classes (Assuming the DataHelper one is removed)?

I would create a field in TrackDetail which I think makes sense because the TrackDetail activity is about a specific track, so having a field holding its data is reasonable and useful as it needs to be updated. You can create it in onStart() and then refer to it / update it in the various methods like onResume().

In TracklistAdapter it's a bit tricky since there are the need for one of them for each track. I guess you could use the Map idea here perhaps...

The problem with storing it in DataHelper is that it's not the idiomatic way to share data between activities and views in Android and that's likely to cause issues and bug difficult to track down from past experience. You'll also need to deal with events like tracks being deleted (removing items from the map). It also complicates maintenance because if we have something else updating the DB not going through DataHelper.track() then the track statistics won't be updated

It would be preferable for the data to be local to the component that needs it (Activity, View) and to tie it to the lifecycle of this component (the TrackStatstics object gets garbage collected when the view is removed)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fourth one is TrackManager, removeTrackStatistics in deleteTrack. I saw that track id's aren't reused but didn't want my code to rely on that.

Should it be a field of TracklistAdapter? If TracklistAdapter is to update its own statistics inside bind(...), wouldn't I have to write some additional complicated code to figure out which trackpoints aren't a part of the statistics yet? Also TracklistAdapter is recreated in TrackManager's onResume(), which means all statistics will be recalculated from scratch every time that thing is called, wouldn't it be a problem?

Also TrackDetail, you mean there should be two copies of the same data (and the same code), one in TracklistAdapter and another one in TrackDetail?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't I have to write some additional complicated code to figure out which trackpoints aren't a part of the statistics yet?

No I wouldn't, just add a WHERE clause to the database query, so that's out of the way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's also the performance aspect to consider as you said initially. If it turns out computing these values is too slow any way on big tracks we'll have to store it in the DB and that'll change the approach completely. Perhaps it's worth trying to run it with a big track with thousands of track points and see how the list view performs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10k points will take half a second second on 1.3 GHz CPU (it's quad core but I guess that's irrelevant?). In other words, 10 hours worth of recording takes 2 seconds. Which, to be honest, feels 100 times slower than what should be possible with C code and a simple data storage, but that aside – it's good enough for my purposes, and what do you say? Is it worth changing the database?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not if that's this fast.

One thing to consider to avoid storing state in the DataHelper class are ContentObservers. They allow you to register a function to be called when something changes in the DB, so perhaps doing that in TrackListAdapter would allow you to update the values as they change?


/**
* ContentResolver to interact with content provider
*/
Expand Down Expand Up @@ -125,6 +131,7 @@ public void track(long trackId, Location location, float azimuth, int accuracy)
}

Uri trackUri = ContentUris.withAppendedId(TrackContentProvider.CONTENT_URI_TRACK, trackId);
getTrackStatistics(trackId, contentResolver).update(location);
contentResolver.insert(Uri.withAppendedPath(trackUri, Schema.TBL_TRACKPOINT + "s"), values);
}

Expand Down Expand Up @@ -373,4 +380,20 @@ public static File getTrackDirectory(long trackId) {
return _return;
}

/**
* Get statistics for a given track
* If doesn't exists, create and calculate from the database
*/
public static TrackStatistics getTrackStatistics(long trackId, ContentResolver resolver) {
if (! tracksStatistics.containsKey(trackId))
tracksStatistics.put(trackId, new TrackStatistics(trackId, resolver)); // getContentResolver() if non-static member
return tracksStatistics.get(trackId);
}

/**
* Remove statistics for a given track
*/
public static void removeTrackStatistics(long trackId) {
tracksStatistics.remove(trackId);
}
}
35 changes: 35 additions & 0 deletions app/src/main/java/me/guillaumin/android/osmtracker/db/TracklistAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import me.guillaumin.android.osmtracker.R;
import me.guillaumin.android.osmtracker.db.TrackContentProvider.Schema;
import me.guillaumin.android.osmtracker.db.model.Track;
import me.guillaumin.android.osmtracker.db.model.TrackStatistics;
import android.content.Context;
import android.content.res.Resources;
import android.database.Cursor;
import android.view.LayoutInflater;
import android.view.View;
Expand All @@ -21,6 +23,34 @@
*/
public class TracklistAdapter extends CursorAdapter {

public static String distanceToString(float distance, Resources resources) {
if (distance < 100)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use brackets on the IF statements to be consistent with the rest of the code

return String.format("%d %s", Math.round(distance),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to use placeholders for those as well

resources.getString(R.string.trackmgr_distance_meters));
else if (distance < 1000)
return String.format("%d %s", 10*Math.round(distance/10),
resources.getString(R.string.trackmgr_distance_meters));
else if (distance < 10000)
return String.format("%.1f %s", distance/1000,
resources.getString(R.string.trackmgr_distance_kilometers));
else
return String.format("%d %s", Math.round(distance/1000),
resources.getString(R.string.trackmgr_distance_kilometers));
}

public static String speedToString(float speed, Resources resources) {
float kmph = (float)3.6*speed;
if (kmph < 1)
return String.format("%.1g %s", kmph,
resources.getString(R.string.trackmgr_speed_kmph));
else if (kmph < 20)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some values (20, 10, 100, 1000 above) could be put in constants

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is math, so not necessarily. Like for example, in that code copy-pasted from osmand, number of degrees in a flat angle (180) is used directly rather than as a named constant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to disagree, math or not math the only values that don't deserve a constant are usually zero or 1. I think at least "3.6" and "20" deserve constants. In any case the existing code uses constant so being consistent is good.

If OSMAnd get included as a library the point is moot.

return String.format("%.1f %s", kmph,
resources.getString(R.string.trackmgr_speed_kmph));
else
return String.format("%d %s", Math.round(kmph),
resources.getString(R.string.trackmgr_speed_kmph));
}

public TracklistAdapter(Context context, Cursor c) {
super(context, c);
}
Expand Down Expand Up @@ -53,6 +83,8 @@ private View bind(Cursor cursor, View v, Context context) {
TextView vNameOrStartDate = (TextView) v.findViewById(R.id.trackmgr_item_nameordate);
TextView vWps = (TextView) v.findViewById(R.id.trackmgr_item_wps);
TextView vTps = (TextView) v.findViewById(R.id.trackmgr_item_tps);
TextView vDistance = (TextView) v.findViewById(R.id.trackmgr_item_distance);
TextView vSpeed = (TextView) v.findViewById(R.id.trackmgr_item_speed);
ImageView vStatus = (ImageView) v.findViewById(R.id.trackmgr_item_statusicon);
ImageView vUploadStatus = (ImageView) v.findViewById(R.id.trackmgr_item_upload_statusicon);

Expand Down Expand Up @@ -83,8 +115,11 @@ private View bind(Cursor cursor, View v, Context context) {

// Bind WP count, TP count, name
Track t = Track.build(trackId, cursor, context.getContentResolver(), false);
TrackStatistics stat = DataHelper.getTrackStatistics(trackId, context.getContentResolver());
vTps.setText(Integer.toString(t.getTpCount()));
vWps.setText(Integer.toString(t.getWpCount()));
vDistance.setText(distanceToString(stat.totalLength(), context.getResources()));
vSpeed.setText(speedToString(stat.averageSpeed(), context.getResources()));
vNameOrStartDate.setText(t.getName());

return v;
Expand Down
Loading