Skip to content

Route progress classes should be codable #1940

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

Closed
1ec5 opened this issue Jan 22, 2019 · 6 comments · Fixed by #2615
Closed

Route progress classes should be codable #1940

1ec5 opened this issue Jan 22, 2019 · 6 comments · Fixed by #2615
Assignees
Labels
op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 22, 2019

RouteProgress, RouteLegProgress, and RouteStepProgress should conform to NSSecureCoding or Codable. MapboxDirections.swift classes such as Route are already NSSecureCoding-conformant.

/cc @mapbox/navigation-ios

@1ec5 1ec5 added the op-ex Refactoring, Tech Debt or any other operational excellence work. label Jan 22, 2019
@1ec5 1ec5 mentioned this issue Jan 6, 2020
10 tasks
@1ec5 1ec5 added this to the v1.0.0 milestone Jan 16, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Jan 16, 2020

Once we move the progress classes off NSObject (#2294), it’ll be straightforward to implement Codable conformance in these classes. This will make it much easier for developers to implement state restoration.

@Udumft Udumft self-assigned this Mar 25, 2020
@Udumft
Copy link
Contributor

Udumft commented Mar 25, 2020

RouteLegProgress already conforms to Encodable here:

extension RouteLegProgress: Encodable {
private enum CodingKeys: String, CodingKey {
case upcomingInstruction
case upcomingType
case upcomingModifier
case upcomingName
case previousInstruction
case previousType
case previousModifier
case previousName
case distance
case duration
case distanceRemaining
case durationRemaining
}
public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)
try container.encodeIfPresent(upcomingStep?.instructions, forKey: .upcomingInstruction)
try container.encodeIfPresent(upcomingStep?.maneuverType, forKey: .upcomingType)
try container.encodeIfPresent(upcomingStep?.maneuverDirection, forKey: .upcomingModifier)
try container.encodeIfPresent(upcomingStep?.names?.joined(separator: ";"), forKey: .upcomingName)
try container.encodeIfPresent(currentStep.instructions, forKey: .previousInstruction)
try container.encode(currentStep.maneuverType, forKey: .previousType)
try container.encode(currentStep.maneuverDirection, forKey: .previousModifier)
try container.encode(currentStep.names?.joined(separator: ";"), forKey: .previousName)
try container.encode(Int(currentStep.distance), forKey: .distance)
try container.encode(Int(currentStep.expectedTravelTime), forKey: .duration)
try container.encode(Int(currentStepProgress.distanceRemaining), forKey: .distanceRemaining)
try container.encode(Int(currentStepProgress.durationRemaining), forKey: .durationRemaining)
}
}

But this implementation does not seem to be exactly what is expected, because it does not directly encodes it's variables and instead encodes sub-sub properties. I thought that it is intended to be used for passing as context info for some Events, but I could not find any real usage.
What is this implementation for?

@JThramer
Copy link
Contributor

JThramer commented Apr 1, 2020

Blocked by #2352

@JThramer
Copy link
Contributor

JThramer commented Apr 1, 2020

Posterity: Regarding this comment, as long as route-step is encodable, RouteLegProgress should just encode RouteStep directly. Needs investigation.

@IodaMikeMapbox IodaMikeMapbox self-assigned this Jul 31, 2020
@zugaldia zugaldia modified the milestones: v1.0.0, v1.1.0 Sep 8, 2020
@zugaldia
Copy link
Member

zugaldia commented Sep 8, 2020

Closing per sprint planning convo.

@zugaldia zugaldia closed this as completed Sep 8, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Sep 8, 2020

Sorry, I got some wires crossed in sprint planning. This change is still relevant but doesn’t need to happen for v1.0.0.

But this implementation does not seem to be exactly what is expected, because it does not directly encodes it's variables and instead encodes sub-sub properties. I thought that it is intended to be used for passing as context info for some Events, but I could not find any real usage.
What is this implementation for?

Looks like this implementation was introduced in 66cb213 for #1533 but ended up being unused after 982a94c in the same PR. We can safely remove that implementation and replace it with a more standard one that directly encodes its members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants