Skip to content
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

Minimize NSObject inheritance #2294

Closed
1 of 10 tasks
1ec5 opened this issue Jan 6, 2020 · 4 comments · Fixed by #2352
Closed
1 of 10 tasks

Minimize NSObject inheritance #2294

1ec5 opened this issue Jan 6, 2020 · 4 comments · Fixed by #2352
Assignees
Labels
backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 6, 2020

As a followup to #2230, we should minimize the number of classes that explicitly inherit from NSObject. NSObject inheritance hinders Codable adoption and potentially has other subtle side effects.

The following classes look like good candidates for becoming ordinary Swift classes, though we’ll need to make sure there isn’t any KVO lurking around. Not listed here are any classes that need to conform to NSObjectProtocol because of some Foundation protocol inheritance such as CLLocationManagerDelegate.

  • RouteProgress, RouteLegProgress, and RouteStepProgress
    Multi-leg routing fix #2229 registered key-value observers on these classes. Clients can set indices directly and a third object, RouteController, needs to update in response to the change. Though Foundation’s Progress class also inherits from NSObject, it solves this problem by allowing an object to register blocks to handle specific changes. It’s basically a hard-coded subset of KVO, more structured than the delegate pattern. Removing NSObject usage here will unblock codability: Route progress classes should be codable #1940.

  • NavigationSettings
    This class also uses KVO, but I think only it registers on its own key paths, for the purpose of sending out notifications. Given the manageable number of properties and outstanding reflection mysteries like is not key value coding-compliant for the key  #2262, we should replace this mechanism with a series of didSets that call a common method.

  • MapboxNavigationService

  • NavigationOptions

  • NavigationEventsManager: Cleanup of Deprecated / Obsoleted Symbols #2297

  • EndOfRouteFeedback

  • FeedbackItem

  • CarPlaySearchManager

  • StyleManager

  • DataCache

/cc @mapbox/navigation-ios

@1ec5 1ec5 added op-ex Refactoring, Tech Debt or any other operational excellence work. backwards incompatible changes that break backwards compatibility of public API labels Jan 6, 2020
@1ec5 1ec5 added this to the v1.0.0 milestone Jan 6, 2020
@Udumft
Copy link
Contributor

Udumft commented Mar 16, 2020

I think there are some problems with following classes.
MapboxNavigationService conforms to CLLocationManagerDelegate. So it is required to implement NSObjectProtocol:

extension MapboxNavigationService: CLLocationManagerDelegate {

I couldn't find CarPlaySearchManager. There are CarPlaySearchController and CarPlayManager classes, inheriting NSObject but both require such inheritance due to protocols implementation:

extension CarPlaySearchController: CPSearchTemplateDelegate {

extension CarPlayManager: CPApplicationDelegate {

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

Udumft commented Mar 16, 2020

If I understood the suggested change, RouteProgress.legIndex notifications should use something like this:

  dynamic public var legIndex: Int {
      didSet {
          // ...
          legIndexHandler?(oldValue, legIndex)
      }
  }

  public typealias LegIndexSubscriptionAction = (_ oldValue: Int, _ newValue: Int) -> ()
  public var legIndexHandler: LegIndexSubscriptionAction?

But what if there will be more than 1 observer needed? What if user wants to observe any other property?
Maybe I should spend a bit more time on that and implement something that allow multiple subscribers and can be expanded to other properties?

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 16, 2020

MapboxNavigationService conforms to CLLocationManagerDelegate. So it is required to implement NSObjectProtocol

Ah, good point. That’s a legitimate reason to inherit from NSObject.

I couldn't find CarPlaySearchManager.

That was a typo; I meant CarPlaySearchController. But you’re right, CPSearchTemplateDelegate forces NSObjectProtocol conformance here too.

If I understood the suggested change, RouteProgress.legIndex notifications should use something like this:

But what if there will be more than 1 observer needed? What if user wants to observe any other property?

Do we want developers to be able to observe RouteProgress model changes directly, or should they observe routeControllerProgressDidChange notifications instead?

@Udumft
Copy link
Contributor

Udumft commented Mar 17, 2020

Disabling KVO for NotificationSettings will also update our own usage of subscriptions to settings updates. For example RouteVoiceController and MapboxVoiceController will be updated too:

muteToken = NavigationSettings.shared.observe(\.voiceMuted) { [weak self] (settings, change) in
if settings.voiceMuted {
self?.speechSynth.stopSpeaking(at: .immediate)
}
}

volumeToken = NavigationSettings.shared.observe(\.voiceVolume) { [weak self] (settings, change) in
self?.audioPlayer?.volume = settings.voiceVolume
}
muteToken = NavigationSettings.shared.observe(\.voiceMuted) { [weak self] (settings, change) in
if settings.voiceMuted {
self?.audioPlayer?.stop()
guard let strongSelf = self else { return }
strongSelf.safeUnduckAudio(instruction: nil, engine: self?.speech) {
strongSelf.voiceControllerDelegate?.voiceController(strongSelf, spokenInstructionsDidFailWith: $0)
}
}
}

This change will conflict with #2268 PR.

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

Successfully merging a pull request may close this issue.

2 participants