Make FeedbackViewController public #1605

bsudekum · 2018-08-14T21:54:32Z

This PR makes the FeedbackViewController public for developers to show within their custom navigation UI.

todo:

document.
convert public structs to classes.
figure out why self.parent is nil in many instances.

/cc @mapbox/navigation-ios

bsudekum · 2018-08-14T21:59:56Z

MapboxNavigation/FeedbackViewController.swift

+            guard let strongSelf = self else { return }
+
+            // todo, can this be moved as a delegate on this view controller?
+            //strongSelf.delegate?.mapViewController(strongSelf, didSendFeedbackAssigned: uuid, feedbackType: item.feedbackType)


Would it make sense to make a protocol for FeedbackViewController here? This would remove some plumbing.

Is this delegate method even helpful? It only returns the uuid and the feedback type.

To make it useful, pass in the feedback item itself instead of the feedback type.

bsudekum · 2018-08-14T22:00:15Z

MapboxNavigation/FeedbackViewController.swift

+            }
+
+            strongSelf.dismiss(animated: true) {
+                DialogViewController().present(on: parent)


For some reason, parent is nil here.

It may be because you're using a custom transitioning coordinator. If so, you may need to manually add the child VC and do all the lifecycle methods, such as UIViewController.willMove(toParentViewController:)

@JThramer solved with bc32b5e#diff-a25e8b3bfe9ceec368b83a004a6d1708R200. How do you feel about it?

Ah yeah. That makes a lot more sense. 👍

1ec5 · 2018-08-14T22:11:12Z

MapboxNavigation/FeedbackItem.swift

@@ -7,7 +7,7 @@ extension UIImage {
    }
 }

-struct FeedbackItem {
+public struct FeedbackItem {


Note that a Swift struct can’t bridge to Objective-C, nor can any method or property that uses that type.

1ec5 · 2018-08-14T22:14:18Z

MapboxNavigation/FeedbackViewController.swift

@@ -3,29 +3,25 @@ import MapboxCoreNavigation
 import AVFoundation

 extension FeedbackViewController: UIViewControllerTransitioningDelegate {
-    func animationController(forDismissed dismissed: UIViewController) -> UIViewControllerAnimatedTransitioning? {
+    public func animationController(forDismissed dismissed: UIViewController) -> UIViewControllerAnimatedTransitioning? {


If we make these methods public, we’ll need to give them less awkward names (forDismissing?) and documentation and consider making them accessible to Objective-C.

Noting, we have no control over the naming conventions of this method since it's a func defined on UIViewControllerTransitioningDelegate .

So it is! 😄 (By the way, the Objective-C method name looks fine; it’s just the Swift name that’s funky.) Oh well, carry on.

1ec5 · 2018-08-14T22:17:05Z

MapboxNavigation/FeedbackViewController.swift

-    override func viewDidLoad() {
+    var eventsManager: EventsManager
+
+    public init(for eventsManager: EventsManager) {


Use a more descriptive argument label: init(eventsManager:).

1ec5 · 2018-08-14T22:17:40Z

MapboxNavigation/FeedbackViewController.swift

+    }
+
+    required public init?(coder aDecoder: NSCoder) {
+        fatalError("init(coder:) has not been implemented")


Should this method be implemented?

frederoni · 2018-08-19T19:20:46Z

Examples/Swift/CustomViewController.swift

@@ -31,6 +33,8 @@ class CustomViewController: UIViewController, MGLMapViewDelegate {

        mapView.delegate = self
        mapView.compassView.isHidden = true
+
+        feedbackViewController = FeedbackViewController(eventsManager: routeController.eventsManager)


Postpone initializing the FeedbackViewController until we present it.

Why is that? Is it really that expensive to just keep it in memory?

It's not that expensive, I'd guess a few MBs but it's good practice not to allocate view controllers, views, and objects in general before use, unless it needs to be pre-cached.

In this case, the feedback view controller is not always going to be presented, e.g. if you cancel navigation or never reach the destination, making it excessive to keep in memory during the whole session.

Also, some settings or state might change after the view is loaded, e.g. landscape orientation, or accessibility settings. Then the view could've been created w/ an incorrect draggable height, font settings etc...

bsudekum · 2018-08-20T21:43:07Z

This is ready for review.

1ec5 · 2018-08-21T06:51:24Z

MapboxNavigation/FeedbackItem.swift

@@ -7,7 +7,8 @@ extension UIImage {
    }
 }

-struct FeedbackItem {
+@objc(MBFeedbackItem)


This class needs documentation.

1ec5 · 2018-08-21T06:51:53Z

MapboxNavigation/FeedbackItem.swift

@@ -7,7 +7,8 @@ extension UIImage {
    }
 }

-struct FeedbackItem {
+@objc(MBFeedbackItem)
+public class FeedbackItem: NSObject {
    var title: String
    var image: UIImage
    var feedbackType: FeedbackType


Do any of these properties need to be public? What about the initializer?

I think the initializer should be enough, no?

The initializer is now public, but the arguments passed into the initializer are thereafter inaccessible to client code, making the feedback item something of a dropbox. I think we should make these three properties public for consistency.

1ec5 · 2018-08-21T06:53:19Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     The feedback items that are visible and selectable by the user.
+     */
+    public var sections: [[FeedbackItem]] = [[.turnNotAllowed, .closure, .reportTraffic, .confusingInstructions, .generalMapError, .badRoute]]


Why is this an array nested inside an array? What does the outer array represent?

Nested sections were previously used to forcefully use one or two rows based on device orientation. This is being handled automatically now:

mapbox-navigation-ios/MapboxNavigation/FeedbackViewController.swift

Lines 287 to 289 in 4fa5157

let width = traitCollection.verticalSizeClass == .compact

? floor(availableWidth / CGFloat(items.count))

: floor(availableWidth / CGFloat(items.count / 2))

So should be fine to flatten [[FeedbackItem]] to [FeedbackItem] and always use the first section. However, keeping the current structure is a bit more versatile.

1ec5 · 2018-08-21T06:54:25Z

MapboxNavigation/FeedbackViewController.swift

@@ -71,7 +75,33 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog
        return fullHeight
    }

-    override func viewDidLoad() {
+    var eventsManager: EventsManager


This property (or at least its getter) should be public. If application code can pass an EventsManger into the initializer, it should be able to see which EventsManager the object wound up with.

1ec5 · 2018-08-21T06:56:12Z

MapboxNavigation/FeedbackViewController.swift

@@ -165,10 +202,48 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog
        progressBar.trailingAnchor.constraint(equalTo: view.trailingAnchor).isActive = true
        progressBar.bottomAnchor.constraint(equalTo: view.safeBottomAnchor).isActive = true
    }
+
+    func defaultFeedbackHandlers(source: FeedbackSource = .user) -> (send: (FeedbackItem) -> Void, dismiss: () -> Void) {


Inline this method into the one call site, to avoid the cognitive overhead of passing these closures around.

bsudekum · 2018-08-22T17:25:37Z

@1ec5 updated

1ec5

To-do items need to be addressed or removed before merging.

1ec5 · 2018-08-23T05:38:21Z

MapboxNavigation/FeedbackItem.swift

@@ -7,7 +7,8 @@ extension UIImage {
    }
 }

-struct FeedbackItem {
+@objc(MBFeedbackItem)
+public class FeedbackItem: NSObject {
    var title: String
    var image: UIImage
    var feedbackType: FeedbackType


The initializer is now public, but the arguments passed into the initializer are thereafter inaccessible to client code, making the feedback item something of a dropbox. I think we should make these three properties public for consistency.

1ec5 · 2018-08-23T05:39:08Z

MapboxNavigation/FeedbackItem.swift

+    /**
+     Creates a new `FeedbackItem`.
+     */
+    public init(title: String, image: UIImage, feedbackType: FeedbackType) {


Add @objc to make this initializer accessible to Objective-C code.

1ec5 · 2018-08-23T05:42:00Z

MapboxNavigation/FeedbackViewController.swift

+            guard let strongSelf = self else { return }
+
+            // todo, can this be moved as a delegate on this view controller?
+            //strongSelf.delegate?.mapViewController(strongSelf, didSendFeedbackAssigned: uuid, feedbackType: item.feedbackType)


To make it useful, pass in the feedback item itself instead of the feedback type.

1ec5 · 2018-08-23T05:45:03Z

MapboxNavigation/FeedbackViewController.swift

@@ -165,12 +205,42 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog
        progressBar.trailingAnchor.constraint(equalTo: view.trailingAnchor).isActive = true
        progressBar.bottomAnchor.constraint(equalTo: view.safeBottomAnchor).isActive = true
    }
+
+    func defaultSendFeedbackHandler(source: FeedbackSource = .user, uuid: UUID) -> (FeedbackItem) -> Void {


There’s only one call site and it doesn’t specify the source argument, so inline the default .user value below.

1ec5 · 2018-08-23T05:47:47Z

MapboxNavigation/FeedbackViewController.swift

+
+        let uuid = eventsManager.recordFeedback()
+        sendFeedbackHandler = defaultSendFeedbackHandler(uuid: uuid)
+        dismissFeedbackHandler = defaultDismissFeedbackHandler(uuid: uuid)


Instead of currying closures within defaultSendFeedbackHandler(uuid:) and defaultDismissFeedbackHandler(uuid:), pull those methods into simple send(_:withUUID:) and dismiss(_:) methods and store uuid on the class instead. Then, instead of calling sendFeedbackHandler, call send(_:withUUID:).

frederoni · 2018-08-23T16:26:15Z

MapboxNavigation/FeedbackViewController.swift

@@ -39,6 +57,8 @@ public class FeedbackViewController: UIViewController, DismissDraggable, UIGestu
     */
    public var sections: [FeedbackItem] = [.turnNotAllowed, .closure, .reportTraffic, .confusingInstructions, .generalMapError, .badRoute]

+    public var delegate: FeedbackViewControllerDelegate?


1ec5 · 2018-08-23T17:51:58Z

MapboxNavigation/FeedbackViewController.swift

@@ -165,12 +227,32 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog
        progressBar.trailingAnchor.constraint(equalTo: view.trailingAnchor).isActive = true
        progressBar.bottomAnchor.constraint(equalTo: view.safeBottomAnchor).isActive = true
    }
+
+    func sendFeedback(_ item: FeedbackItem) {


Nit: send(_:) is the more Swiftian way of naming it these days, or sendFeedbackItem(_:) if there’s a conflict with another send(_:).

1ec5 · 2018-08-23T17:52:29Z

MapboxNavigation/FeedbackViewController.swift

+        }
+    }
+
+    func defaultDismissFeedback() {


Just dismissFeedbackItem(), no longer “default”.

1ec5 · 2018-08-23T17:53:46Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     Called when a `FeedbackViewController` is dismissed for any reason without giving explicit feedback.
+     */
+    @objc optional func feedbackViewControllerDidCancelFeedback(_ feedbackViewController: FeedbackViewController)


Would it be possible to pass in the FeedbackItem so the application can distinguish between cancellations?

Then feedbackViewControllerDidCancel(_:).

1ec5 · 2018-08-23T17:54:30Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     Called when the user submits a feedback event.
+     */
+    @objc optional func feedbackViewController(_ feedbackViewController: FeedbackViewController, didSendFeedbackAssigned: UUID, feedback: FeedbackItem)


Name this method feedbackViewController(_:didSend:uuid:) in Swift and -feedbackViewController:didSendFeedbackItem:UUID: in Objective-C.

1ec5 · 2018-08-23T17:55:10Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     Called when the user opens the feedback form.
+     */
+    @objc optional func  feedbackViewControllerDidOpenFeedback(_ feedbackViewController: FeedbackViewController)


Name this method feedbackViewControllerDidOpen(_:), since it gets called when opening the form, not when opening a specific piece of feedback.

1ec5 · 2018-08-23T20:42:03Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     Called when the user submits a feedback event.
+     */
+    @objc optional func feedbackViewController(_ feedbackViewController: FeedbackViewController, didSend: FeedbackItem, UUID: UUID)


Distinguish between the argument label and argument name: didSend feedbackItem: FeedbackItem

The Swift name is currently feedbackViewController(_:didSend:UUID:); it should be feedbackViewController(_:didSend:uuid:)

The Objective-C name is currently -feedbackViewController:didSend:UUID:; it should be -feedbackViewController:didSendFeedbackItem:UUID:

1ec5 · 2018-08-23T20:42:39Z

MapboxNavigation/FeedbackViewController.swift

+    /**
+     Called when a `FeedbackViewController` is dismissed for any reason without giving explicit feedback.
+     */
+    @objc optional func feedbackViewControllerDidCancelFeedback(_ feedbackViewController: FeedbackViewController)


Then feedbackViewControllerDidCancel(_:).

1ec5

Remember to mention any new APIs in the changelog.

Make FeedbackViewController public

2692fd5

bsudekum commented Aug 14, 2018

View reviewed changes

Bobby Sudekum added 2 commits August 14, 2018 15:02

use class

5dd01cd

@objc

194a5b1

1ec5 reviewed Aug 14, 2018

View reviewed changes

Bobby Sudekum added 2 commits August 15, 2018 08:59

Consolidate

bc32b5e

nscoder

e77d7f4

bsudekum added the ✓ ready for review label Aug 17, 2018

frederoni reviewed Aug 19, 2018

View reviewed changes

Bobby Sudekum added 4 commits August 20, 2018 09:50

Merge branch 'master' into public-feedback

83783c1

lazy

f9c3b5e

No need for this

b7b79f3

doc, objc

4fa5157

1ec5 reviewed Aug 21, 2018

View reviewed changes

Make more public, use one section, doc

0801b8b

1ec5 reviewed Aug 23, 2018

View reviewed changes

Add delegate, make public, remove piping

0d465ed

frederoni reviewed Aug 23, 2018

View reviewed changes

Bobby Sudekum added 2 commits August 23, 2018 09:42

weak

4f56aed

Merge branch 'master' into public-feedback

c058839

1ec5 reviewed Aug 23, 2018

View reviewed changes

Bobby Sudekum added 2 commits August 23, 2018 11:14

Update names

72e3740

Let Style class define color

eaa640a

1ec5 reviewed Aug 23, 2018

View reviewed changes

Change names

41bf3ac

1ec5 approved these changes Aug 23, 2018

View reviewed changes

1ec5 reviewed Aug 23, 2018

View reviewed changes

changelog

7ccfa2d

bsudekum merged commit ae4dfb5 into master Aug 23, 2018

bsudekum deleted the public-feedback branch August 23, 2018 21:25

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018

1ec5 added this to the v0.20.0 milestone Dec 18, 2018

	let width = traitCollection.verticalSizeClass == .compact
	? floor(availableWidth / CGFloat(items.count))
	: floor(availableWidth / CGFloat(items.count / 2))

Make FeedbackViewController public #1605

Make FeedbackViewController public #1605

Conversation

bsudekum commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum Aug 14, 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

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

frederoni Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

bsudekum commented Aug 20, 2018

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

bsudekum commented Aug 22, 2018

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

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 left a comment

Choose a reason for hiding this comment

bsudekum commented Aug 14, 2018 •

edited

Loading

bsudekum Aug 14, 2018 •

edited

Loading

frederoni Aug 20, 2018 •

edited

Loading