-
Notifications
You must be signed in to change notification settings - Fork 103
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
Remove Deprecations #744
Remove Deprecations #744
Conversation
* Fixed a bug in `SDLConfiguration` and added additional tests
Codecov Report
@@ Coverage Diff @@
## release/5.0.0 #744 +/- ##
===============================================
Coverage ? 4.96%
===============================================
Files ? 318
Lines ? 7248
Branches ? 655
===============================================
Hits ? 360
Misses ? 6862
Partials ? 26 |
SmartDeviceLink/SDLConfiguration.m
Outdated
@@ -59,6 +59,7 @@ - (instancetype)initWithLifecycle:(SDLLifecycleConfiguration *)lifecycleConfig l | |||
_lifecycleConfig = lifecycleConfig; | |||
_lockScreenConfig = lockScreenConfig ?: [SDLLockScreenConfiguration enabledConfiguration]; | |||
_loggingConfig = logConfig ?: [SDLLogConfiguration defaultConfiguration]; | |||
_streamingMediaConfig = streamingMediaConfig; | |||
|
|||
if (_streamingMediaConfig != nil) { | |||
NSAssert(!([_lifecycleConfig.appType isEqualToEnum:SDLAppHMITypeNavigation] || [_lifecycleConfig.appType isEqualToEnum:SDLAppHMITypeProjection]), @"You should only set a streaming media configuration if your app is a NAVIGATION or PROJECTION HMI type"); |
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.
This assert statement is getting called when app type is set to Navigation. Since assert statements only get called if they are false, the ! should be removed from the conditional statement.
@@ -17,11 +17,6 @@ NS_ASSUME_NONNULL_BEGIN | |||
- (void)handleProtocolEndServiceNAKMessage:(SDLProtocolMessage *)endServiceNAK; | |||
|
|||
// Older protocol handlers | |||
- (void)handleProtocolStartSessionACK:(SDLServiceType)serviceType sessionID:(Byte)sessionID version:(Byte)version __deprecated_msg("use handleProtocolStartSessionACKMessage: instead"); |
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.
The handleProtocolStartSessionACK
protocol handler is still being used in the SDLProxy
class. The SDLProxy
class needs to implement the handleProtocolStartServiceACKMessage
protocol handler, otherwise setup will not complete. This is why setup is not finishing when the protocol string is com.smartdevicelink.prot2.
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.
I found a couple of bugs.
- (void)handleProtocolStartSessionACK:(SDLProtocolHeader *)header __deprecated_msg("use handleProtocolStartSessionACKMessage: instead"); | ||
- (void)handleProtocolStartSessionNACK:(SDLServiceType)serviceType __deprecated_msg("use handleProtocolStartSessionNAKMessage: instead"); | ||
- (void)handleProtocolEndSessionACK:(SDLServiceType)serviceType __deprecated_msg("use handleProtocolEndSessionACKMessage: instead"); | ||
- (void)handleProtocolEndSessionNACK:(SDLServiceType)serviceType __deprecated_msg("use handleProtocolEndSessionNAKMessage: instead"); | ||
- (void)handleHeartbeatForSession:(Byte)session; |
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.
handleProtocolStartSessionACK
, handleProtocolStartSessionNACK
, handleProtocolEndSessionACK
, and handleProtocolEndSessionNACK
are all being used in the SDLStreamingMediaLifecycleManager
. This means that video is not streaming because setup does not finish.
# Conflicts: # SmartDeviceLink/SDLAbstractProtocol.h # SmartDeviceLink/SDLAbstractProtocol.m # SmartDeviceLink/SDLStreamingMediaLifecycleManager.m
@NicoleYarroch Fixed and merged base back in |
Fixes #679
This PR is ready for review.
Risk
This PR makes major API changes.
Testing Plan
Tests have been updated
Summary
This PR removes deprecated code (and also fixes a bug in SDLConfiguration)
Changelog
Breaking Changes
CLA