Implement SDL 0157 - Choice Set Manager #975

joeljfischer · 2018-05-21T20:41:49Z

Fixes #970

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Unit tests
Smoke tests

Summary

Implement SDL-0157, a choice set manager making implementing interactions (choice sets and keyboards) much easier.

Changelog

Enhancements

Implement a choice set manager, enabling much easier interactions (choice sets and keyboards). [SDL-0157] ([SDL 0157] Mobile Choice Set Manager #970)

Tasks Remaining:

Implement the manager
Unit tests

CLA

I have signed the CLA

* Update `deleteChoices:` to `deleteChoices:andAttachedImages:`

* Add a state machine to the choice set manager * Add checking voice optional

* Add ScreenManager delegate for checking errors

* ChoiceManager code organization

* Add missing properties to ChoiceSetManager * First pass at deleting choice cells

* In-progress

# Conflicts: # SmartDeviceLink/SDLScreenManager.h

* Rework error information and completion handler information from operations

* Updating present choice set operation

* Updates to ChoiceSetManager * Fix CheckChoiceVROptionalOperation not working

* Fixes to check choice VR optional operation

* Fix crash when preloading choices without artworks * Update log modules to split screen manager into separate modules for easier filtering * Add new SDLChoiceSet initializer for easier TTS creation * Update Swift example app to use new ChoiceSetManager

* Fix preloading without artworks not properly returning * Fix choices without images not properly creating the Choice * Fix presentation errors not properly returning

* Add description methods to SDLChoiceCell and SDLChoiceSet

* Fix not setting screen manager keyboard configuration * Documentation

* All keyboard presentations now set keyboard properties before hand. It simplifies logic and makes sure we have the correct properties available

* PreloadChoiceCompletionHandler error should be nullable * Fix name of ChoiceSetManager transaction queue * Don’t attempt to upload choices if the choices are uploaded or pending * Add uploaded choices to the uploaded choices and remove from the pending upload choices * Make sure presented choice sets have correct choice ids

* In progress delete tests

* PresentChoiceSetOperation now correctly resets the keyboard only when using a searchable choice set

* Choice set manager removes deleted cells from in progress preloads * Choice set manager tests

NicoleYarroch · 2018-06-18T18:57:16Z

SmartDeviceLink/SDLScreenManager.h

+
+ @param error The error if one occurred
+ */
+typedef void(^SDLPreloadChoiceCompletionHandler)(NSError *error);


The returned error in the SDLPreloadChoiceCompletionHandler should be nullable.
Also, there appears to be a duplicate of SDLPreloadChoiceCompletionHandler in the SDLChoiceSetManager class.

NicoleYarroch · 2018-06-19T13:47:20Z

SmartDeviceLink/SDLChoiceSet.m

+    if (choiceVoiceCommandSet.count < nonNilVoiceCommands.count) {
+        SDLLogW(@"Attempted to create a choice set with duplicate voice commands. Voice commands must be unique.");
+        return nil;
+    }


I think it would be better if the data validation was done in the SDLChoiceSetManager's preloadChoices:withCompletionHandler method. Currently:

If the SDLChoiceCell is null, the only way to get the error is to look at the logs.

It is still possible to send invalid data if setting each of params individually.

If data validation is done in the preloadChoices:withCompletionHandler method, the custom error can be passed back via the handler.

We shouldn't do it in preloadChoices:withCompletionHandler: because we can upload multiple choices with the same primary text, but we just can't display a choice set with 2 that have the same text. I believe it's the same for voice commands.

True, it is the most efficient way to validate the choice sets since the manager has 3 choice set upload methods.

NicoleYarroch · 2018-06-19T15:00:52Z

SmartDeviceLink/SDLChoiceCell.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface SDLChoiceCell: NSObject


It would be handy to have a cellSelectedHandler on the SDLChoiceCell. That way, when the SDLChoiceSetDelegate's choiceSet:didSelectChoice:withSource is called, the handler can be triggered. Otherwise, I have to check the cell's text and figure out what I'm going to do based on that... which can get messy.

The problem with blocks is that we'd have to store all of them...and there could be a lot. For now, I've added a row index to the delegate callback, which should be sufficient. If we want to go to blocks, we can do so in a later release.

NicoleYarroch · 2018-06-19T15:14:36Z

SmartDeviceLink/SDLCheckChoiceVROptionalOperation.m

+    SDLChoice *choice = [[SDLChoice alloc] init];
+    choice.choiceID = @0;
+    choice.menuName = @"Test Cell";
+    choice.vrCommands = hasVR ? @[@"Test VR"] : nil;


This shows up on Manticore's Voice tab. Could be confusing for developers.

Unfortunately, that's unavoidable. This has to have a VR command to be valid, and it's used for the keyboard.

NicoleYarroch

I left a few comments and enhancement requests.

NicoleYarroch · 2018-06-25T14:33:53Z

SmartDeviceLink_Example/PerformInteractionManager.m

+
+#pragma mark - SDLChoiceSetDelegate
+
+- (void)choiceSet:(SDLChoiceSet *)choiceSet didSelectChoice:(SDLChoiceCell *)choice withSource:(SDLTriggerSource)source {


This method needs to be updated to include the new rowIndex param. App crashes.

NicoleYarroch

The Obj-C example app SDLChoiceSetDelegate methods need to be updated.

joeljfischer added 2 commits May 21, 2018 16:16

Stub out new choice set manager classes

e8323e1

Update documentation

8bf60fa

* Update `deleteChoices:` to `deleteChoices:andAttachedImages:`

joeljfischer added enhancement proposal Accepted SDL Evolution Proposal labels May 21, 2018

joeljfischer added this to the 6.1.0 milestone May 21, 2018

joeljfischer self-assigned this May 21, 2018

joeljfischer requested a review from NicoleYarroch May 21, 2018 20:41

joeljfischer added 23 commits May 22, 2018 10:19

Setting up internals of choice set manager

46ac2b7

Choice set manager state machine additions

02617d1

* Add a state machine to the choice set manager * Add checking voice optional

* Add ScreenManager convenience endUpdates

4efba03

* Add ScreenManager delegate for checking errors

* Add ChoiceSetManager to internal log file module map

9bc4833

* ChoiceManager code organization

Implement ScreenManager Methods / Properties as passthrough

9c6d211

* Add missing properties to ChoiceSetManager * First pass at deleting choice cells

Rejigger ChoiceSetManager to use operations

6b85bc6

* In-progress

Add new ChoiceManager operations

5690738

Remove unused code

1685ffc

Add names for choice set operations

a7b8e94

Merge branch 'develop' into feature/issue_970_mobile_choice_manager

d0ccfcc

# Conflicts: # SmartDeviceLink/SDLScreenManager.h

Working on presenting keyboard delegates

f4a5d3f

* Rework error information and completion handler information from operations

Add choice set operations to log module map

217f63d

* Updating present choice set operation

Updates to PresentChoiceSetOperation

8cf5116

* Updates to ChoiceSetManager * Fix CheckChoiceVROptionalOperation not working

Startup the choice set manager correctly

f8cce42

* Fixes to check choice VR optional operation

* Fix Choice ids not being set properly

fb485f2

* Fix preloading without artworks not properly returning * Fix choices without images not properly creating the Choice * Fix presentation errors not properly returning

Fix preloaded choice sets having non-matching ids

2b33f9f

* Add description methods to SDLChoiceCell and SDLChoiceSet

Fix crashes when optional keyboard delegate methods aren’t present

c630ea6

* Fix not setting screen manager keyboard configuration * Documentation

Fix default keyboard properties

444d02a

* All keyboard presentations now set keyboard properties before hand. It simplifies logic and makes sure we have the correct properties available

Fix setting keyboard configuration

a8910ce

Use a searchable choice set in the example app

e5bd3d0

Merge branch 'develop' into feature/issue_970_mobile_choice_manager

79d5891

joeljfischer added 15 commits June 8, 2018 16:20

Stub the rest of the specs

6c35b87

CheckChoiceVROperation tests

ef0b79f

* In progress delete tests

Delete choices operation tests

ca1477f

Preload choice operation tests

c8bdf93

Fixes to preload choices operation tests

32394ee

Present keyboard tests

ef4dd7b

Stub present choice set tests

470c320

Additional small updates

650f235

Present Choice Set Operation tests

c47a662

* PresentChoiceSetOperation now correctly resets the keyboard only when using a searchable choice set

In progress choice set manager tests

133faf4

Preloading choice operations can now have cells removed

9e316bf

* Choice set manager removes deleted cells from in progress preloads * Choice set manager tests

Additional ChoiceSetManager tests

786eedf

Finish choice set manager tests

0ef0b3a

Test fixes

7092424

Fix remaining test cases

607f7ce

joeljfischer changed the title ~~WIP: Implement SDL 0157 - Choice Set Manager~~ Implement SDL 0157 - Choice Set Manager Jun 15, 2018

joeljfischer added 2 commits June 15, 2018 09:24

Remove unused delegate

a664564

Update obj-c example app for choicesetmanager

bfe4754

NicoleYarroch reviewed Jun 18, 2018

View reviewed changes

NicoleYarroch reviewed Jun 19, 2018

View reviewed changes

NicoleYarroch suggested changes Jun 19, 2018

View reviewed changes

Add row index to choice set delegate

1b046de

NicoleYarroch reviewed Jun 25, 2018

View reviewed changes

NicoleYarroch suggested changes Jun 25, 2018

View reviewed changes

Fix obj-c app using incorrect delegate

aa39378

NicoleYarroch approved these changes Jun 25, 2018

View reviewed changes

joeljfischer merged commit aa39378 into develop Jun 27, 2018

joeljfischer deleted the feature/issue_970_mobile_choice_manager branch June 27, 2018 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SDL 0157 - Choice Set Manager #975

Implement SDL 0157 - Choice Set Manager #975

joeljfischer commented May 21, 2018 •

edited

Loading

NicoleYarroch Jun 18, 2018

joeljfischer Jun 22, 2018

NicoleYarroch Jun 19, 2018

joeljfischer Jun 22, 2018

NicoleYarroch Jun 25, 2018

NicoleYarroch Jun 19, 2018

joeljfischer Jun 22, 2018

NicoleYarroch Jun 19, 2018

joeljfischer Jun 22, 2018

NicoleYarroch left a comment

NicoleYarroch Jun 25, 2018

joeljfischer Jun 25, 2018

NicoleYarroch left a comment


		#pragma mark - SDLChoiceSetDelegate

		- (void)choiceSet:(SDLChoiceSet )choiceSet didSelectChoice:(SDLChoiceCell )choice withSource:(SDLTriggerSource)source {


		NS_ASSUME_NONNULL_BEGIN

		@interface SDLChoiceCell: NSObject

Implement SDL 0157 - Choice Set Manager #975

Implement SDL 0157 - Choice Set Manager #975

Conversation

joeljfischer commented May 21, 2018 • edited Loading

Risk

Testing Plan

Summary

Changelog

Enhancements

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

NicoleYarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoleYarroch left a comment

Choose a reason for hiding this comment

joeljfischer commented May 21, 2018 •

edited

Loading