SDL 0088 System Capability Manager #916

NicoleYarroch · 2018-03-27T20:46:46Z

Fixes #720

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Added the SDLSystemCapabilityManagerSpec file

Summary

This PR creates a centralized manager for accessing all system capabilities. These capabilities were previously scattered between the RegisterAppInterfaceResponse, SetDisplayLayoutResponse and SDLGetSystemCapabilityResponse.

Changelog

Breaking Changes

None

Bug Fixes

Fixed broken test cases in the SDLScreenManagerSpec file.
Fixed broken change registration test cases in the SDLLifecycleManagerSpec file.
Fixed failed verification checks in the SDLLifecycleManagerSpec file. The failed tests were due to notifications for the SDLManagerDelegate being sent on the main thread, creating a slight delay in sending the notification. The tests are now forced to run immediately on the main thread.

Tasks Remaining:

Add test cases to SDLLifecycleMangerSpec
Use new API in Example app
Review documentation

CLA

I have signed the CLA

* added `SDLSystemCapabilitiesManager` to .podspec files * added `SDLSystemCapabilitiesManager` to SmartDeviceLink.h file * made `SDLSystemCapabilitiesManager` header public

* Used the `SDLSystemCapabilityManager` to: * check if the head unit supports graphics * check if the head unit supports making phone calls

Fixed failed verification checks in the SDLLifecycleManagerSpec file. The failed tests were due to notifications for the SDLManagerDelegate being sent on the main thread, causing a slight delay in sending the notification. The tests are now forced to run immediately on the main thread.

joeljfischer · 2018-04-04T13:33:57Z

SmartDeviceLink/SDLManager.h

@@ -95,8 +96,16 @@ typedef void (^SDLManagerReadyBlock)(BOOL success, NSError *_Nullable error);
 */
 @property (strong, nonatomic, readonly, nullable) SDLStreamingMediaManager *streamManager;

+/**
+ *  The screen manager for sending SDLShow RPCs.


It's actually for UI related RPCs, we just started with Show

joeljfischer · 2018-04-04T13:41:36Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+ *
+ * Optional, Array of length 1 - 100, of SDLPrerecordedSpeech
+ */
+@property (nullable, copy, nonatomic, readonly) NSArray<SDLPrerecordedSpeech> *prerecordedSpeech;


I'd prefer a better name for this, like supportedPrerecordedSpeechTypes, or to match with the others prerecordedSpeechCapabilities.

Fixed. Renamed var to prerecordedSpeechCapabilities

joeljfischer · 2018-04-04T13:42:58Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+ *
+ * Optional, Array of length 1 - 100, of SDLVRCapabilities
+ */
+@property (nullable, copy, nonatomic, readonly) NSArray<SDLVRCapabilities> *vrCapabilities;


What does this even do? We only have one enum defined...if it's defined it's supported? Should we change this to a BOOL in the manager? Why is it an array...gah.

Fixed. Changed to a BOOL.

joeljfischer · 2018-04-04T13:59:55Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+ *
+ *  @param error The error returned if the request for a capability type failed. The error is nil if the request was successful.
+ */
+typedef void (^SDLUpdateCapabilityHandler)(NSError * _Nullable error);


I would like to see this pass back the manager as well, so that the dev doesn't have to do the weak / strong dance.

joeljfischer · 2018-04-04T14:11:17Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface SDLSystemCapabilityManager : NSObject


It would be nice to get a callback when capabilities have changed, either to observe an individual one, or a group, or all. It wasn't a part of the proposal, but we should record in an issue that these would be nice enhancements.

Added issue #925.

joeljfischer · 2018-04-04T14:14:56Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+
+- (void)updateCapabilityType:(SDLSystemCapabilityType)type completionHandler:(SDLUpdateCapabilityHandler)handler {
+    SDLGetSystemCapability *getSystemCapability = [[SDLGetSystemCapability alloc] initWithType:type];
+    [self.connectionManager sendConnectionManagerRequest:getSystemCapability withResponseHandler:^(__kindof SDLRPCRequest *request, __kindof SDLRPCResponse *response, NSError *error) {


Do we want to use the manager version? This is an API available to devs and therefore allows them to bypass the checks we have in the lifecycle manager.

Fixed. Now using sendConnectionRequest:withResponseHandler.

joeljfischer · 2018-04-04T14:27:40Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+        } else if ([systemCapabilityType isEqualToEnum:SDLSystemCapabilityTypeVideoStreaming]) {
+            self.videoStreamingCapability = systemCapabilityResponse.videoStreamingCapability;
+        } else {
+            NSAssert(NO, @"Received response for unknown SDLSystemCapabilityType: %@", systemCapabilityType);


I'm trying to decide if we should assert or do nothing...

I removed the assert and replaced it with a log message.

joeljfischer · 2018-04-04T14:31:12Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+
+- (void)updateCapabilityType:(SDLSystemCapabilityType)type completionHandler:(SDLUpdateCapabilityHandler)handler {
+    SDLGetSystemCapability *getSystemCapability = [[SDLGetSystemCapability alloc] initWithType:type];
+    [self.connectionManager sendConnectionManagerRequest:getSystemCapability withResponseHandler:^(__kindof SDLRPCRequest *request, __kindof SDLRPCResponse *response, NSError *error) {


We should observe the response handler for the GetSystemCapabilities so that we can pull those which the dev uses outside of our manager and capture them as well. Then we can move this response handler code to that method.

Is there an example of how this works somewhere the library? I'm now sure how to intercept a request sent outside of this manager.

You're doing it with SetDisplayLayout in this manager

joeljfischer · 2018-04-04T14:53:33Z

SmartDeviceLinkTests/SDLSystemCapabilityManagerSpec.m

+            testSetDisplayLayoutResponse.presetBankCapabilities = testPresetBankCapabilities;
+        });
+
+        afterEach(^{


It took me a while to figure out how this worked. While it's pretty clever, I'm not really a fan because it depends on the order of the its and it is quite brittle and doesn't allow for very much flexibility. If we add a new it, or rearrange the order of the its, it could require a refactor, or be quite confusing to a new contributor.

Not sure what you mean by the order of the its. Since all values being tested get reset to nil in the topmost beforeEach, the order in which the tests are performed doesn't matter.

If we have to check all 15 vars in each test, it is going to be a pain to keep the tests updated when new system capabilities are added.

I guess what I'm wondering is why you're using afterEach so much instead of beforeEach

Don't beforeEach and afterEach both exist to avoid writing redundant code? Each test case only has two afterEachs that get's called which doesn't seem like a lot to me.

No, my question is what the purpose of afterEach is rather than using a beforeEach in this case.

joeljfischer · 2018-04-04T15:11:22Z

SmartDeviceLink_Example/Classes/ProxyManager.m

+        [manager.systemCapabilityManager updateCapabilityType:SDLSystemCapabilityTypePhoneCall completionHandler:^(NSError * _Nullable error) {
+            if (error == nil) {
+                SDLLogD(@"Dialing number");
+                [self.class sdlex_sendDialNumberWithManager:manager];


This also needs to check the permission manager. Most apps don't have permission for this, probably including this app.

Renamed var prerecordedSpeech to prerecordedSpeechCapabilities Renamed vrCapabilities to vrCapability

- Replaced with error message

Renamed the pcmStreamCapabilities variable in the SDLSystemCapabilityManager

…tiesResponse

joeljfischer · 2018-04-16T14:38:14Z

The above changelog should include the enhancements of this proposal implementation so it's easier to copy it into CHANGELOG.md when the release comes.

joeljfischer · 2018-04-16T14:47:29Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+@property (nullable, copy, nonatomic, readwrite) NSArray<SDLPrerecordedSpeech> *prerecordedSpeechCapabilities;
+@property (nonatomic, readwrite) BOOL vrCapability;
+@property (nullable, copy, nonatomic, readwrite) NSArray<SDLAudioPassThruCapabilities *> *audioPassThruCapabilities;
+@property (nullable, copy, nonatomic, readwrite) SDLAudioPassThruCapabilities *pcmStreamCapability;


The non-array ones should be strong, not copy, right?

joeljfischer · 2018-04-16T14:51:50Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+
+    return self;
+}
+


This should have a stop method so that internal data can be cleared when the lifecycle manager stops.

joeljfischer · 2018-04-16T14:56:09Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+ *  @param error The error returned if the request for a capability type failed. The error is nil if the request was successful.
+ *  @param systemCapabilityManager The system capability manager
+ */
+typedef void (^SDLUpdateCapabilityHandler)(NSError * _Nullable error, SDLSystemCapabilityManager *systemCapabilityManager);


This should be up at the top, outside of the interface.

Signed-off-by: NicoleYarroch <nicole@livio.io>

joeljfischer · 2018-04-17T18:34:51Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+
+- (void)stop {
+    SDLLogD(@"System Capability manager stopped");
+    [[NSNotificationCenter defaultCenter] removeObserver:self];


This method should not remove the observer, or else if you disconnect and reconnect, the observers will never fire, because this is not re-inited. This should clear all the internal capability data however. Or else, all this data will continue to be available while we disconnect and will still be there on a reconnect, even if it's to a different head unit.

joeljfischer · 2018-04-17T18:38:25Z

SmartDeviceLink/SDLSystemCapabilityManager.h

+ *
+ * True if the head unit supports voice recognition; false if not.
+ */
+@property (nonatomic, readonly) BOOL vrCapability;


(nonatomic, assign, readonly)

Signed-off-by: NicoleYarroch <nicole@livio.io>

NicoleYarroch added 9 commits March 26, 2018 15:50

Added SDLSystemCapabilityManager class

962cbd8

Fixed incorrectly capitalized SDLVrCapabilities

82b22a6

Reverted to old VR capitalization

0718b9a

Added SDLSystemCapabilitiesManager to .podspec files

e486431

* added `SDLSystemCapabilitiesManager` to .podspec files * added `SDLSystemCapabilitiesManager` to SmartDeviceLink.h file * made `SDLSystemCapabilitiesManager` header public

Added system capability manager to the sdl manager

c238923

Added test cases for system capability manager

5970752

Basic test cases for system capability manager setup

0e8e350

Test cases finished for the SDLSystemCapabilityManagerSpec

9692a8f

Merge branch 'develop' into feature/issue_720_system_capability_manager

7921c60

NicoleYarroch self-assigned this Mar 27, 2018

NicoleYarroch added enhancement proposal Accepted SDL Evolution Proposal labels Mar 27, 2018

NicoleYarroch added this to the 5.3.0 milestone Mar 27, 2018

NicoleYarroch added 11 commits March 27, 2018 16:49

Reverted to old spelling of VR

1ee0633

Fixed broken test case

8e912c7

Removed fdescribe from test case

fcf7222

Added check for system capability manager to lifecycle manager spec

438a66d

Fixed lifecycle manager tests

7b086fa

Added missing documentation to the system capability manager

9e888e9

Used the SDLSystemCapabilitiesManager in the Example App

9a6b805

* Used the `SDLSystemCapabilityManager` to: * check if the head unit supports graphics * check if the head unit supports making phone calls

Refactored the dial number add command in the example app

afd8335

Fixed test cases for SDLScreenManager

f137871

Refactored comments in SDLLifecycleSpec

22000a4

NicoleYarroch requested a review from joeljfischer March 29, 2018 17:03

NicoleYarroch changed the title ~~WIP: SDL 0088 System Capability Manager~~ SDL 0088 System Capability Manager Mar 29, 2018

joeljfischer suggested changes Apr 4, 2018

View reviewed changes

NicoleYarroch added 3 commits April 4, 2018 15:28

Fixed screen manager documentation in SDLManager class

5791341

Renamed variables in the SDLSystemCapabilityManager

9732ce6

Renamed var prerecordedSpeech to prerecordedSpeechCapabilities Renamed vrCapabilities to vrCapability

Deleted assert for unknown systemCapabilityType

77325e1

- Replaced with error message

NicoleYarroch added 8 commits April 4, 2018 16:18

Fixed VRCapability tests

868c739

Added permissions check for dial number in the example app

dd20365

Added the system capability manager to the SDLUpdateCapabilityHandler

6caa86c

Renamed variable

6427905

Renamed the pcmStreamCapabilities variable in the SDLSystemCapabilityManager

System capability manager now observes SDLDidReceiveGetSystemCapabili…

195c001

…tiesResponse

Refactored System Capability Manager

df20f42

Fixed the System Capability Manager test cases

6599d0d

Merge branch 'develop' into feature/issue_720_system_capability_manager

6a05711

joeljfischer suggested changes Apr 16, 2018

View reviewed changes

NicoleYarroch added 2 commits April 17, 2018 11:48

Fixed retain keyword for pcmStreamCapability

81a17b7

Signed-off-by: NicoleYarroch <nicole@livio.io>

Added a stop method to the System Capability Manager

163cbf3

Signed-off-by: NicoleYarroch <nicole@livio.io>

joeljfischer suggested changes Apr 17, 2018

View reviewed changes

Fixed stop() method of the system capability manager

0f188ae

Signed-off-by: NicoleYarroch <nicole@livio.io>

joeljfischer approved these changes Apr 18, 2018

View reviewed changes

joeljfischer merged commit 2a3fb78 into develop Apr 18, 2018

joeljfischer deleted the feature/issue_720_system_capability_manager branch April 18, 2018 20:28

theresalech mentioned this pull request Apr 19, 2018

[SDL 0088] iOS System Capability Manager #720

Closed

joeljfischer mentioned this pull request May 14, 2018

v6.0.0 Release Candidate 1 Release #959

Merged

2 tasks


		NS_ASSUME_NONNULL_BEGIN

		@interface SDLSystemCapabilityManager : NSObject

SDL 0088 System Capability Manager #916

SDL 0088 System Capability Manager #916

Conversation

NicoleYarroch commented Mar 27, 2018 • edited Loading

Risk

Testing Plan

Summary

Changelog

Breaking Changes

Bug Fixes

Tasks Remaining:

CLA

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

NicoleYarroch Apr 4, 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

NicoleYarroch Apr 4, 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

joeljfischer commented Apr 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoleYarroch commented Mar 27, 2018 •

edited

Loading

NicoleYarroch Apr 4, 2018 •

edited

Loading

NicoleYarroch Apr 4, 2018 •

edited

Loading