Make FeedbackViewController public #1605
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,29 +3,25 @@ import MapboxCoreNavigation | |
| import AVFoundation | ||
|
|
||
| extension FeedbackViewController: UIViewControllerTransitioningDelegate { | ||
| public func animationController(forDismissed dismissed: UIViewController) -> UIViewControllerAnimatedTransitioning? { | ||
|
There was a problem hiding this comment. If we make these methods public, we’ll need to give them less awkward names ( There was a problem hiding this comment. Noting, we have no control over the naming conventions of this method since it's a func defined on There was a problem hiding this comment. 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. |
||
| abortAutodismiss() | ||
| return DismissAnimator() | ||
| } | ||
|
|
||
| public func animationController(forPresented presented: UIViewController, presenting: UIViewController, source: UIViewController) -> UIViewControllerAnimatedTransitioning? { | ||
| return PresentAnimator() | ||
| } | ||
|
|
||
| public func interactionControllerForDismissal(using animator: UIViewControllerAnimatedTransitioning) -> UIViewControllerInteractiveTransitioning? { | ||
| return interactor.hasStarted ? interactor : nil | ||
| } | ||
| } | ||
|
|
||
| @objc(FeedbackViewController) | ||
| public class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecognizerDelegate { | ||
|
|
||
| var sendFeedbackHandler: ((FeedbackItem) -> Void)? | ||
| var dismissFeedbackHandler: (() -> Void)? | ||
| var activeFeedbackItem: FeedbackItem? | ||
|
|
||
| static let sceneTitle = NSLocalizedString("FEEDBACK_TITLE", value: "Report Problem", comment: "Title of view controller for sending feedback") | ||
|
|
@@ -35,6 +31,8 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog | |
|
|
||
| let interactor = Interactor() | ||
|
|
||
| public var sections: [[FeedbackItem]] = [[.turnNotAllowed, .closure, .reportTraffic, .confusingInstructions, .generalMapError, .badRoute]] | ||
|
There was a problem hiding this comment. Why is this an array nested inside an array? What does the outer array represent? There was a problem hiding this comment. 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 So should be fine to flatten |
||
|
|
||
| lazy var collectionView: UICollectionView = { | ||
| let view: UICollectionView = UICollectionView(frame: .zero, collectionViewLayout: flowLayout) | ||
| view.translatesAutoresizingMaskIntoConstraints = false | ||
|
|
@@ -71,7 +69,30 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog | |
| return fullHeight | ||
| } | ||
|
|
||
| var eventsManager: EventsManager | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| public init(eventsManager: EventsManager) { | ||
| self.eventsManager = eventsManager | ||
| super.init(nibName: nil, bundle: nil) | ||
| commonInit() | ||
| } | ||
|
|
||
| public override func encode(with aCoder: NSCoder) { | ||
| aCoder.encode(eventsManager, forKey: "EventsManager") | ||
| } | ||
|
|
||
| required public init?(coder aDecoder: NSCoder) { | ||
| eventsManager = aDecoder.decodeObject(of: [EventsManager.self], forKey: "EventsManager") as? EventsManager ?? EventsManager(accessToken: nil) | ||
| super.init(coder: aDecoder) | ||
| commonInit() | ||
| } | ||
|
|
||
| func commonInit() { | ||
| self.modalPresentationStyle = .custom | ||
| self.transitioningDelegate = self | ||
| } | ||
|
|
||
| override public func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| setupViews() | ||
| setupConstraints() | ||
|
|
@@ -80,14 +101,18 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog | |
| view.backgroundColor = .white | ||
| progressBar.barColor = #colorLiteral(red: 0.9347146749, green: 0.5047877431, blue: 0.1419634521, alpha: 1) | ||
| enableDraggableDismiss() | ||
|
|
||
| let defaults = defaultFeedbackHandlers() //this is done every time to refresh the feedback UUID | ||
| sendFeedbackHandler = defaults.send | ||
| dismissFeedbackHandler = defaults.dismiss | ||
| } | ||
|
|
||
| override public func viewWillAppear(_ animated: Bool) { | ||
| super.viewWillAppear(animated) | ||
| progressBar.progress = 1 | ||
| } | ||
|
|
||
| override public func viewDidAppear(_ animated: Bool) { | ||
| super.viewDidAppear(animated) | ||
|
|
||
| UIView.animate(withDuration: FeedbackViewController.autoDismissInterval) { | ||
|
|
@@ -97,7 +122,7 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog | |
| enableAutoDismiss() | ||
| } | ||
|
|
||
| override public func willTransition(to newCollection: UITraitCollection, with coordinator: UIViewControllerTransitionCoordinator) { | ||
| super.willTransition(to: newCollection, with: coordinator) | ||
|
|
||
| // Dismiss the feedback view when switching between landscape and portrait mode. | ||
|
|
@@ -131,7 +156,7 @@ class FeedbackViewController: UIViewController, DismissDraggable, UIGestureRecog | |
| dismissFeedbackHandler?() | ||
| } | ||
|
|
||
| public func gestureRecognizer(_ gestureRecognizer: UIGestureRecognizer, shouldReceive touch: UITouch) -> Bool { | ||
| // Only respond to touches outside/behind the view | ||
| let isDescendant = touch.view?.isDescendant(of: view) ?? true | ||
| return !isDescendant | ||
|
|
@@ -165,10 +190,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) { | ||
|
There was a problem hiding this comment. Inline this method into the one call site, to avoid the cognitive overhead of passing these closures around. |
||
| let uuid = eventsManager.recordFeedback() | ||
| let send = defaultSendFeedbackHandler(uuid: uuid) | ||
| let dismiss = defaultDismissFeedbackHandler(uuid: uuid) | ||
|
|
||
| return (send, dismiss) | ||
| } | ||
|
|
||
| func defaultSendFeedbackHandler(source: FeedbackSource = .user, uuid: UUID) -> (FeedbackItem) -> Void { | ||
|
There was a problem hiding this comment. There’s only one call site and it doesn’t specify the |
||
| return { [weak self] (item) in | ||
| 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) | ||
|
There was a problem hiding this comment. Would it make sense to make a protocol for FeedbackViewController here? This would remove some plumbing. There was a problem hiding this comment. Is this delegate method even helpful? It only returns the uuid and the feedback type. There was a problem hiding this comment. To make it useful, pass in the feedback item itself instead of the feedback type. |
||
| strongSelf.eventsManager.updateFeedback(uuid: uuid, type: item.feedbackType, source: source, description: nil) | ||
|
|
||
| guard let parent = strongSelf.presentingViewController else { | ||
| strongSelf.dismiss(animated: true) | ||
| return | ||
| } | ||
|
|
||
| strongSelf.dismiss(animated: true) { | ||
| DialogViewController().present(on: parent) | ||
|
There was a problem hiding this comment. For some reason, parent is nil here. There was a problem hiding this comment. 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 There was a problem hiding this comment. @JThramer solved with bc32b5e#diff-a25e8b3bfe9ceec368b83a004a6d1708R200. How do you feel about it? There was a problem hiding this comment. Ah yeah. That makes a lot more sense. 👍 |
||
| } | ||
| } | ||
| } | ||
|
|
||
| func defaultDismissFeedbackHandler(uuid: UUID) -> (() -> Void) { | ||
| return { [weak self ] in | ||
| guard let strongSelf = self else { return } | ||
|
|
||
| // todo, can this be moved as a delegate on this view controller? | ||
| // strongSelf.delegate?.mapViewControllerDidCancelFeedback(strongSelf) | ||
| strongSelf.eventsManager.cancelFeedback(uuid: uuid) | ||
| strongSelf.dismiss(animated: true, completion: nil) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| extension FeedbackViewController: UICollectionViewDataSource { | ||
| public func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell { | ||
| let cell = collectionView.dequeueReusableCell(withReuseIdentifier: FeedbackCollectionViewCell.defaultIdentifier, for: indexPath) as! FeedbackCollectionViewCell | ||
| let item = sections[indexPath.section][indexPath.row] | ||
|
|
||
|
|
@@ -179,15 +242,15 @@ extension FeedbackViewController: UICollectionViewDataSource { | |
| return cell | ||
| } | ||
|
|
||
| public func numberOfSections(in collectionView: UICollectionView) -> Int { | ||
| return sections.count | ||
| } | ||
|
|
||
| public func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int { | ||
| return sections[section].count | ||
| } | ||
|
|
||
| public func scrollViewDidScroll(_ scrollView: UIScrollView) { | ||
| // In case the view is scrolled, dismiss the feedback window immediately | ||
| // and reset the `progressBar` back to a full progress. | ||
| abortAutodismiss() | ||
|
|
@@ -196,16 +259,15 @@ extension FeedbackViewController: UICollectionViewDataSource { | |
| } | ||
|
|
||
| extension FeedbackViewController: UICollectionViewDelegate { | ||
| public func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) { | ||
| abortAutodismiss() | ||
| let item = sections[indexPath.section][indexPath.row] | ||
| sendFeedbackHandler?(item) | ||
| } | ||
| } | ||
|
|
||
| extension FeedbackViewController: UICollectionViewDelegateFlowLayout { | ||
| public func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize { | ||
| let availableWidth = collectionView.bounds.width | ||
| // 3 columns and 2 rows in portrait mode. | ||
| // 6 columns and 1 row in landscape mode. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postpone initializing the FeedbackViewController until we present it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that? Is it really that expensive to just keep it in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...