Skip to content
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

[Returned for Revisions] SDL 0157 - Mobile Choice Set Manager #449

Closed
theresalech opened this issue Apr 4, 2018 · 19 comments
Closed

[Returned for Revisions] SDL 0157 - Mobile Choice Set Manager #449

theresalech opened this issue Apr 4, 2018 · 19 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Apr 4, 2018

Hello SDL community,

The review of "Mobile Choice Set Manager" begins now and runs through April 10, 2018. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/06c1ebb006603400dc6b022f2248d10442531a39/proposals/0157-mobile-choice-manager.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#449

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
[email protected]

@kshala-ford
Copy link
Contributor

kshala-ford commented Apr 9, 2018

Thank you @joeljfischer for this proposal. I took a depth look into your proposal and overall I agree to adding choiceset management. Below are some comments:

SDLChoiceCell

  • Does the name SDLChoiceCell fit for voice interactions? Sounds unfamiliar but I believe you want to follow the menu manager cells and UITableviewCell? What about SDLChoiceItem?
  • SDLChoiceCell.subText should be .subtext
  • Should SDLChoiceCell.voiceCommands be .voiceRecognitionCommands? Just following naming from other managers...
  • Not sure if it's a good idea to have every property readonly... Any reason for that? What initializers are you planning to add?

Choice Set (or Interaction)

  • I think the name SDLInteraction makes much more sense than SDLChoiceSetObject
  • What's the purpose of the different prompt names? Rename .voicePrompt to .initialPrompt? Same for help and timeout prompt?
  • The type for .layout should be SDLLayoutMode
  • I guess the type of .choices should be NSArray<SDLChoiceCell *> *?
  • What about using NSOrderedSet<SDLChoiceCell *> *? It's not important though... just an idea...

SDLChoiceSetDelegate (or Interaction delegate)

  • When renaming above this delegate should be called SDLInteractionDelegate
  • - (void)choiceSet:(SDLChoiceSet *)choiceSet ... should be changed if renaming happens.
  • AFAIK the trigger source for keyboard is always .KEYBOARD.
    • Change ...didReceiveText:(NSString *)text fromSource:(SDLTriggerSource)source; to
    • ...didReceiveText:(NSString *)text;
  • What's didReceiveText: doing? Provide the text at the end of the interaction? Does it repeat the current entry? Did you consider other keyboard modes?
  • I have a bad feeling with - (BOOL)shouldDeleteChoiceSet;. I'll try to explain later in a "Caching" section. Don't think the delegate needs it

Screen Manager Additions

I think the design is not ideal yet. I'm struggling a bit with how choice sets and interactions are merged. It looks like parameters like initialPrompt are part of the caching. Below are more questions:

  • Is it correct that the choice set object and the delegate must remain for the whole lifetime of the app?
  • What if the delegate object does not exist anymore?
  • Can is pre-upload dynamic choice sets?
  • Can I delete choice sets without performing it a second time?
  • Can I modify interaction related parameters for a static choice set?
  • What of the methods are blocking or non-blocking?

Instead of the tree methods I think of the following (using proposed class names) (moving out delegate from the interaction class as it's weak):

Note: The methods below without dynamic parameter are shortened versions.

@interface SDLScreenManager
...
/**
 * Presents an interaction on the head unit using the interaction and mode provided. 
 * The results of the interaction are forwarded to the provided delegate.
 * The choice set of the interaction will be automatically uploaded prior to the interaction
 * in as a static set if items of the choice set are not uploaded yet.
 * The method is non-blocking.
 */
- (void)presentInteraction:(SDLInteraction *)interaction mode:(SDLInteractionMode)mode delegate:(id<SDLChoiceSetDelegate>)delegate;

/**
 * Uploads a choice set to the head unit as a static set.
 * The completion handler provides information if the upload failed.
 * The method is non-blocking.
 */
- (void)uploadChoiceSet:(NSArray<SDLChoiceItem *> *)set completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Uploads a choice set to the head unit depending on the dynamic parameter.
 * The choice set will be dynamic if `.dynamic` is YES. Otherwise the choice set is static.
 * The completion handler provides information if the upload failed.
 * The method is non-blocking.
 */
- (void)uploadChoiceSet:(NSArray<SDLChoiceItem *> *)set dynamic:(BOOL)dynamic completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Checks if the head unit already contains the provided static choice set.
 * The method is blocking but returns immediately without remote communication.
 */
- (BOOL)containsChoiceSet:(NSArray<SDLChoiceItem *> *)set;

/**
 * Checks if the head unit already contains the provided choice set.
 * If dynamic is YES the provided choice set is treated as dynamically created. Otherwise it's treated as static.
 * The method is blocking but returns immediately without remote communication.
 */
- (BOOL)containsChoiceSet:(NSArray<SDLChoiceItem *> *)set dynamic:(BOOL)dynamic;

/**
 * Deletes the provided static choice set.
 * The method is non-blocking
 */
- (void)deleteChoiceSet:(NSArray<SDLChoiceItem *> *)set completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Deletes the provided choice set.
 * If dynamic is YES the provided choice set is treated as dynamically created. Otherwise it's treated as static.
 * The method is non-blocking
 */
- (void)deleteChoiceSet:(NSArray<SDLChoiceItem *> *)set dynamic:(BOOL)dynamic completionHandler:(void (^)(NSError *error))completionHandler;
...
@end

Again I don't think the delegate protocol needs another way to delete a choice set.

Potential downside

This API also does not consider the Keyboard use-case. Creating and using a keyboard seems shoehorned into the ChoiceSet API by the author. Therefore, a separate manager / high-level API should be created specifically for the keyboard use-case. Additionally, the keyboard cannot be used by non-NAVIGATION developers and therefore has a much smaller audience.

I disagree. Keyboards make sense to be used in combination with choices. POI apps and media apps can and are using the keyboard to search for ... parking or albums. The choices shown in the keyboard interaction are suggestions or history/previous search text/result. Keyboard support should be considered. It's almost there though with the extra delegate method.

@joeljfischer
Copy link
Contributor

Hey Kujtim, I appreciate the review.

SDLChoiceCell

Does the name SDLChoiceCell fit for voice interactions? Sounds unfamiliar but I believe you want to follow the menu manager cells and UITableviewCell? What about SDLChoiceItem?

Those are indeed one of the main reasons for using the cell nomenclature. I think cell still works; after all, the cell requires (and in the future, only may) have a voice command but also requires cell text. Since it is actually a cell that appears on screen, I believe calling it a cell is apt.

SDLChoiceCell.subText should be .subtext

Perhaps detailText would actually be better to fit with UITableViewCell.

Should SDLChoiceCell.voiceCommands be .voiceRecognitionCommands? Just following naming from other managers...

Actually, voiceCommands matches the SDLMenuCell naming. I think adding "recognition" is unnecessary in this case.

Not sure if it's a good idea to have every property readonly... Any reason for that? What initializers are you planning to add?

The primary reason is so that developers don't try to modify them after they're sent or while it's in the process of being sent, so safety primarily. As far as initializers go, there would be an initializer for every property. I'm not sure if others would be added; I considered it an implementation detail.

SDLChoiceSetObject

I think the name SDLInteraction makes much more sense than SDLChoiceSetObject

It's certainly an option, but I don't know that Interaction is clear or specific enough for a developer to understand what it is without documentation. It's not as clear that it contains choices. We could do a ChoiceArray or OrderedChoiceSet or something like that.

What's the purpose of the different prompt names? Rename .voicePrompt to .initialPrompt? Same for help and timeout prompt?

It was primarily for clarity that these are voice / spoken prompts, not text (though the type may make that clear). The naming could be reverted to the same as is in PerformInteraction.

The type for .layout should be SDLLayoutMode
I guess the type of .choices should be NSArray<SDLChoiceCell *> *?

Yup

What about using NSOrderedSet<SDLChoiceCell *> *? It's not important though... just an idea...

I had to think about this one a bit. The two primary reasons I think we should stay with NSArray are (1) syntactic sugar of @[], and (2) the need to either (a) compare all the properties on ChoiceCell for equality, or (b) Add a unique property the dev would have to set in order to verify uniqueness (having a generated one would defeat the purpose of not using NSArray).

SDLChoiceSetDelegate (or Interaction delegate)

When renaming above this delegate should be called SDLInteractionDelegate
- (void)choiceSet:(SDLChoiceSet *)choiceSet ... should be changed if renaming happens.

See above reasons for remaining with ChoiceSetObject naming.

AFAIK the trigger source for keyboard is always .KEYBOARD.

Yup, those changes should be made.

What's didReceiveText: doing? Provide the text at the end of the interaction? Does it repeat the current entry? Did you consider other keyboard modes?

Yes, providing the text typed in for a withSearch layout (from the Perform Interaction Response) without having to observe the OnKeyboardInput notification, which is more useful for the navigation keyboard.

Is it correct that the choice set object and the delegate must remain for the whole lifetime of the app?

Unless the choice set is deleted, certainly. I imagine an app will either have one delegate for all their choice sets, or will have one per choice set (like a UITableViewDelegate). I'm trying to think about how this could work with the ChoiceSetManager. I would think the developer would want a UITableView-like API with the delegate in it's own UITableViewController-like file. I noticed that the screen manager does not currently expose the uploaded choice sets. Perhaps they should remain available in a dictionary so that the UI manager can set the delegate before it's presented?

@property (copy, nonatomic, readonly) NSDictionary<NSString *, SDLChoiceSetObject *> *availableChoiceSets, where the string is the title of the SDLChoiceSetObject. The delegate would also need to be writable so that it could be set when needed. Hopefully that helps with the below questions as well.

What if the delegate object does not exist anymore?

That would certainly be a problem, but a common one.

Can is pre-upload dynamic choice sets?

No, the idea of a dynamic choice set is that it's data is based on the time it was pressed (think of a recents list for a media app). The developer could, of course, send a static one, show it, then delete it.

Can I delete choice sets without performing it a second time?

That would be a good addition.

Can I modify interaction related parameters for a static choice set?

Not currently in this design. It would be good to do so, however. Perhaps those SDLChoiceSetObject parameters should be writable.

What of the methods are blocking or non-blocking?

None block. If present is called before it finishes uploading, it will present immediately after it finishes uploading.

Potential downside

The keyboard is supported for retrieving searched info (i.e. LIST WITH SEARCH / ICON WITH SEARCH), but actually presenting a raw keyboard with autocomplete, etc. (i.e. LayoutMode is KEYBOARD) is AFAIK restricted to navigation apps only.

I would like to note you mention an issue with caching a few times but never describe it.

@kshala-ford
Copy link
Contributor

kshala-ford commented Apr 10, 2018

SDLChoiceCell

Those are indeed one of the main reasons for using the cell nomenclature. I think cell still works; after all, the cell requires (and in the future, only may) have a voice command but also requires cell text. Since it is actually a cell that appears on screen, I believe calling it a cell is apt.

OK. Let's stay with SDLChoiceCell

Perhaps detailText would actually be better to fit with UITableViewCell.

I'm not OK with .detailText. Looking again at the three text properties I'm not OK to any of those. For SYNC3 interactions in list mode use:

  • Choice.menuName like UITableViewCell.textLabel
  • Choice.secondaryText like UITableViewCell.detailedTextLabel
  • Choice.tertiaryText like a right hand side text label for e.g. distances. No equivalent label in UIKit

I googled for an example image and found this. It's not an app driven interaction but the cells looks very similar:
untitled

I would do the following mapping:

  • Choice.menuName to SDLChoiceCell.text
  • Choice.secondaryText to SDLChoiceCell.secondaryText
  • Choice.tertiaryText to SDLChoiceCell.tertiaryText

The reason for that is simple. SDL should not come up with meanings for text fields so the So OEMs can design the HMI more flexible. When we speak of a title we expect a text field with bigger font on top of everything else. I want to avoid that because .menuName doesn't necessarily mean to be a title. But to give a specific reason for my proposed naming: to a right hand text field the parameter name tertiaryText fits more than detailedText.

Actually, voiceCommands matches the SDLMenuCell naming. I think adding "recognition" is unnecessary in this case.

I agree it's unnecessary but if you check this search in sdl_ios there are a few results for voiceRecognition. I just want to make this visible and point to the potential difference. It's a detail... may be not worth mention it but I would appreciate consistent writing. I would prefer voiceRecognition throughout the library.

The primary reason is so that developers don't try to modify them after they're sent or while it's in the process of being sent, so safety primarily. As far as initializers go, there would be an initializer for every property. I'm not sure if others would be added; I considered it an implementation detail.

Why don't we copy the choice set when passed to the SDL library? We should anyway do that so modifications don't affect our copy. I would add two initializers to the choice cell. One for the mandatory parameters only and one for all parameters. Still I don't want to readonly when it's not needed.

SDLChoiceSetObject

It's certainly an option, but I don't know that Interaction is clear or specific enough for a developer to understand what it is without documentation. It's not as clear that it contains choices. We could do a ChoiceArray or OrderedChoiceSet or something like that.

I don't think so. I think the text "interact with the driver using a set of choices or a keyboard using SDLInteraction" is clear and easy to understand. The property .choices is a typed array so the developer will easily figure it out. Leaving the name as is makes it more confusing for keyboard use. SDLChoiceSetObject doesn't sound intuitive at all and it mixes interaction parameters which are not cached... it's going to cause confusion. The class should be called SDLInteraction.

Use NSOrderedSet ... ...

I'm OK with that but I want to note that for caching we need to compare cells already uploaded and requested to be uploaded.

SDLChoiceSetDelegate (or Interaction delegate)

Again I see SDLInteractionDelegate as a better name.

Yes, providing the text typed in for a withSearch layout (from the Perform Interaction Response) without having to observe the OnKeyboardInput notification, which is more useful for the navigation keyboard.

It should include OnKeyboardInput. To be clear: The keyboard can be used by any app. Media, POI, NAV etc.

Unless the choice set is deleted, certainly. I imagine an app will either have one delegate for all their choice sets, or will have one per choice set (like a UITableViewDelegate). .... ....

The table view delegate remains alive only during the lifecycle of the table view controller. View controller can be destroyed together with the delegate but the table content still exists, stored somewhere.

What if the delegate object does not exist anymore?

That would certainly be a problem, but a common one.

We should avoid that by asking for a delegate when the interaction starts. I don't want a weak reference for the time the choice set is stored on the head unit.

Can I pre-upload dynamic choice sets?

No, the idea of a dynamic choice set is that it's data is based on the time it was pressed (think of a recents list for a media app). The developer could, of course, send a static one, show it, then delete it.

That's not a good idea. There are specific use cases for that. It's a big chance of improving choice sets.

Not currently in this design. It would be good to do so, however. Perhaps those SDLChoiceSetObject parameters should be writable.

I think with that we would start confusing the app developer because they would expect that also PerformInteraction related parameters are uploaded to the head unit.

I would like to note you mention an issue with caching a few times but never describe it.

I was hoping the questions I asked are making the problems visible.

Not sure if you missed that part but please take a look at the provided block of code showing methods to allow managing choice sets of each type (static or dynamic). When uploading we should only request the choice set itself; not the whole interaction. Only a set of choices or single choices can be uploaded. I highly recommend to allow pre-upload and deletion of choice sets even if they are dynamic.

SDLChoiceSetManager should avoid uploading duplicates when possible. When performing an interaction the manager should compare the requested choice set with the available choices and set and perform the interaction immediately if the requested set already exist. Otherwise it should upload the set. Same goes for pre-upload choice sets. If the app request to upload a choice set which already exist the call should not affect any changes.

@joeljfischer
Copy link
Contributor

SDLChoiceCell

I would do the following mapping:...

I'm on board.

I agree it's unnecessary but if you check this search in sdl_ios there are a few results for voiceRecognition.

Yeah, those were probably my attempt to simply expand what is "vr" in the RPC spec for clarity. I think we're in agreement that voice is all that's necessary here.

Why don't we copy the choice set when passed to the SDL library? We should anyway do that so modifications don't affect our copy. I would add two initializers to the choice cell. One for the mandatory parameters only and one for all parameters. Still I don't want to readonly when it's not needed.

Even when copying them into the SDL library, the dev would be able to go in and modify them falsely. e.g. If we had an array of uploaded choice sets available to them, they could go in and change them, but those changes wouldn't be reflected on the head unit.

The class should be called SDLInteraction.

Hmm...I don't think we're going to agree here. When I hear "Interaction" I don't immediately think "Modal list view". We're going to probably have to ask for more feedback on this naming, perhaps from the Android devs (and the SDLC generally), since this will affect them too. I just don't like the "Interaction" naming and hope that this abstraction allows us to move to some clearer / more iOS / Android-like naming.

I'm OK with that but I want to note that for caching we need to compare cells already uploaded and requested to be uploaded.

True...I'm not sure what to do about that right now. It may be an implementation detail, or I may have to think about it more.

SDLChoiceSetDelegate

It should include OnKeyboardInput. To be clear: The keyboard can be used by any app. Media, POI, NAV etc.

I must have been mistaken. I'll need to play around with it a bit more. What kind of delegate methods would you suggest?

The table view delegate remains alive only during the lifecycle of the table view controller. View controller can be destroyed together with the delegate but the table content still exists, stored somewhere.

We should avoid that by asking for a delegate when the interaction starts. I don't want a weak reference for the time the choice set is stored on the head unit.

Yeah. I don't seem to be as worried about this as you are. I think devs will generally keep their delegates around. The reason for this system was, in my mind, the UI framework would have a separate UITableViewController-like controller per choice set, and the delegate would be tied to that...thinking about it, I think you're right and it may be better to pass a delegate as you go.

No, the idea of a dynamic choice set is that it's data is based on the time it was pressed (think of a recents list for a media app). The developer could, of course, send a static one, show it, then delete it.

That's not a good idea. There are specific use cases for that. It's a big chance of improving choice sets.

I'm not sure what you mean here. The current design provides two possibilities: a "my data is the same" set, and a "my data changes up to when the button is pressed" set, and automates those two. The first is an upload early on, and then a presentation. The second is an upload and present all in one call. I'll need to continue to think about this. Maybe upload and present should always

I think with that we would start confusing the app developer because they would expect that also PerformInteraction related parameters are uploaded to the head unit.

I don't think that in general, they don't want that deep understanding of what's uploaded, what's not, etc. I'm trying to smooth those edges with this API, so that generally they don't even need to know that we are uploading things to the head unit – kind of like how the screen manager hides the file manager. They can use the file manager to preload images, but most devs don't care about that and just want the image to show when they set the property. The existing choice set API is so complicated and problematic, and so hard to get right, that I'm trying to simplify where possible to reduce the number of things the developer needs to think about to get it right.

I would like to note you mention an issue with caching a few times but never describe it.

I was hoping the questions I asked are making the problems visible.

Clearly explaining them would certainly be helpful to make sure that I have your entire thought process and concerns covered.

Not sure if you missed that part but please take a look at the provided block of code showing methods to allow managing choice sets of each type (static or dynamic). When uploading we should only request the choice set itself; not the whole interaction. Only a set of choices or single choices can be uploaded.

I'm not a fan of your proposed set of methods. There's too many, and it doesn't seem intuitive enough. Remember that we both have several years of SDL knowledge; this stuff is confusing to someone starting out. In your scheme, a starting SDL developer with knowledge of iOS development seems to have more to learn than he does currently. All of the existing complexity of choice sets (minus IDs) seems to be in place, plus the addition of the "dynamic" BOOL. I would actually change my schemes uploadStaticChoiceSet to preloadChoiceSet, presentDynamicChoiceSet to presentChoiceSet, and presentStaticChoiceSet to presentPreloadedChoiceSet.

I highly recommend to allow pre-upload and deletion of choice sets even if they are dynamic.

I think we have a disconnect with naming. The entire point of a dynamic choice set (in my terminology) is that they are not pre-uploaded. That is why they are uploaded as individual choice sets with single choices, instead of a whole batch like the static (because the static is meant to not change over the lifecycle of the app's connection). The individual choice sets of individual choices can remain if desired, or deleted (via the delegate).

SDLChoiceSetManager should avoid uploading duplicates when possible. When performing an interaction the manager should compare the requested choice set with the available choices and set and perform the interaction immediately if the requested set already exist. Otherwise it should upload the set. Same goes for pre-upload choice sets. If the app request to upload a choice set which already exist the call should not affect any changes.

Sorry if it wasn't clear, that is certainly already a part of this proposal. That was the point of caching the dyanmic choice sets. They would be uploaded individually and, if they already exist, the developer's present would immediately run without having to upload again. Preloaded would do the same thing, of course, except that they're less likely to have been deleted (since they were preloaded).

@kshala-ford
Copy link
Contributor

kshala-ford commented Apr 11, 2018

SDLChoiceObject (or SDLInteraction)

I want to start with the naming of SDLChoiceSetObject to SDLInteraction and, more portantly, the fact that only the choices (without interaction related data) should be requested for uploading/caching:

First: The ChoiceSetObject class contains PerformInteraction parameters and the actual choice set. You're saying it's a pro to merge them but it'll be very confusing and limiting because none of the parameters are pre-uploadable except the choice set. Think of a ["yes","no"] choice set. If I want to use that choice set for another question I need to "re-upload" but actually the manager doesn't upload anything but stores another copy of the same choice set blocking memory. If you develop further for actual apps the managers store would hold many many choice set objects with duplicate chioces. In order to provide caching it needs to go through all objects and see if there's at least one occurrence/duplicate choice before creating/deleting the choice set. This class shouldn't be used for uploading/caching. Instead the actual array full of choices should be requested for pre-uploads.

Second: The name ChioceSetObject does not fit for voice (grammar) or keyboard (including SEARCH) related interactions. "Interaction" is like a hypernym covering all types of interactions including "modal list view". Because keyboards can be used by any app and for the purpose of providing a unified API for interactions the class should be called SDLInteraction.

At last: The name "Interaction" worked out for Android and iOS. I know dealing with choices sets and interactions is challenging but instead of hiding out interactions we should hide choice sets and all the pain coming with sets and start talking about "interactions" only. Means there should be an SDLInteractionManager, SDLInteraction with SDLInteractionDelegate and SDLChoiceCell.

Pre-uploads and caching

I think I now understand the confusing point. You're saying there are either static choice sets known before performing the interaction and dynamic choice sets to be created right before performing it. In fact this is not the whole truth. There are cases where a choice set can be pre-uploaded long before performing it but most of the choices are already known. Here are some examples:

  • Media App: "Recently played" list ordered by last time use. Every time you listen to a new playlist a new choice is added as the first entry. The oldest item is removed if the list reached a certain size.
  • Media App: "Current playback queue" showing a list of the next songs being played. When the player moves to the next song the first item must be removed. In case of "repeat all" it must be moved to the end of the list. This means no deletion at all. It's just a reorder.
  • Any app: "Search" to search for albums, artists, playlists etc. The interaction would be LIST_WITH_SEARCH. The choices would represent the history of previous search terms. Still the user can use the keyboard to use a new search term. If so the new search term would be added as a new choice right after the end of the interaction.

Those pre-known choice sets don't change too much so pre-upload as a dynamic set would be created very fast. That said the use case would be available very soon. The managaer should support pre-upload of dynamic choice sets allowing to present an interaction with any type of choice set. Actually we could hide the complication with static and dynamic by treating dynamic as the default.

The code example is revised to prioritize dynamic sets and remove method overrides (they are not needed...).

@interface SDLInteractionManager
/**
 * Presents an interaction on the head unit using the specified interaction and mode.
 * The choices of the interaction are automatically upload if they don't exist already on the head unit.
 * The results of the interaction are forwarded to the provided delegate.
 * The method is non-blocking.
 */
- (void)presentInteraction:(SDLInteraction *)interaction mode:(SDLInteractionMode)mode delegate:(id<SDLInteractionDelegate>)delegate;

/**
 * Uploads a set of choices to the head unit. 
 * Choices that were uploaded in another set won't be uploaded again.
 * If the set already exists this method has no effect.
 * Use this method to upload a set of choices in advance to an interaction.
 * Using this method helps to improve performance for choice sets known before being used in interactions and contain less than 100 items.
 * The method is non-blocking.
 */
- (void)uploadChoiceSet:(NSArray<SDLChoiceItem *> *)set completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Uploads a static set of choices to the head unit. 
 * Different to regular choice set the whole set will be uploaded at once without comparing choices from other sets.
 * If the static set already exists this method has no effect.
 * Use this method to upload a non-modifyable set of choices in advance to an interaction.
 * Using this method helps to improve performance for choice sets known before being used in interactions and contain more than 100 items.
 * The metod is non-blocking.
 */
- (void)uploadStaticChoiceSet:(NSArray<SDLChoiceItem *> *)set completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Deletes the provided choice set. This method works for both, regular and static choice sets.
 * The method is non-blocking
 */
- (void)deleteChoiceSet:(NSArray<SDLChoiceItem *> *)set completionHandler:(void (^)(NSError *error))completionHandler;


/**
 * Checks if the head unit already contains the provided static choice set.
 * The method is blocking but returns immediately without remote communication.
 */
- (BOOL)containsChoiceSet:(NSArray<SDLChoiceItem *> *)set;

@end

Even when copying them into the SDL library, the dev would be able to go in and modify them falsely. e.g. If we had an array of uploaded choice sets available to them, they could go in and change them, but those changes wouldn't be reflected on the head unit.

That would be true for your proposed .availableChoiceSets property. I don't think we should expose that but instead provide methods like .containsChoiceSet in the choice set manager. Then it would not be a problem at all to copy the set and make properties writeable.

@theresalech
Copy link
Contributor Author

The Steering Committee voted to defer this proposal, keeping it in review until the next meeting on 2018-04-17, to allow more time for discussion on the issue. The Steering Committee also agreed it would be helpful to have a comment on the issue summarizing the discussions that have taken place on the proposal thus far, so that all members can more easily understand the situation. The summary comment should include the changes to the proposal that have been agreed upon by @joeljfischer and @kshala-ford, and the changes that have not yet been agreed upon, with the two options for each change. The Steering Committee is encouraged to review this comment and become familiar enough with this proposal by the next meeting, so that they can informatively vote on the changes that still need to be agreed upon.

@joeljfischer
Copy link
Contributor

joeljfischer commented Apr 11, 2018

Changes to this Proposal

For the method / property name changes in context, see #453

SDLChoiceCell

  • title -> text
  • text -> secondaryText
  • subText -> tertiaryText

SDLChoiceSetObject

  • layout's type should be SDLLayoutMode
  • Review and investigate calculations of equality for cached uploaded set objects.
  • Clarify how choices are uploaded to avoid uploading duplicates (caching).

SDLChoiceSetDelegate

  • - (void)choiceSet:(SDLChoiceSet *)choiceSet didReceiveText:(NSString *)text fromSource:(SDLTriggerSource)source; -> - (void)choiceSet:(SDLChoiceSet *)choiceSet didReceiveText:(NSString *)text; Also move this to an optional method.
  • - (BOOL)shouldDeleteChoiceSet; -> - (NSArray<SDLChoiceCell *> *)shouldDeleteChoiceCells:(NSArray<SDLChoiceCell *> *)presentedCells;
  • Review and make possible additions for more keyboard based parameters (based on onKeyboardInput, based on setting global keyboard properties(?)).

SDLScreenManager Additions

  • Add a parameter for passing a delegate when presenting a choice set. Possibly remove the delegate parameter from the choice set itself, so that developers don't have to worry about keeping the delegate reference around after preloading a choice set.
  • Expose preloaded choice sets with a new parameter @property (copy, nonatomic, readonly) NSDictionary<NSString *, SDLChoiceSetObject *> *preloadedChoiceSets.
  • Add method - (BOOL)deletePreloadedChoiceSet:(NSUUID *)preloadedChoiceSetId
  • - (void)presentDynamicChoiceSet:(SDLChoiceSetObject *)set mode:(SDLInteractionMode)mode -> - (void)presentChoiceSet:(SDLChoiceSetObject *)set mode:(SDLInteractionMode)mode
  • - (NSUInteger)uploadStaticChoiceSet:(SDLChoiceSetObject *)set -> - (NSUUID)preloadChoiceSet:(SDLChoiceSetObject *)set
  • - (void)presentStaticChoiceSetWithId:(NSUInteger)id mode:(SDLInteractionMode)mode -> - (void)presentPreloadedChoiceSet:(NSUUID)preloadedChoiceSetId mode:(SDLInteractionMode)mode

Under Discussion

SDLChoiceCell

  • Should it have readonly properties or not?
    • Yes (author): It would be more confusing for a developer to change them and not have the changes be manifested on the head unit. It could cause safety problems if mutated at the wrong time. Copying in doesn't help because they can access uploaded ones off the screen manager.
    • No: It's not needed. We could copy them in.

SDLChoiceSetObject

  • Should this be renamed SDLInteraction?
    • Yes: This object contains parameters related to the PerformInteractionChoiceSet RPC. "I think the text "interact with the driver using a set of choices or a keyboard using SDLInteraction" is clear and easy to understand...Leaving the name as is makes it more confusing for keyboard use. SDLChoiceSetObject doesn't sound intuitive at all and it mixes interaction parameters which are not cached... it's going to cause confusion. 'Interaction' is like a hypernym covering all types of interactions including 'modal list view'. Because keyboards can be used by any app and for the purpose of providing a unified API for interactions the class should be called SDLInteraction."
    • No (author): "Interaction" was a poor name when chosen and remains a poor name today. It's not nearly descriptive (it is far too broad) enough, and now that we have higher-level APIs, we should get abstract the naming away in favor of more descriptive names. Having one "Interaction" that both presents a list/table view or presents a keyboard is unintuitive; the higher-level APIs should split those. I'm not a fan of the "choice set" name either, but hope the higher level UI manager can abstract it into more descriptive naming for a "modal list view". In sum, "Interaction" is a poor SDL naming provides the developer no information about what they're doing without reading the documentation, and we should abstract away required knowledge of RPCs naming and functionality.

@joeljfischer
Copy link
Contributor

SDLChoiceSetObject / SDLInteraction

I think I finally understand where you're coming from, but I remain in disagreement. The "Interaction" comes from "request the user's input / interaction, whether from a list of options by manual (hand) selection or voice selection, or through the user inputting a string of text via manual keyboard." This could be described as "requesting input", "data entry", "requesting user interaction", etc. That it took me so long to understand that from the "Interaction," should say something about how daft I can be, but also about how un-descriptive "Interaction" is, and that we should clarify it.

I maintain that separating the "choice / option / list / table picker" and the "keyboard" should be done. I understand now why you want "ChoiceSetObject" renamed, but maintain that "Interaction" is not clear enough. Many of the namings I would prefer would be a better fit for the UI manager, such as "ChoiceListViewController", "ChoicePickerViewController", "ChoiceListSearchViewController", "KeyboardViewController", etc.

Pre-uploads and Caching

here are cases where a choice set can be pre-uploaded long before performing it but most of the choices are already known. Here are some examples:

All of these examples would be "dynamic" choice sets. The first presentation would push them onto the screen, the remaining ones would reuse whichever were from the previous presentation. It is true that these list of options could be uploaded whenver their backing data changed, but I was considering the cost of sending that data over if the user doesn't press the button and prioritize not sending any data if the user didn't request to see it in those cases. We could do a more generic upload / present system, without the static / dynamic bifurcation (and I moved away a little from that in my latest revision with a preload system instead of "static").

Those pre-known choice sets don't change too much so pre-upload as a dynamic set would be created very fast.

All of these cases after the first would be very fast, the first would be slower because we don't know if the user will access it or not. The current state of "preload" doesn't allow for preloading dynamic sets in this case, I should note, because a preloaded set is considered as a set. I struggled over this for quite a while, and the reason I didn't allow for this case is because those preloaded "static" choice sets could then have some of its individual choices deleted, and I wanted to help the dev out by not allowing for that to happen.

With that said, I'm generally okay with the methods you provided, outside of a few things: (1) I like "preload" over "upload", and of course I prefer an alternative to "Interaction". (2) I prefer having the "interaction" information be cached so that they don't need to recreate it or hang onto it themselves...though perhaps that would be better held in the appropriate view controller of the UI manager than cached in this manager. (3) "Static" choice sets should have an id for presentation / deletion, or else they have to hold onto all the items, re-use them later, and we have to compare and try to determine if they mean to present a static choice set by comparing all items against all items in all choice sets. I've really been trying to reduce the number of methods to make it less confusing, but I'm not sure what can be done if we have the static/dynamic bifurcation. (4) What happens if they "delete" an item from a static choice set using deleteChoiceSet? I think this should fail. (5) See below quote/response.

@interface SDLScreenManager
/**
 * Presents an interaction on the head unit using the specified interaction and mode.
 * The choices of the interaction are automatically upload if they don't exist already on the head unit.
 * You *do not* need to wait for a preload to complete before presenting, the presentation will occur as soon as the preload completes.
 * The results of the interaction are forwarded to the provided delegate.
 * The method is non-blocking.
 */
 - (void)presentChoices:(SDLChoiceList *)list mode:(SDLInteractionMode)mode delegate:(id<SDLInteractionDelegate>)delegate;

/**
 * Uploads a set of choices to the head unit. 
 * Choices that were uploaded in another set won't be uploaded again.
 * If the set already exists this method has no effect.
 * Use this method to upload a set of choices in advance to an interaction.
 * The method is non-blocking.
 */
- (NSUUID *)preloadChoices:(NSArray<SDLChoiceCell *> *)choices completionHandler:(void (^)(NSError *error))completionHandler;

/**
 * Deletes the provided choice set. This method works for both, regular and static choice sets.
 * The method is non-blocking
 */
- (void)deleteChoices:(NSArray<SDLChoiceCell *> *)cells completionHandler:(void (^)(NSError *error))completionHandler;

@property (copy, nonatomic, readonly) NSSet<SDLChoiceCell *> *availableCells;

@end

That would be true for your proposed .availableChoiceSets property. I don't think we should expose that but instead provide methods like .containsChoiceSet in the choice set manager. Then it would not be a problem at all to copy the set and make properties writeable.

The main problem with that is that they can't just loop through the available choices looking for a title, they'd have to actually re-assemble the choice item and allow us to do the comparison check. If we had a unique "name" per choice item, we could do a contains, but without a unique identifier, I'm not sure. We may need something like that anyway, due to caching / comparing uploaded choiceItems.

@kshala-ford
Copy link
Contributor

SDLInteraction

I maintain that separating the "choice / option / list / table picker" and the "keyboard" should be done. I understand now why you want "ChoiceSetObject" renamed, but maintain that "Interaction" is not clear enough.

I stand by SDLInteraction for the reason that choice sets and keyboard usage cannot be separated. The interaction layout modes LIST_WITH_SEARCH, ICON_WITH_SEARCH use choice sets along with a keyboard. Even in KEYBOARD you can use of choices. Below you'll find more reasons for using "Interaction".

Many of the namings I would prefer would be a better fit for the UI manager, such as "ChoiceListViewController", "ChoicePickerViewController", "ChoiceListSearchViewController", "KeyboardViewController", etc.

I don't want the developer to use different managers/view controllers to reach out a layout that by a single option parameter. It's a bloat to the library of an unnecessary classes. Actually I would see the SDLInteraction to be the base class for where your subclasses set .layoutMode to the preferred option... but that would be overkill. I also think these subclasses are more difficult to find than offering everything in a single class and provide .layoutMode.

Interaction explains everything. Please don't be mad at me but I don't understand why it's not clear enough. You interact with the driver via voice, by presenting a set of choices and/or using a keyboard.

Every layoutMode is already covered by the proposed class.

Pre-upload and Caching

First of all I don't think we have an agreement to the screen manager additions.

I was considering the cost of sending that data over if the user doesn't press the button and prioritize not sending any data if the user didn't request to see it in those cases

I don't agree. First the app developer should make the decision what to preload so we should allow them to do so. Second it's very very important to provide a good responsive design. This is only possible if the choices already exist. The driver want's the interaction to start immediately when triggering the command. This is the important moment where the user rates the experience. Even creating a single choice prior to the interaction is a waste of time if the choice was already known. Creating a single choice can take up to a second (send request, let head unit compute, wait for response). I experienced creating a static 100 item choice set to take 30-45 seconds. This is also a reason for #180. Preload must be offered for dynamic sets. Preload for static sets only would waste time recreating the same choices over and over again. I think we are in an agreement to treat dynamic sets as the default type of choice sets?

All of these cases after the first would be very fast, the first would be slower because we don't know if the user will access it or not.

I don't get your point. Yes subsequent preload of dynamic sets are much faster. I don't see where my proposed additions are slower compared to your proposal.

I still propose my manager addtions for the following reasons:

  • I'm OK with using preload´ instead of upload`
  • No need to deal with NSUUID
  • provides compare method so developer doesn't need to implement a search

Internally the manager would hold dynamic choices and static sets separately. Comparison could be done by comparing the parameter values to avoid the need of an identifier. Time to compare is only a few microseconds. Let me try to give an explanation on what would happen under the hood:

  • Private @property NSSet preloadedChoices storing (dynamic) choices already preloaded
  • Private @proprty NSArray preloadedStaticChoiceSets storing static choice sets already preloaded
  • -presentInteraction:
    • If number of choices is > 100 calls preloadStaticChoiceSet otherwise calls preloadChoiceSet
    • Read out choice set ids internally then simply performs the interaction
  • -preloadChoiceSet:
    • Uses -minusSet: with a private property storing preloaded choices to get non-existing choices.
    • Returns immediately if all choices already exist
    • Every non existing choice will be uploaded (concurrent, not sequential)
    • After all choices are compared and non-existing choices are finished completionHandler is called
    • If one of the choices failed completionHandler is called with an Error
    • Accepts more than 100 choices (the provided set can be a bulk of choices e.g. contact list, artists etc.)
  • -preloadStaticChoiceSet
    • Loops trough the list and checks for existance
    • Uploads all choices at once (up to 100, they would be splitted into 100 chunks)
    • After the upload the completionHandler is called
  • -deleteChoiceSet
    • Deletes preloaded static choice sets and dynamic choices

It would be much easier if we agree to not support more than 100 choices per interaction anymore. I don't say I propose this limitation.

The main problem with that is that they can't just loop through the available choices looking for a title, they'd have to actually re-assemble the choice item and allow us to do the comparison check. If we had a unique "name" per choice item, we could do a contains, but without a unique identifier, I'm not sure. We may need something like that anyway, due to caching / comparing uploaded choiceItems.

Yes we need comparison anyway but I don't think we need a unique name. It would be yet another identifier. That's exactly what we are trying to avoid. The app developer should not think of unique identifiers (regardless of numbers or names).

Instead we should compare the parameters of the RPC struct. This would cause a few lines of code for -compare: but the computation is pretty fast as we only have to compare a few strings. The choice comparison is also more robust without an identifer. With an ID the app developer can use two ids for the same choice... and we wouldn't know about it.

@AndrewRMitchell
Copy link

We need the ability to thumbs up part of reply and not all of it. I want to give thumbs up but do not want it construed to be for the entire comment, oh well. Also forgive me, there is a lot going on in this conversation (and I've been on vacation for the last week) so I'm trying to make sure I digest it all.

Re: ChoiceSet vs Interaction -> ChoiceSet to me is way more descriptive of what is going on. Interaction is far to vague a term as it could refer to any and all interaction the user has with the HMI. We are specifically talking about a ChoiceSet here, and as such the naming should clearly reflect that.

Re: Preuploading -> As a developerI would say it should be my choice whether I provide that information ahead of time or not. That being said as someone who obsess over user experiences the best experience seems to clearly be when the options are already uploaded. The thing to remember is whatever your choice is, preload all or preload non (or even preload some), as a developer I am sure to have a use case that makes fulfilling that request extremely expensive for me. So while the obvious answer on the surface may be to preload all, it might actually work better for my user to preload none, or only preload the next screen worth of data. You can't (and won't) ever get every possible flow right for all situations. And while most of you are concerned with what the best user experience is from your point of view, there will always be a developer out there that has to deal with an entirely different set of constraints then what you've envisioned.

Re: Unique Names -> One thing we want to avoid at all costs is asking the user if they mean "Choice 1" or "Choice 1" and then having to disambiguate which one they mean. I don't think that is the case here based on the context and discussion. I am just pointing out however that while humans brains tend not to fault with things like division by 0, being asked to disambiguate between two identical items is far to resource intensive (mentally) a task to be asking someone to perform while driving.

@kshala-ford
Copy link
Contributor

kshala-ford commented Apr 16, 2018

Thank you @AndrewRMitchell for your comment.

Re: ChoiceSet vs Interaction -> Sure; ChoiceSet is obviously more descriptive for presenting a choice set but not for presenting a keyboard. Unfortunately due to the design of the PerformInteraction RPC it's difficult to impossible to separate choice and keyboard presentation because Search modes combine both. Due to the fact that PerformInteraction.interactionChoiceSetIDList is mandatory even the mode KEYBOARD requires a choice set. Depending on the HMI implementation KEYBOARD interactions either ignore the choice set or it performs an interaction in a mode like SearchWithList (the keyboard is primary but a choice can be selected). Let me try to list all possible variants of an interaction:

InteractionMode LayoutMode Uses
choices
Uses
keyboard
Note
Voice Ignored Yes No presentChoiceSet: OK
Manual Icon Yes No presentChoiceSet: OK
Manual List Yes No presentChoiceSet: OK
Manual IconWithSearch Yes Yes presentChoiceSet: OK although keyboard input can be returned instead of a selected choice
Manual ListWithSearch Yes Yes presentChoiceSet: OK although keyboard input can be returned instead of a selected choice
Manual Keyboard (No) Yes presentChoiceSet: doesn't make sense because keyboard is the primary interaction but still it requires a choice set. It also returns a keyboard input just as WithSearch.

Different to how it's proposed I want to keep the keyboard layout mode close to the other modes because it's so similar to the Search modes. I don't think it's intuitive to use one manager for keyboard including choices and another manager for keyboard without choices. I'm looking for a name that describes all the modes so a single manager can be used. That's all I'm trying to do. presentInteraction:layoutMode: is more generic and less descriptive but to me it describes both presentChoiceSet: and presentKeyboard:.

Re: Preuploading -> I fully agree. Preload none, some and all choices so they can be used later on (even in a different order) should be provided so the developer can decide.

Re: Unique Names -> To my understanding SDL (or may be it's Ford HMI) is able to detect and reject interactions with two identical choices. I guess what I was trying to propose is to use the content of the choice for preload comparison instead of a choice identifier set by the developer.

@kshala-ford
Copy link
Contributor

kshala-ford commented Apr 16, 2018

With regards to #189 I would like to see SDLChoiceCell.voiceCommands to be nullable to be prepared for non-voice choices. Internally the manager responsible for creating choice sets can use a placeholder for backwards compatibility (e.g. the choiceID as grammar computation performance is fast on numbers) as the VR command. Still the manager should not allow using non-voice choices in a voice interaction. Of course this should only be done for head units still require vrCommands for choices.

@joeljfischer
Copy link
Contributor

I don't want the developer to use different managers/view controllers to reach out a layout that by a single option parameter. It's a bloat to the library of an unnecessary classes. Actually I would see the SDLInteraction to be the base class for where your subclasses set .layoutMode to the preferred option... but that would be overkill. I also think these subclasses are more difficult to find than offering everything in a single class and provide .layoutMode.

I'm not worried about class bloat so long as it makes things clearer for the developer. If it becomes that the number of classes are confusing, then that's worrisome to me. I'm most concerned about clarity. I'm not advocating different class names for the sake of it, but to provide additional keyboard related parameters and a cleaner separation of code in relevant classes. Maybe a required initialization parameter with a configuration object would be better? In either case, this is side-tracking us from this proposal.

Interaction explains everything. Please don't be mad at me but I don't understand why it's not clear enough. You interact with the driver via voice, by presenting a set of choices and/or using a keyboard.

Because when you see a class named "interaction," I don't believe a developer will think "present the driver with a set of choices accessible by voice or pressing, or present them a keyboard," without additional information. I think that you may have lenses caused by using SDL for many years as a primary part of your job. I've been using SDL for several years and I've still recently found new stuff the Interaction RPC can do. I want to make that cleaner in code and clearer in concept for developers by separating what will certainly seem to them to be separate use cases.

I was considering the cost of sending that data over if the user doesn't press the button and prioritize not sending any data if the user didn't request to see it in those cases

...

I agree that we should allow preloading choices in all situations.

Time to compare is only a few microseconds. Let me try to give an explanation on what would happen under the hood

...

Yes we need comparison anyway but I don't think we need a unique name. It would be yet another identifier. That's exactly what we are trying to avoid. The app developer should not think of unique identifiers (regardless of numbers or names).

Instead we should compare the parameters of the RPC struct. This would cause a few lines of code for -compare: but the computation is pretty fast as we only have to compare a few strings. The choice comparison is also more robust without an identifer. With an ID the app developer can use two ids for the same choice... and we wouldn't know about it.

I strongly agree that I do not want the developer to be thinking of unique identifiers, however I also do not believe that it will only take few microseconds to compare. With large amounts of choices in the set (I'm thinking of a thousand or more) it could take a significant amount of time. Each "choice" has a not-insignificant number of parameters, and while comparing each individually does not take much time, when you consider that you are comparing, say 8 parameters on a 100 item choice set with 1000 already preloaded, you've ballooned to several hundred thousand comparisons. This might be faster using NSObjects hash parameter, which we would have to custom implement, as it would cut down on our raw comparison count, but we're still looking at tens of thousands or over a hundred thousand comparisons. Alternatives that would be faster include: (1) key-value lookup using a dictionary with a unique key (I think that the primary text of a choice must be unique), or (2) pointer-identity lookup using NSHashTable, but the developer would be required to find the item they want and pass that back to us. Any other options would be welcome.

With regards to #189 I would like to see SDLChoiceCell.voiceCommands to be nullable to be prepared for non-voice choices. Internally the manager responsible for creating choice sets can use a placeholder for backwards compatibility (e.g. the choiceID as grammar computation performance is fast on numbers) as the VR command.

I think this would be a good change.

@AndrewRMitchell
Copy link

@kshala-ford I understand where you're coming from, but also agree with @joeljfischer 100%. That being said, I think I may have a viable alternative to Choice Sets. Call them Choice Interactions. presentChoiceInteraction I think would be OK for every row in the table you were so nice to provide (thank you for that). And I think it would also work for making it logical for developers, the standpoint that Joel and I are so strongly arguing for. Its solves your problem (they are not always sets) and ours (Interaction by itself is too nondescript).

@kshala-ford
Copy link
Contributor

I'm sorry guys. I think I'm too focused in finding a single word that fits for interactions with choices, keyboards or both. I thought about it for a while and I think separating makes sense but more within the same context of the screen manager. Below you'll find some code representing my idea of starting a voice session, presenting a choice set only, a keyboard only or the combination of both (choice set with keyboard):

@interface SDLChoiceSet
@property (copy, nonatomic, readonly) NSString *title;
@property (copy, nonatomic, readonly, nullable) NSArray<SDLTTSChunk *> *initialPrompt;
@property (copy, nonatomic, readonly) NSArray<SDLChoiceCell *> *choices;
@end

typedef NS_ENUM(NSUInteger, SDLChoiceSetLayout) {
    SDLChoiceSetLayoutList,
    SDLChoiceSetLayoutTiles,
};

@protocol SDLChoiceSetDelegate
- (void)choiceSet:(SDLChoiceSet *)choiceSet didSelectChoice:(SDLChoice *)choice;
@end

@protocol SDLKeyboardDelegate
- (void)keyboardDidSendEvent:(nonnull SDLKeyboardEvent)event text:(nullable NSString *)text;
@end

@interface ScreenManager

// Starts a voice session with the user to select a choices from the given set.
// The provided help prompt is used if the user say "help" or if a timeout is reached with no voice input.
// interaction mode: VOICE
- (void)presentVoiceChoiceSet:(SDLChoiceSet *)choiceSet helpPrompt:(NSArray<SDLTTSChunk *> *helpPrompt delegate:(id<SDLChoiceSetDelegate>)delegate;

// Presents the given choice set on the user interface
// The choices are presented either as a list or in tiles.
// interaction mode: MANUAL, layout mode: [LIST or ICON]
- (void)presentChoiceSet:(SDLChoiceSet *)choiceSet layout:(SDLChoiceSetLayout)layout delegate:(id<SDLChoiceSetDelegate>)delegate;

// Starts a session with the user to select a choices from the given set.
// The choices are presented either as a list or in tiles.
// Additionally a keyboard with the given properties will be presented.
// interaction mode: MANUAL, layout mode: [LIST_WITH_SEARCH or ICON_WITH_SEARCH]
- (void)presentChoiceSet:(SDLChoiceSet *)choiceSet withKeyboardProperties:(SDLKeyboardProperties *)properties layout:(SDLChoiceSetLayout)layout delegate:(id<SDLChoiceSetDelegate, SDLKeyboardDelegate>)delegate;

// Starts a session with the user to 
// interaction mode: MANUAL, layout mode: KEYBOARD
- (void)presentKeyboardWithProperties:(SDLKeyboardProperties *)properties delegate:(id<SDLKeyboardDelegate>)delegate;

...

@end

First of all I think helpPrompt and timeoutPrompt is the same for pretty much every app. I don't know of any app using different prompts. Second I moved it out of the set because it's used for voice sessions only. I think it slims the choice set fairly well. Last when it comes to a voice mode the layout mode is ignored. So I thought it's not necessarily needed inside the choice set class.

The methods to present a choice set now ask for the layout. List or Tiles. That's pretty much it. Whenever a keyboard is used it'll say that in the method name. This is how the other layout modes (WITH_SEARCH or KEYBOARD) are covered. That's why the choice set class doesn't require the layout mode.

When it comes to keyboard the keyboard related methods should be used. In my opinion it's crystal clear that the choice set is used together with a keyboard or the keyboard is used exclusively (with no relation to choice set classes/protocols). It would work for OnKeyboardInput as well as for PerformInteraction.manualTextEntry.

I'm struggling with the keyboard properties though... The above API wouldn't allow changing the keyboard properties during a session (e.g. for auto complete). As an alternative we could move them out to a screen manager property:

@interface ScreenManager
...

@property (nonatomic, nullable) SDLKeyboardProperties *keyboardProperties;

- (void)presentChoiceSetWithKeyboard:(SDLChoiceSet *)choiceSet layout:(SDLChoiceSetLayout)layout delegate:(id<SDLChoiceSetDelegate, SDLKeyboardDelegate>)delegate;

- (void)presentKeyboardWithDelegate:(id<SDLKeyboardDelegate>)delegate;

What I'm trying to do here is to cover the possible cases of how PerformInteraction is used today. I think the above suggestion is a good compromise.

@joeljfischer
Copy link
Contributor

Hey @kshala-ford I think we're narrowing in on an API we can all agree with. I'll take a look at this and make revisions after the meeting tonight.

@NickFromAmazon
Copy link

NickFromAmazon commented Apr 17, 2018

I like @kshala-ford's initial suggestion of Interaction suggestion for a slightly different reason (sorry in advance for the long-winded tangent):

I'd prefer to see these Managers remaining somewhat analogous to the SDL APIs that they're wrapping. As an SDL application developer, the MOBIL_API.xml document is the first place I'll go to understand what functionality is available, as any mechanism supported by the mobile libraries to influence the HMI is necessarily a subset of what's described there.
What's generally unclear from that document is how to use each API - (things like pre-requisite states for each call, what is persisted on the head unit between between connections or HMILevel changes, how precedence is handled between different temporal/modal interactions, etc.) If these managers can clearly illustrate that with an obvious mapping between the transport API and the code, that is useful.
Providing an API with more familiar mobile paradigms is definitely helpful, but I feel that belongs a layer up, and we already have a great proposal in flight for that.

Given that both a keyboard and choice 'cells' can be part of a single Interactionmodal, I don't like the idea of splitting apart separate APIs, as I don't think that accurately represents the experience.

However, I also agree that Interaction is excessively generic, and I liked the direction @AndrewRMitchell was headed by adding some kind of descriptive word to the name ChoiceInteraction or UserInputInteraction or similar.

A few other thoughts/opinions:

  • Regarding readonly properties - I'm strongly in support, as this makes development easier and removes any ambiguity around whether properties could have dynamic behavior.
  • delegate should be attached to the Interaction/whatever instance as in the original proposal. This is a familiar Obj-C pattern, and I'd expect more issues if that isn't followed.
  • I found the static/dynamic behavior relatively hard to follow, especially with the delegate involved.
  • I'm not a fan of the proposed storage of an entire SDLChoiceSetObject object, as the actual API is more expressive - I don't believe this proposal allows clients to change other PerformInteraction parameters while re-using a pre-loaded choice set.
  • It looks like the API allows presenting multiple choice sets as part of an interaction, which also doesn't seem to be covered in this proposal and could be useful if clients are preloading multiple categories of choices.

Given all that, I'd prefer to see the manager kept somewhat more inline with the underlying core API, something along the lines of:

/// Ample documentation describing what an `InputInteraction` is
@interface SDLInputInteraction
...
// ChoiceSet will be uploaded when able
-(instancetype)initWithXYZ... choiceSet:(NSArray<SDLChoice *>):choiceSet (BOOL)persist;

// ChoiceSet is pre-loaded
-(instancetype)initWithXYZ... choiceSetId:(NSUInteger):choiceSetId;

// No ChoiceSet (keyboard only)
-(instancetype)initWithXYZ...;

@property (assign, nonatomic, readonly) SDLInteractionMode interactionMode;
@property (copy, nonatomic, readonly) NSArray<SDLChoice *> *choices;

// This can be set by the SDLInputInteractionManager if `persist` is true, possibly with a default value > 2000000000 indicating unset 
// Could also be an array if a list of ids is provided
@property (nonatomic, readonly) NSUInteger choiceSetId;

@property (weak, nonatomic) id<SDLInputInteraction> delegate;
...
@end
@interface SDLInputInteractionManager

...

- (NSUInteger)preloadChoiceSet:(NSArray<SDLChoice *> *)choiceSet;
- (void)deleteChoiceSet:(NSUInteger)choiceSetId;
- (void)presentInputInteraction:(SDLInputInteraction *)interaction;

...

@end

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Apr 18, 2018
@theresalech theresalech changed the title [In Review] SDL 0157 - Mobile Choice Set Manager [Returned for Revisions] SDL 0157 - Mobile Choice Set Manager Apr 18, 2018
@theresalech
Copy link
Contributor Author

The Steering Committee has voted to return this proposal for revisions, to incorporate the agreed upon changes and additional changes based on the conversation above between the Ford, Livio, and Amazon teams. It was noted that the revised proposal will likely require additional discussion, but a cleaned up version of the proposal to reflect the discussions so far would be helpful for members to review the proposal in its current state.

@theresalech
Copy link
Contributor Author

Due to the number of comments on the original proposal and the amount of changes within the revised proposal, a new review issue has been created for the revised proposal: #475. Please provide any comments on the revised proposal there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants