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.

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

Conversation

ars3niy
Copy link

@ars3niy ars3niy commented Jan 5, 2016

While I'm working on the other patch, please have a look at this one. Also, note that this is the second time I ever encountered Java in any way (the previous patch being the first one) so I could have done something really stupid, watch out :)

Incidentally, is there a way to test GPS tracking that doesn't involve getting my arse on a tram? Running around my apartment doesn't help, and it's cold outside.

@ars3niy
Copy link
Author

ars3niy commented Jan 5, 2016

I didn't really want to use global variables but I couldn't find a place to put a non-static member that would be accessible from TrackManager, TracklistAdapter, TrackDetail and DataHelper. Is there a better way?

@nguillaumin
Copy link
Contributor

Thanks, I'll have a look tomorrow it's bed time for me ;)

For the GPS the emulator has controls to simulate GPS events either via the command line or DDMS. It's a bit rudimentary but it beats walking in the snow.

// 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"...

@nguillaumin
Copy link
Contributor

That doesn't look too bad for a first shot at Java ;) The most problematic parts are:

  • Storing of data inside the DataHelper class. I don't think that's necessary because when you need the statistics of a track you're always in a context of a single track, so you can just build a new TrackStatsitcs object for you track and use it
  • Copying of OSMAnd code, I'd prefer if we just their library directly unless there's a good reason not to.

@ars3niy
Copy link
Author

ars3niy commented Jan 6, 2016

Thanks for replies! The rest was quite obvious.
If you'd like mo to rely upon osmand, can you please tell me step by step how to do this? Because I have no idea :)

Updating details for the active track
@ars3niy
Copy link
Author

ars3niy commented Jan 11, 2016

Here's a new version. I don't really like that TrackManager and TrackDetail don't share TrackStatistics, it causes an unnecessary delay upon opening details for a long track. Is there a way for TrackDetail to have a reference to TrackManager who created it? This way, I could use TrackManager's getTrackStatistics() similar to what I now do in TracklistAdapter. TrackDetail appears to be created indirectly through Intent so I don't know how to pass a parameter to it.

@ars3niy
Copy link
Author

ars3niy commented Feb 3, 2016

Hi Nicolas, how're you doin'? :)

I've found a way to share data between TrackManager and TrackDetail spawned by it. Actually it's the same way you pass the track ID to TrackDetail, I just pass total distance, total time, etc. along with it. With that, I'm totally satisfied with everything, but are you satisfied with the code?

@nguillaumin
Copy link
Contributor

Sorry I'm lacking the time to review the code, I'll try to get back to it as soon as I can.


public TracklistAdapter(Context context, Cursor c) {
super(context, c);
owner = (TrackManager) context;
Copy link
Contributor

Choose a reason for hiding this comment

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

That adds a dependency against the container of the view, meaning that we can't use TracklistAdapter from another activity. It would be nice to avoid

@nguillaumin
Copy link
Contributor

I think that looks pretty good, it's just the dependency between TracklistAdapter and TrackManager that concerns me a bit. Since it's all cast at run-time, it would be easy to miss when calling TracklistAdapter from somewhere else.

Perhaps the map of statistics could be passed in the constructor of TracklistAdapter instead, when we create it there ? It has the benefit of making the dependency explicity: "If you want to use TrackListAdapter, you have to provide a map of track statistics".

@ars3niy
Copy link
Author

ars3niy commented Feb 12, 2016

Perhaps the map of statistics could be passed in the constructor of TracklistAdapter instead, when we create it there ?

Yep, only I encapsulated the map into a new class to avoid duplicating the code of what is now TrackStatisticsCollection.get

@ars3niy
Copy link
Author

ars3niy commented Mar 5, 2016

C'mon you can do this, it's so close! Let's do this in the name of the free software movement!

@thePanz
Copy link

thePanz commented Oct 15, 2016

This PR could be the reason to close #38 ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants