Visual Instruction Presentation Customization #1530

JThramer · 2018-06-29T20:33:46Z

Implements #1503.

Adding delegate method to customize attributed string that is computed by InstructionPresenter.

To-do:

Add tests
Add example

/cc @mapbox/navigation-ios

…d by `InstructionPresenter`.

bsudekum · 2018-06-29T20:44:55Z

MapboxNavigation/NavigationViewController.swift

@@ -538,6 +538,10 @@ extension NavigationViewController: RouteMapViewControllerDelegate {
        }
        return roadName
    }
+
+    @objc public func label(_ label: InstructionLabel, willPresent instruction: VisualInstruction, as presented: NSAttributedString) -> NSAttributedString? {


Curious, why does NavigationViewController need to implement the delegate method?

This is just plumbing the delegate method through the view-controller hierarchy, just like we do here for a NavigationMapViewDelegate method or here for the same method (label(_:willPresent:as:)), being routed through RouteMapViewController.

It's not really accurate to say that the NavigationViewController is implementing the delegate method so much as it's merely passing the message onto its delegate.

JThramer · 2018-06-29T23:00:23Z

Demonstrative screenshot, where in the delegate method, I implemented logic to reverse the presented strings:

1ec5 · 2018-07-02T18:18:20Z

MapboxNavigation/InstructionLabel.swift

+     - parameter presented: the formatted string that is provided by the instruction presenter
+     - returns: optionally, a customized NSAttributedString that will be presented instead of the default, or if nil, the default behavior will be used.
+     */
+    @objc optional func label(_ label: InstructionLabel, willPresent instruction: VisualInstruction, as presented: NSAttributedString) -> NSAttributedString?


A delegate method’s first argument should always be the delegator. Is the delegator an InstructionLabel, or something else? Can it be either or two classes? That would be an antipattern in Cocoa.

Does this “will” method’s name make the implementation’s purpose clear enough? Is it clear that it’s an opportunity to override the presentation?

To answer your first question, yes, the label is the delegator.

1ec5 · 2018-07-02T18:20:44Z

MapboxNavigation/InstructionLabel.swift

+     - parameter presented: the formatted string that is provided by the instruction presenter
+     - returns: optionally, a customized NSAttributedString that will be presented instead of the default, or if nil, the default behavior will be used.
+     */
+    @objc optional func label(_ label: InstructionLabel, willPresent instruction: VisualInstruction, as presented: NSAttributedString) -> NSAttributedString?


The Objective-C signature of this method is currently -label:willPresent:as:. I think it should be -…:willPresentVisualInstruction:label:presentedAttributedString: or somesuch.

Ah, right you are. Good catch. Will fix.

bsudekum · 2018-07-03T21:58:06Z

Examples/Swift/ViewController.swift

+// Mark: VisualInstructionDelegate
+extension ViewController: VisualInstructionDelegate {
+    func label(_ label: InstructionLabel, willPresent instruction: VisualInstruction, as presented: NSAttributedString) -> NSAttributedString? {
+        guard exampleMode == .customInstruction else { return nil }


With VoiceController(_:WillSpeak:Instruction:), returning nil omits the voice instruction. I'm guess that's not the case here? In reality, I can't really think of a use case where a developer would want to show nothing though.

That's kind of the idea. The utility of "I only want to override default behavior under specific circumstances" seems to outweigh the utility of "it's nice to say 'show nothing' via nil instead of NSAttributedString()". In this specific example you could also implement default behavior by returning presented, but I think we should get into the general habit of using a "returning nil means the delegate has no answer to the delegator, and thus default behavior should be used" pattern in our delegation, as otherwise you kind of force the developer to implement logic if they implement the optional delegate function. Not a super great pattern.

bsudekum · 2018-07-03T22:01:22Z

With #1514 coming down the line, can you think of anything that might make this play nicely with the lanes view? Maybe returning nil hides/omits the view? #1530 (comment)

bsudekum · 2018-07-03T22:05:08Z

MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift

@@ -69,6 +71,16 @@ class InstructionsBannerViewIntegrationTests: XCTestCase {

        super.tearDown()
    }
+
+    func testCustomVisualInstructionDelegate() {


Also add a test for returning nil.

Test for nil response

…ns no answer.

frederoni · 2018-07-04T13:02:37Z

MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift

+
+    func testCustomVisualInstructionDelegate() {
+        let view = instructionsView()
+        view.instructionDelegate = TextReversingDelegate()


instructionDelegate will be deallocated immediately because it's weak. Keep a local ref to TextReversingDelegate(). (×2)

… it's visual instruction delegate.

…test.

Adding delegate method to customize attributed string that is compute…

38a2442

…d by `InstructionPresenter`.

JThramer added feature New feature request. topic: instructions Needs platform parity ticket labels Jun 29, 2018

JThramer self-assigned this Jun 29, 2018

bsudekum reviewed Jun 29, 2018

View reviewed changes

JThramer added ⚠️ DO NOT MERGE PR should not be merged! wip labels Jun 29, 2018

JThramer removed the Needs platform parity ticket label Jul 2, 2018

1ec5 reviewed Jul 2, 2018

View reviewed changes

Jerrad Thramer added 3 commits July 2, 2018 15:21

Adding test for VisualInstructionDelegate.

ebc036c

Adding OBJ-C method name that's more in-line with convention.

b128e67

Adding example.

eb39237

JThramer added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jul 3, 2018

bsudekum reviewed Jul 3, 2018

View reviewed changes

Jerrad Thramer added 2 commits July 3, 2018 16:23

Making these constant since they are never mutated

f397d9c

Adding test that ensures default behavior is used when delegate retur…

876505f

…ns no answer.

frederoni reviewed Jul 4, 2018

View reviewed changes

Jerrad Thramer added 2 commits July 5, 2018 11:47

Fixing issue where InstructionLabel was retaining strong ownership of…

90e8012

… it's visual instruction delegate.

Providing local ownership of newly orphaned delegates in integration …

10c9ec5

…test.

bsudekum approved these changes Jul 5, 2018

View reviewed changes

JThramer merged commit 7fd6f1f into master Jul 5, 2018

JThramer deleted the jerrad/instruction-presentation-customization branch July 5, 2018 21:25

This was referenced Mar 6, 2019

An app developer should be able to perform client-side customization of banner instructions #1503

Closed

Allow application to insert visual instructions on demand #2011

Open

1ec5 mentioned this pull request Nov 8, 2021

NavigationViewControllerDelegate unable to override instruction label #3567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visual Instruction Presentation Customization #1530

Visual Instruction Presentation Customization #1530

JThramer commented Jun 29, 2018 •

edited

Loading

bsudekum Jun 29, 2018

JThramer Jun 29, 2018

JThramer commented Jun 29, 2018 •

edited

Loading

1ec5 Jul 2, 2018

JThramer Jul 2, 2018

1ec5 Jul 2, 2018

JThramer Jul 2, 2018

bsudekum Jul 3, 2018

JThramer Jul 3, 2018

bsudekum commented Jul 3, 2018

bsudekum Jul 3, 2018

JThramer Jul 3, 2018 •

edited

Loading

frederoni Jul 4, 2018

Visual Instruction Presentation Customization #1530

Visual Instruction Presentation Customization #1530

Conversation

JThramer commented Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer commented Jun 29, 2018 • edited Loading

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

bsudekum commented Jul 3, 2018

Choose a reason for hiding this comment

JThramer Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer commented Jun 29, 2018 •

edited

Loading

JThramer commented Jun 29, 2018 •

edited

Loading

JThramer Jul 3, 2018 •

edited

Loading