Skip to content

Minimize NSObject inheritance #2352

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

Merged
merged 3 commits into from
Apr 2, 2020
Merged

Conversation

Udumft
Copy link
Contributor

@Udumft Udumft commented Mar 17, 2020

Resolves #2294
Removes some objects NSObject Inheritance. Existing KVO observers are removed in favor of using NSNotifications observations.

There are also some other Public available classes, inheriting NSObject:

  • LanesStyleKit
  • ManeuversStyleKit
  • StyleKitMarker
  • SpeedLimitStyleKit
  • Style

Didn't check in detail, if such inheritance is legitimate, but quick removing NSObject from declaration showed successful compile. Should I check these too?

Victor Kononov added 3 commits March 17, 2020 12:36
…ogress, RouteStepProgress, DataCache, FeedbackItem, NavigationOptions, StyleManager
…ndler callback to respond to legIndex updates
@Udumft Udumft added op-ex Refactoring, Tech Debt or any other operational excellence work. backwards incompatible changes that break backwards compatibility of public API labels Mar 17, 2020
@Udumft Udumft added this to the v1.0.0 milestone Mar 17, 2020
@Udumft Udumft requested review from JThramer and 1ec5 March 17, 2020 14:11
@Udumft Udumft self-assigned this Mar 17, 2020
@@ -34,15 +34,16 @@ open class RouteController: NSObject {
}

private var _routeProgress: RouteProgress {
willSet {
resetObservation(for: _routeProgress)
}
didSet {
movementsAwayFromRoute = 0
updateNavigator(with: _routeProgress)
updateObservation(for: _routeProgress)
Copy link
Contributor Author

@Udumft Udumft Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we not only subscribe to legIndex updates here, but also run the subscription handler too to update RouteController with new _routeProgress?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by this? _routeProgress is the memoized property that represent's RouteController's copy of the current route progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When _routeProgress is set, we observe it's legIndex to call updateRouteLeg when it is updated. But shouldn't we call updateRouteLeg right now, since we've just updated the entire _routeProgress and thus it's legIndex is technically updated too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Udumft No, it's unnecessary, because the leg is set when we call updateNavigator(with:).

Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'm not sure if this question you asked is stale or not.

@@ -34,15 +34,16 @@ open class RouteController: NSObject {
}

private var _routeProgress: RouteProgress {
willSet {
resetObservation(for: _routeProgress)
}
didSet {
movementsAwayFromRoute = 0
updateNavigator(with: _routeProgress)
updateObservation(for: _routeProgress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by this? _routeProgress is the memoized property that represent's RouteController's copy of the current route progress.

@Udumft Udumft merged commit 441ffc3 into master Apr 2, 2020
@Udumft Udumft deleted the 2294-minimize-nsobject-inheritance branch April 2, 2020 14:24
@1ec5 1ec5 modified the milestones: v1.0.0, v0.x next Apr 14, 2020
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 this pull request may close these issues.

Minimize NSObject inheritance
3 participants