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

Basic Speed support #23

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

Conversation

yoiang
Copy link
Contributor

@yoiang yoiang commented Sep 25, 2018

GPX 1.1 no longer provides a speed field, when it is not provided calculate speed manually, simple point to point calculation.

I haven't applied the linting and organization changes found in #22 because I assume we're going to merge that PR first. If not I can bring them to this change as well!

@vermont42
Copy link
Owner

@yoiang Are you still looking to get this merged? If so, please resolve the conflicts, which are fortunately not in a .pbxproj or .storyboard file. 😅 On a somewhat-related note, I'm curious how you prefer to resolve conflicts. I like Atom's conflict-resolution plugin.

I see that speed has not been in the GPX standard since 2004, so your PR seems appropriate. Do you agree, @nkanetka?

@yoiang
Copy link
Contributor Author

yoiang commented Nov 8, 2018

Sounds good, I'll fix the conflicts!

@yoiang
Copy link
Contributor Author

yoiang commented Nov 15, 2018

Should be good to merge!

@vermont42
Copy link
Owner

Thanks. I will review it this weekend.

@vermont42
Copy link
Owner

vermont42 commented Nov 17, 2018

@yoiang It's getting close. Some comments.

I'm not sure the speeds being displayed in meters per second are correct. This was a run I recorded, and I doubt I ever reached a speed of, for example, 262.8 m/s. That number was reported on 10X mode. I slowed down to 1X, and speeds were lower, for example 25 m/s, but that's still too high for a run. Speeds should be closer to 2.6 m/s, which is ten minutes per mile, likely the pace I averaged on this extremely hilly run.

I am not a designer, but the layouts of the new labels could use some love. I would align the trailing edges of actualSpeedLabelLabel and headingLabelLabel to centerX and the leading edges of actualSpeedLabel and headingLabel to centerX. I would put a 16-point cushion between the bottom of headingLabelLabel and the bottom safeAreaLayoutGuide, as is customary on iOS. One goal of the sample app is to make the best impression possible on potential users. (I recognize that heading is from the previous PR, but heading spacing could be improved in this one.)

simulator screen shot - iphone se - 2018-11-17 at 13 28 27

When left running long enough, the demo repeatably crashes on this line in GpxLocationManager.swift. I see that this crash also occurs on master, so I won't ask you to fix it. Instead, I'll take a look at it during my upcoming paternity leave.

screen shot 2018-11-17 at 1 39 48 pm

@yoiang
Copy link
Contributor Author

yoiang commented Dec 22, 2018

@vermont42 262.80 m/s is your speed calculated at 10X. If you want the speed at 1X you can switch down.

The layout is built on top of what was there already, feel free to change it however you'd like.

The crash is the topic of discussion in #22 (comment) , #26, #27, should the fix be included here?

@nkanetka
Copy link
Collaborator

@vermont42 Let's try to get PR #27 merged in so that we can update this PR with safe array accessing.

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