Implement SDL-0243 Manager Update for Display Capability #1390

kshala-ford · 2019-08-29T00:12:02Z

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Extend unit tests to confirm display capability/capabilities conversion is as expected.

Summary

This PR changes SystemCapabilityManager according to SDL-0243 to make new capabilities available and also convert DisplayCapability and DisplayCapabilities objects.

Changelog

Existing SystemCapabilityTypes are marked as deprecated hence will appear as deprecated if apps are using them.

Enhancements

Adds new capability to the manager
Adds conversion of display capability objects for developer's convenience

Tasks Remaining:

[Add DISPLAYS to manager]
[Add Conversion code]
[Do the unit tests]
[Wait for Implement SDL-0216 Widget Support #1350 to be merged before requesting review]

CLA

I have signed the CLA

…getter. Fixed bool vs. NSNumber condition. Fixed first time display backwards conversion. Fixed etst error in file manager toBeCloseTo was within 0.001 now is 0.01. Fixed a bug in menu manager test using wrong response notification name.

kshala-ford · 2019-08-29T16:58:25Z

@joeljfischer the proposal is implemented and pretty much ready for review. Awaiting merge of #1350 so that I can pull latest develop and mark review ready

…issue_1386_manager_update_display_capability

kshala-ford · 2019-09-04T10:42:44Z

@joeljfischer I just merged the latest develop into my branch and fixed some merge conflicts. The PR is now ready for review.

joeljfischer

I think that portions of this PR are missing, and tests do not build and have numerous warnings.

SmartDeviceLink/SDLSystemCapability.m

SmartDeviceLink/SDLSystemCapabilityManager.h

…issue_1386_manager_update_display_capability # Conflicts: # SmartDeviceLink/SDLSystemCapability.h # SmartDeviceLink/SDLSystemCapability.m

kshala-ford · 2019-09-06T09:45:49Z

@joeljfischer Sorry... Something went wrong with merging develop. The conflicts were not resolved correctly and I accidentally deleted some code like the windowCapabilityForWindowID... The PR should now be correct and up-to-date (merged latest develop again).

joeljfischer

Mostly minor stuff

SmartDeviceLink/SDLSystemCapability.h

SmartDeviceLink/SDLSystemCapability.m

SmartDeviceLink/SDLSystemCapabilityManager.h

SmartDeviceLink/SDLSystemCapabilityManager.m

SmartDeviceLinkTests/SDLSystemCapabilityManagerSpec.m

SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

…ions. Renamed convert flag. Created window and display capability group. Changed media clock to empty array. Changed repeat test to unique data. Other minor suggestions applied.

joeljfischer

Additionally, sdl_systemCapabilityTypes needs to be updated with DISPLAYS in order for subscriptions to work properly.

joeljfischer · 2019-09-11T14:33:08Z

SmartDeviceLink/SDLSystemCapabilityManager.m

    if (!response.success.boolValue) { return; }

+    self.displays = [self sdl_createDisplayCapabilityListFromSetDisplayLayoutResponse:response];


I think it's kind of odd that we'd be overwriting the displays information and then immediately overwriting it again with the OnSystemCapabilityUpdated information. IMO, this backward compatibility shouldn't occur if we're connected to a head unit that supports OnSystemCapabilityUpdated for display information.

This would be conflicting to what was accepted with the proposal:

If the application is connected to a < 6.0 or if the head unit did not provide DisplayCapability or WindowCapability for the DEFAULT_WINDOW yet, the system capability manager should convert DisplayCapabilities, ButtonCapabilities, SoftButtonCapabilities and PresetBankCapabilities objects and the string of displayName from RegisterAppInterfaceResponse and SetDisplayLayoutResponse into a new DisplayCapability object. This object should be stored in the DISPLAYS enum value.

Sorry, I need to clarify. I mean that if you're connected to a 6.0+ head unit, then the displays data should not be overwritten by the deprecated data. On a < 6.0 head unit, as you quote, then it should pull the deprecated data. Currently, the code overwrites the OnSystemCapabilityUpdated.displayCapability data with the deprecated SetDisplayLayoutResponse data.

If the application is connected to a < 6.0 or if the head unit did not provide DisplayCapability or WindowCapability for the DEFAULT_WINDOW yet

That should work

BrettyWhite · 2019-09-12T16:54:17Z

@kshala-ford Please also ensure the sub-managers are updated to use the new capabilities in this PR, similar to what i mentioned in the JavaSuite equivalent. Thank you

kshala-ford · 2019-09-13T19:11:30Z

@joeljfischer I made a bunch of changes so that the new capability type is used instead of deprecated.

joeljfischer

Mostly small stuff. The one that still concerns me the most is ensuring that updates are not sent on 6.0+ system when we've already received an update.

SmartDeviceLink/SDLWindowCapability+ShowManagerExtensions.m

joeljfischer · 2019-09-16T15:25:05Z

SmartDeviceLink/SDLWindowCapability+ShowManagerExtensions.m

            if (highestFound == 4) { break; }
        }
    }
-
+    


Suggested change

fixed in 8fac298

SmartDeviceLink/SDLWindowCapability+ShowManagerExtensions.m

SmartDeviceLink/SDLScreenManager.m

SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m

SmartDeviceLink/SDLSystemCapabilityManager.m

joeljfischer · 2019-09-16T18:32:57Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+
+    // call the observers in case the new display capability list is created from deprecated types
+    SDLSystemCapability *systemCapability = [[SDLSystemCapability alloc] initWithDisplayCapabilities:self.displays];
+    [self sdl_callSaveHandlerForCapability:systemCapability andReturnWithValue:YES handler:nil];


This doesn't appear to be the same as the agreed upon implementation. This shouldn't call the observers when a SetDisplayLayoutResponse is recorded unless we're on a < 6.0 system or no displayCapability has been provided through the notification. This is just always calling the observers.

joeljfischer · 2019-09-16T18:58:52Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+    return convertedCapabilities;
+}
+
+- (void)sdl_updateDeprecatedDisplayCapabilities {


It would be very helpful if you could add documentation comments to new private methods here. I keep forgetting what methods do exactly and have to re-read the code to figure it out.

joeljfischer · 2019-09-16T19:19:38Z

SmartDeviceLink/SDLSystemCapabilityManager.m

+    SDLDisplayCapability *newDefaultDisplayCapabilities = newCapabilities.firstObject;
+    NSArray<SDLWindowCapability *> *newWindowCapabilities = newDefaultDisplayCapabilities.windowCapabilities;
+
+    for (SDLWindowCapability *newWindow in newWindowCapabilities) {


Please add a comment or two to this for loop to describe what's going on here for future reference

joeljfischer

A few more important issues after testing against Core 6.0 in-progress.

joeljfischer · 2019-09-17T14:21:50Z

SmartDeviceLink/SDLSoftButtonManager.m

@@ -65,8 +66,7 @@ - (instancetype)initWithConnectionManager:(id<SDLConnectionManagerType>)connecti
    _transactionQueue = [self sdl_newTransactionQueue];
    _batchQueue = [NSMutableArray array];

-    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(sdl_registerResponse:) name:SDLDidReceiveRegisterAppInterfaceResponse object:nil];
-    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(sdl_displayLayoutResponse:) name:SDLDidReceiveSetDisplayLayoutResponse object:nil];
+    [_systemCapabilityManager subscribeToCapabilityType:SDLSystemCapabilityTypeDisplays withObserver:self selector:@selector(sdl_displayCapabilityUpdate:)];


This always fails because the SystemCapabilityManager.isStreamingSupported check in SystemCapabilityManager.subscribeToCapabilityType fails. Two solutions I can think of:

This check always succeeds if checking DISPLAYS

This check is removed entirely

I think I lean toward (1) right now. What do you think?

It is implemented as:

[_systemCapabilityManager subscribeToCapabilityType:SDLSystemCapabilityTypeDisplays withObserver:self selector:@selector(sdl_displayCapabilityUpdate:)]; [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(sdl_hmiStatusNotification:) name:SDLDidChangeHMIStatusNotification object:nil];

joeljfischer · 2019-09-17T14:34:43Z

SmartDeviceLink/SDLSoftButtonManager.m

-
+- (void)sdl_displayCapabilityUpdate:(SDLSystemCapability *)systemCapability {
+    // we won't use the object in the parameter but the convenience method of the system capability manager
+    self.defaultMainWindowCapability = _systemCapabilityManager.defaultMainWindowCapability;


I don't think there's a reason to use the getter from the systemCapabilityManager, the systemCapability that's passed should be equivalent because the handler here is only called after the merge finishes.

fixed in 89d623d
though funny it looks

…issue_1386_manager_update_display_capability # Conflicts resolved: # SmartDeviceLink/SDLSystemCapabilityManager.m # SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m # SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m

smartdevicelink#1390 (comment)

smartdevicelink#1390 (comment) smartdevicelink#1390 (comment)

smartdevicelink#1390 (comment) smartdevicelink#1390 (comment) smartdevicelink#1390 (comment) smartdevicelink#1390 (comment)

smartdevicelink#1390 (comment)

theresalech · 2019-09-30T13:50:43Z

@kshala-ford @yLeonid can you please confirm if this PR should be closed in favor of #1419?

yLeonid · 2019-09-30T14:04:23Z

@theresalech #1419 contains all changes from this branch and also it has newer commits. So yes I confirm this PR can be closed

kshala-ford added 4 commits August 29, 2019 02:08

Adding logic for display capabilities conversion.

625189e

All unit tests.

770bb99

Fixing a potential case with nil display capability.

f1710f9

kshala-ford changed the title ~~[WIP] Implement SDL-0243 Manager Update for Display Capability~~ Implement SDL-0243 Manager Update for Display Capability Aug 29, 2019

joeljfischer added this to In progress in v6.4 via automation Aug 30, 2019

joeljfischer added enhancement proposal Accepted SDL Evolution Proposal labels Aug 30, 2019

joeljfischer added this to the 6.4.0 milestone Aug 30, 2019

kshala-ford added 3 commits August 31, 2019 11:35

Update SDLMenuManagerSpec.m

cc470c5

Merge commit 'd433cf5ae5cb04d0ede46e2cfbd1dea105698891' into feature/…

ca5b403

…issue_1386_manager_update_display_capability

Resolved some issues from the merge conflicts.

471e095

joeljfischer self-requested a review September 4, 2019 14:28

joeljfischer suggested changes Sep 4, 2019

View reviewed changes

SmartDeviceLink/SDLSystemCapability.m Show resolved Hide resolved

SmartDeviceLink/SDLSystemCapability.m Show resolved Hide resolved

SmartDeviceLink/SDLSystemCapabilityManager.h Show resolved Hide resolved

v6.4 automation moved this from In progress to Review in progress Sep 4, 2019

kshala-ford added 2 commits September 6, 2019 11:28

Fixed a conflicting merge conflict conflict ...

7e33cfd

Merge commit '8b783865896daa67a55b78d2ce9ff4127d21b0f8' into feature/…

5af489c

…issue_1386_manager_update_display_capability # Conflicts: # SmartDeviceLink/SDLSystemCapability.h # SmartDeviceLink/SDLSystemCapability.m

joeljfischer suggested changes Sep 6, 2019

View reviewed changes

kshala-ford and others added 2 commits September 9, 2019 16:44

Apply suggestions from code review

bf727a4

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

Change defaultMainWindowCapability to property. Ignored some deprecat…

f265d4f

…ions. Renamed convert flag. Created window and display capability group. Changed media clock to empty array. Changed repeat test to unique data. Other minor suggestions applied.

joeljfischer suggested changes Sep 11, 2019

View reviewed changes

Refactored the library to use the new display capability

ce156b1

joeljfischer suggested changes Sep 16, 2019

View reviewed changes

joeljfischer suggested changes Sep 17, 2019

View reviewed changes

yLeonid mentioned this pull request Sep 29, 2019

Feature/issue 1386 manager update display capability kshala-ford/sdl_ios#1

Open

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 29, 2019

issue_1386_manager_update_display_capability : fix review comment

7c2c526

smartdevicelink#1390 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 29, 2019

issue_1386_manager_update_display_capability : fix review comment

03cafb4

smartdevicelink#1390 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 29, 2019

issue_1386_manager_update_display_capability : fix review comment

cc01fee

smartdevicelink#1390 (comment) smartdevicelink#1390 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 29, 2019

issue_1386_manager_update_display_capability : fix code review comments

8fac298

smartdevicelink#1390 (comment) smartdevicelink#1390 (comment) smartdevicelink#1390 (comment) smartdevicelink#1390 (comment)

yLeonid approved these changes Sep 29, 2019

View reviewed changes

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 29, 2019

issue_1386_manager_update_display_capability : fix code review comment:

89d623d

smartdevicelink#1390 (comment)

joeljfischer closed this Sep 30, 2019

v6.4 automation moved this from Review in progress to Done Sep 30, 2019

yLeonid mentioned this pull request Sep 30, 2019

Implement SDL-0243 Manager Update for Display Capability #1419

Closed

5 tasks

joeljfischer mentioned this pull request Oct 2, 2019

Implement SDL-0243 Manager Update for DisplayCapability #1427

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SDL-0243 Manager Update for Display Capability #1390

Implement SDL-0243 Manager Update for Display Capability #1390

kshala-ford commented Aug 29, 2019 •

edited

kshala-ford commented Aug 29, 2019

kshala-ford commented Sep 4, 2019

joeljfischer left a comment

kshala-ford commented Sep 6, 2019

joeljfischer left a comment

joeljfischer left a comment •

edited

joeljfischer Sep 11, 2019

kshala-ford Sep 13, 2019

joeljfischer Sep 13, 2019

kshala-ford Sep 13, 2019

joeljfischer Sep 13, 2019

BrettyWhite commented Sep 12, 2019

kshala-ford commented Sep 13, 2019

joeljfischer left a comment

joeljfischer Sep 16, 2019

yLeonid Sep 29, 2019

joeljfischer Sep 16, 2019

joeljfischer Sep 16, 2019

joeljfischer Sep 16, 2019

joeljfischer left a comment

joeljfischer Sep 17, 2019

yLeonid Oct 1, 2019

joeljfischer Sep 17, 2019

yLeonid Sep 29, 2019

theresalech commented Sep 30, 2019

yLeonid commented Sep 30, 2019

		if (!response.success.boolValue) { return; }

		self.displays = [self sdl_createDisplayCapabilityListFromSetDisplayLayoutResponse:response];

Implement SDL-0243 Manager Update for Display Capability #1390

Implement SDL-0243 Manager Update for Display Capability #1390

Conversation

kshala-ford commented Aug 29, 2019 • edited

Risk

Testing Plan

Summary

Changelog

Enhancements

Tasks Remaining:

CLA

kshala-ford commented Aug 29, 2019

kshala-ford commented Sep 4, 2019

joeljfischer left a comment

Choose a reason for hiding this comment

kshala-ford commented Sep 6, 2019

joeljfischer left a comment

Choose a reason for hiding this comment

joeljfischer left a comment • edited

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

BrettyWhite commented Sep 12, 2019

kshala-ford commented Sep 13, 2019

joeljfischer left a 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

joeljfischer left a 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

theresalech commented Sep 30, 2019

yLeonid commented Sep 30, 2019

kshala-ford commented Aug 29, 2019 •

edited

joeljfischer left a comment •

edited