Add junction views marshaling support. #425

fabian-guerra · 2020-06-03T22:01:08Z

Adds marshaling support for guidance views. This is a port from #405 adds marshaling test and fixes a parsing issue.

1ec5

Apart from the feedback below, also add @avi-c as a coauthor for the commit, since it’s heavily based on the earlier commits in #405.

1ec5 · 2020-06-09T15:26:14Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -34,6 +34,8 @@ public extension VisualInstruction {
         */
        case image(image: ImageRepresentation, alternativeText: TextRepresentation)

+        case guidanceView(image: GuidanceViewImageRepresentation, alternativeText: TextRepresentation)


Add documentation about this case; otherwise it won’t show up in documentation.

1ec5 · 2020-06-09T15:27:59Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -149,13 +151,29 @@ public extension VisualInstruction.Component {
    }
 }

+public struct GuidanceViewImageRepresentation: Equatable {


ImageRepresentation isn’t meant to be limited to the image case; it just means that it contains the details for rendering an image. So does this struct, but the difference is in how the URL is interpreted. I wonder if we should try to use ImageRepresentation for both cases but genericize it to make sense in both. If we must have a separate struct, we should probably rename the two structs to better reflect the distinction. Perhaps one would be ScalableImageRepresentation or something to that effect. Needs more thought.

/ref #405 (comment)

1ec5

In addition to the feedback below, please describe the changes in CHANGELOG.md, for example:

Added the VisualInstructionBanner.quaternaryInstruction property and VisualInstruction.Component.guidanceView(image:alternativeText:) enumeration case to represent a detailed image of an upcoming junction. (#425)
Renamed the VisualInstruction.Component.ImageRepresentation(imageBaseURL:) initializer to VisualInstruction.Component.ImageRepresentation(imageBaseURL:availableScales:availableFormats:) and added the VisualInstruction.Component.ImageRepresentation.availableScales and VisualInstruction.Component.ImageRepresentation.availableFormats properties. (#425)

1ec5 · 2020-06-09T22:54:43Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -35,7 +35,12 @@ public extension VisualInstruction {
        case image(image: ImageRepresentation, alternativeText: TextRepresentation)

        /**
-         The compoment contains the localized word for “Exit”.


Thanks for fixing this typo. 😄

1ec5 · 2020-06-09T22:59:36Z

Sources/MapboxDirections/VisualInstructionComponent.swift

+        ///   - imageBaseURL: The image's base URL.
+        ///   - availableScales: The image’s available scale factors. If this argument is unspecified, the current screen’s native scale factor is used.
+        ///   - availableFormats: The available file formats of the image.
+        public init(imageBaseURL: URL?, availableScales: Set<CGFloat>? = nil, availableFormats: Set<Format>) {


availableScales doesn’t have to have a default value, since it’s easy enough to pass in nil in the couple places that would need to have it be nil. (It would be necessary to define a default value if we were concerned about backwards-incompatibility, but in that case, availableFormats would also have a default value.)

1ec5 · 2020-06-09T23:01:49Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -195,14 +235,22 @@ extension VisualInstruction.Component: Codable {
            if let imageBaseURLString = try container.decodeIfPresent(String.self, forKey: .imageBaseURL) {
                imageBaseURL = URL(string: imageBaseURLString)
            }
-            let imageRepresentation = ImageRepresentation(imageBaseURL: imageBaseURL)
+            let imageRepresentation = ImageRepresentation(imageBaseURL: imageBaseURL, availableFormats: [.png, .svg])


This representation is available at 1×, 2×, and 3× scales.

1ec5 · 2020-06-09T23:04:08Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -124,11 +159,14 @@ public extension VisualInstruction.Component {
         - returns: A remote URL to the image.
         */
        public func imageURL(scale: CGFloat? = nil, format: Format = .png) -> URL? {


Update the scale documentation to replace:

Only the values 1, 2, and 3 are currently supported.

with:

Only the values in the availableScales property are currently supported.

1ec5 · 2020-06-09T23:04:54Z

Sources/MapboxDirections/VisualInstructionComponent.swift

            guard let imageBaseURL = imageBaseURL,
+                self.availableScales.contains(imageScale),
+                self.availableFormats.contains(format),


Nit: it’s OK to omit self. here, but not a big deal.

1ec5 · 2020-06-09T23:08:30Z

Sources/MapboxDirections/VisualInstructionComponent.swift

+            let defaultScales: Set<CGFloat> = [ImageRepresentation.currentScale]
+            self.availableScales = availableScales ?? defaultScales


Think of availableScales as the set of scales that can be explicitly part of the URL, rather than the scale that the image happens to be in. So the default here should be an empty set.

1ec5 · 2020-06-09T23:09:37Z

Sources/MapboxDirections/VisualInstructionComponent.swift

+        /// Initializes an image representation bearing the image at the given URL.
+        /// - Parameters:
+        ///   - imageURL: The image URL.
+        ///   - availableScales: The image’s available scale factors. If this argument is unspecified, the current screen’s native scale factor is used.


If this argument is unspecified, the current screen’s native scale factor is used.

This is true of imageURL(scale:format:), but I think it would be better for a nil availableScales to mean “don’t append a scale to the URL”.

1ec5 · 2020-06-09T23:11:01Z

Sources/MapboxDirections/VisualInstructionComponent.swift

                var imageURLComponents = URLComponents(url: imageBaseURL, resolvingAgainstBaseURL: false) else {
                return nil
            }
-            imageURLComponents.path += "@\(Int(scale ?? ImageRepresentation.currentScale))x.\(format)"
+            imageURLComponents.path += "@\(Int(imageScale))x.\(format)"


Does the Guidance Views API put @1x etc. and a file extension in the URL? If not, let’s use an empty availableScales or availableFormats set to detect that these things should not be added to the path.

It doesn't its a direct URL that's the reason that I consider if you initialized an image with an imageURL you can use the property instead

This line is unconditionally adding e.g. @1x.png to the URL. We should either avoid adding it to the URL or remove it from the URL before initializing the component. Let’s add an assertion to VisualInstructionsTests to verify that the caller of this method will get the same URL that the Directions API returned for a guidance view image.

1ec5 · 2020-06-09T23:14:36Z

Sources/MapboxDirections/VisualInstructionComponent.swift

+        /// The image's available scales.
+        public let availableScales: Set<CGFloat>
+
+        /// The image's available formats.
+        public let availableFormats: Set<Format>


(Note to self: These properties aren’t being encoded, decoded, and compared explicitly because they’re derived from the component type, which is already being encoded, decoded, and compared.)

1ec5 · 2020-06-09T23:21:49Z

https://app.circleci.com/pipelines/github/mapbox/mapbox-directions-swift/585/workflows/b57babdc-132a-4c5e-ac62-69117412d8b9/jobs/1725/steps

Test Case '-[MapboxDirectionsTests.VisualInstructionComponentTests testImageComponent]' started.

/Users/distiller/project/Tests/MapboxDirectionsTests/VisualInstructionComponentTests.swift:39: error: -[MapboxDirectionsTests.VisualInstructionComponentTests testImageComponent] : XCTAssertEqual failed: ("nil") is not equal to ("Optional("https://s3.amazonaws.com/mapbox/shields/v3/us-42@1x.svg")")

Test Case '-[MapboxDirectionsTests.VisualInstructionComponentTests testImageComponent]' failed (0.008 seconds).

Co-authored-by: Avi Cieplinski <avi@mapbox.com>

1ec5

Just a documentation comment to add, but otherwise good to go. Thanks!

1ec5 · 2020-06-10T18:45:34Z

Sources/MapboxDirections/VisualInstructionComponent.swift

@@ -149,13 +154,29 @@ public extension VisualInstruction.Component {
    }
 }

+public struct GuidanceViewImageRepresentation: Equatable {


GuidanceViewImageRepresentation still needs to be documented. Otherwise it won’t show up in the docset.

fabian-guerra self-assigned this Jun 3, 2020

fabian-guerra force-pushed the fabian-junction-views branch from 07c6a54 to 9125c6c Compare June 5, 2020 22:18

fabian-guerra added the improvement Improvement for an existing feature. label Jun 5, 2020

fabian-guerra requested a review from 1ec5 June 5, 2020 22:21

fabian-guerra marked this pull request as ready for review June 5, 2020 22:21

fabian-guerra changed the title ~~Fabian junction views~~ Add junction views marshaling support. Jun 5, 2020

fabian-guerra requested a review from avi-c June 8, 2020 16:14

1ec5 suggested changes Jun 9, 2020

View reviewed changes

1ec5 added this to the v1.0.0 milestone Jun 9, 2020

fabian-guerra force-pushed the fabian-junction-views branch from 9125c6c to 5215e84 Compare June 9, 2020 15:55

mapbox deleted a comment from 1ec5 Jun 9, 2020

1ec5 reviewed Jun 9, 2020

View reviewed changes

fabian-guerra and others added 3 commits June 10, 2020 12:54

Add guidance view marshalling support.

d6fccb7

Co-authored-by: Avi Cieplinski <avi@mapbox.com>

Add guidance view documentation.

095688d

Update changelog.

d2d66de

fabian-guerra force-pushed the fabian-junction-views branch from bb3cd33 to d2d66de Compare June 10, 2020 18:26

1ec5 approved these changes Jun 10, 2020

View reviewed changes

Update class doc

54d5874

fabian-guerra merged commit c228ecf into master Jun 10, 2020

fabian-guerra deleted the fabian-junction-views branch June 10, 2020 22:23

Udumft mentioned this pull request Jun 12, 2020

Update Turf v0.5.0 mapbox/mapbox-navigation-ios#2393

Merged

2 tasks

1ec5 modified the milestones: v1.0.0, v0.32.0 Jun 12, 2020

1ec5 mentioned this pull request Jun 16, 2020

Draft PR for adding guidance view support to data model #405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add junction views marshaling support. #425

Add junction views marshaling support. #425

fabian-guerra commented Jun 3, 2020 •

edited

Loading

1ec5 left a comment

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 left a comment

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

1ec5 Jun 9, 2020

fabian-guerra Jun 9, 2020

1ec5 Jun 9, 2020 •

edited

Loading

1ec5 Jun 9, 2020 •

edited

Loading

1ec5 commented Jun 9, 2020

1ec5 left a comment

1ec5 Jun 10, 2020

		let defaultScales: Set<CGFloat> = [ImageRepresentation.currentScale]
		self.availableScales = availableScales ?? defaultScales

Add junction views marshaling support. #425

Add junction views marshaling support. #425

Conversation

fabian-guerra commented Jun 3, 2020 • edited Loading

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

1ec5 Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Jun 9, 2020

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra commented Jun 3, 2020 •

edited

Loading

1ec5 Jun 9, 2020 •

edited

Loading

1ec5 Jun 9, 2020 •

edited

Loading