Skip to content

Comments

Update ETA info when stale#595

Merged
bsudekum merged 5 commits intomasterfrom
update-eta-time
Sep 12, 2017
Merged

Update ETA info when stale#595
bsudekum merged 5 commits intomasterfrom
update-eta-time

Conversation

@bsudekum
Copy link
Contributor

@bsudekum bsudekum commented Sep 8, 2017

Closes: #594

When the user is not moving, we should still be able to give them accurate info about their trip.

This also removes a few functions that existed just to call another function.

/cc @1ec5 @frederoni @ericrwolfe

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like the name of a property that returns a value without side-effects, which isn’t the case. How about updateETA()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why “info”? updateETATimer is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be an optional instead of an implicitly unwrapped optional? That way the code is explicit about the case where it isn’t set (before viewWillAppear(_:)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we still need to refresh this after a reroute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

@bsudekum
Copy link
Contributor Author

bsudekum commented Sep 8, 2017

@1ec5 @ericrwolfe fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This timer will leak the whole class unless we invalidate it in deinit or viewDidUnload().

@bsudekum
Copy link
Contributor Author

Fixed

super.viewWillDisappear(animated)

UIApplication.shared.isIdleTimerDisabled = false
updateETATimer = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

invalidate() the timer before nilling it out.

@ericrwolfe ericrwolfe modified the milestone: v0.8.0-2 Sep 11, 2017
@bsudekum
Copy link
Contributor Author

@1ec5 fixed.

@bsudekum bsudekum merged commit 364ab38 into master Sep 12, 2017
@bsudekum bsudekum deleted the update-eta-time branch September 12, 2017 19:33
@1ec5 1ec5 mentioned this pull request Jan 25, 2019
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