[Accepted with Revisions] SDL 0157 - Mobile Choice Set Manager (Revised) #475
Comments
|
I'm very confused with the revision. Trying to keep my comment short without giving feedback to every details. I guess it makes sense to discuss the proposed changes in a phone conference before we enter the same like #449 SDLChoiceCellGroup will cause a lot of confusion. Static choice sets can be provided without the need of yet another class. I thought that we had an agreement on that to hide the pain of the differences. What's SDLChoiceCellPrimaryText? Do you want to identify choices using the I don't think the delegates and present methods are based on the given feedback.
Why is there a delegate in the choice set and another delegate in the presentXY method? I'm not sure if I can agree to the revision. I kind of see parts of the feedback from #449 (comment) added but with many changes which are not explained and other additions that were never discussed. |
|
I will try to keep this as short as possible, but there are a lot of questions here.
I had to make a choice. I believe that the old design was becoming too confusing. In order to clarify the differences between presenting a set of choices that are "dynamic" and a set of choices that must always be presented "grouped" together, I decided to make another small class that allows developers to pass the entire group and make crystal clear that the choices in this group cannot be used in a "dynamic" preloaded choices.
I thought I'd gotten all the references to that. It was from an earlier version of this revision and can be ignored. That reference should be an
They were in part, but I saw issues with it as-is and decided to do a more thorough review and design revision in order to take the presented use-cases from the feedback and design a proposal that filled those use-cases as well as possible.
Yes, this can be removed. I like it because it gives an indication to the developer that they can / should delete unneeded cells after the presentation is complete, but, after consideration, this can be done in the documentation.
Hmm, it's not that this is not correct, it seems that this just doesn't do what you want it to. It is correct in that it does what it intends, and provides a delegate callback for user submission of the text. Your specific issue is meant to be mitigated by the optional delegate
Again, I think that this makes perfect sense, it just isn't doing what you think it should. Yes, the keyboard event includes other enums, but this delegate callback would only be called for an enum value that makes sense (i.e. cancel and abort). It wouldn't be called in the other cases, which seems to be your confusion? While, yes, other enum values exist, and I considered creating new enums that split those enums up, because the user isn't passing us enum values it should be okay as-is. Every enum value we pass them will make sense in context.
You'll notice that the choice set delegate is for the choice set and is always used. The other delegate is the keyboard delegate and is only used in some cases. To prevent the keyboard delegate from being optional and creating situations in which no keyboard delegate is set but the choice set is presented in a keyboard context, I chose to put it on the called method when keyboards are in use.
I think it is unfair to dismiss this revision out of hand because it has some changes that weren't discussed. I believe that the changes are fairly well explained, and I have tried to answer any questions above. Just because this revision makes changes that weren't discussed doesn't mean it shouldn't be reviewed on its own merits. If you have additional feedback or issues with the proposal, please bring them forward. |
|
This review is 475 only, I'm ignoring the fact that 449 even existed. I'm also taking into account any discussion occurring above. One issue I have is if I have is if I have an SDLChoiceCell that I want to use in multiple choice sets I can't. In other words, if I have a response that exists in multiple choice sets, I have to treat them as separate responses, even if to me they really are the same. This means I have to maintain a mapping from the SDLChoiceCell to some internal representation just because I'm using it across multiple choice sets. I understand the logic, just calling out it may put additional work on my side. |
|
Hi @AndrewRMitchell, I'm not quite sure what you mean. If you have a choice cell that you want to use in multiple choice sets, you would preload that cell using |
|
@joeljfischer Sorry, I was trying to get the review done in another meeting as I'm time constrained this week. I was reading the static section twice and internally was thinking dynamic. Ignore my issue. |
|
I think the way of using the group class isn't really an improvement to the way choice sets are used today. It's just being done in a different API. Yes, it's helping me with ids but the only way to use >100 choices is to preload them, create the choice set in the completion handler and then present it. I would have expected choice sets with >100 choices to be treated as static ones. I guess it's sufficient to document this so the developer knows about this. I can see the benefit of another proposal increasing the maximum of Regarding the choice set: I believe we might want to merge help prompt and timeout prompt as they are mostly set to the the same anyway. As an alternative if timeout prompt isn't set the manager can use the help prompt array for both. As they are relevant for the voice mode only I don't think it's needed in the choice set. I still don't think the delegate makes sense in that class. I thought we had an agreement to that. Don't want to repeat myself. I don't think the layout mode is necessary in the choice set. I don't even think the interaction mode is needed at all. Not sure if this was visible or described in my last comment. The layout mode is relevant only for manual interaction mode and is ignored for voice mode. The mode "both" is not needed as it can be implemented by the app is desired but at Ford we have received customer complains about "both" as it is confusing why there's a manual interaction although the voice interaction was cancelled... At last I was thinking about the necessarily of timeout parameter and left it out in the beginning... but it's not relevant for this discussion. After all that would reduce the choice set to: @interface SDLChoiceSet
@property (copy, nonatomic, readonly) NSString *title;
@property (copy, nonatomic, readonly, nullable) NSArray<SDLTTSChunk *> *initialPrompt;
@property (assign, nonatomic, readonly) NSTimeInterval timeout;
@property (copy, nonatomic, readonly) NSArray<SDLChoiceCell *> *choices;
...
@endRegarding presentation of choice sets/keyboard Because the interaction mode "voice" ignores the layout mode and the interaction mode "manual" ignores help/timeout prompt I proposed to separate them. - (void)presentVoiceChoiceSet:(SDLChoiceSet *)choiceSet
helpPrompt:(NSArray<SDLTTSChunk *> *helpPrompt
delegate:(id<SDLChoiceSetDelegate>)delegate;The above method starts a voice session with the user to select a choices from the given set. - (void)presentChoiceSet:(SDLChoiceSet *)choiceSet
layout:(SDLChoiceSetLayout)layout
delegate:(id<SDLChoiceSetDelegate>)delegate;This method presents the choice set on the UI either as a list or in tiles/icons. - (void)presentChoiceSetWithKeyboard:(SDLChoiceSet *)choiceSet
layout:(SDLChoiceSetLayout)layout
delegate:(id<SDLChoiceSetDelegate, SDLKeyboardDelegate>)delegate;The above method presents the choice set on the UI including a keyboard (which does not necessarily stands for "search"). The keyboard properties of the screen manager are used by default. The keyboard delegate can be used to modify the keyboard properties during presentation (@joeljfischer I think your optional methods in the delegate are a good idea but I believe the whole keyboard properties should be modifiable for each keypress e.g. to set the limited char set etc.) - (void)presentKeyboardWithDelegate:(id<SDLKeyboardDelegate>)delegate;The above method presents the keyboard directly to request user input. All these methods reduce the amount of parameter to the minimum required for the type of presentation. To me they look much simpler because the app developer can see for each call what properties are needed because unneeded properties are not requested. Regarding the delegates we already covered the one for choice sets. The keyboard delegate you proposed doesn't fully match the SDL API. @protocol SDLKeyboardDelegate
- (nullable SDLKeyboardProperties *)keyboardDidSendInput:(nonnull SDLKeyboardEvent)event text:(nullable NSString *)text;
@endThe above delegate call is different to my originally propsed one but it maps to Regarding choice caching we must not use the primary text because SDL and the HMI don't use it for unique identifcation. There can be interactions with the same primary text e.g. in list mode. Instead the manager should hash the content of a choice which is used to compare it with other choices to avoid full duplicate items. |
|
I forgot to note that #189 (comment) refers to this proposal. The VR commands for choices are already nullable but the placeholder for keyboard interactions isn't considered yet. I propose to include the below text in this proposal:
|
|
I apologize if my brevity sometimes comes off as dismissal. That isn't intentional; I am trying to reduce the length of these posts as much as I can.
I don't think we can strictly go to dynamic sets. I think it's a big help for lists that always remain the same (easier to reference as a whole) or need to be over 100 long (though we could remove documentation around that, as it's kind of a cheat and not recommended). It provides the delegate system and ID management enhancements.
I don't know if that's a good idea. One is for when the user says "help" and the other is if it times out, right? Those are fairly different use cases. I don't have a problem with providing "default" text for either, though that would require translation work.
With the current proposal it makes a lot more sense to be there, as the
If we could remove 1a. User presses menu button 1b. User speaks menu voice command The choice set, even if it has a manual layout, should have the option of being presented in VR mode at presentation time.
Due to the above reasons, I disagree with all that is in this section, and recommend sticking to my outlined methods.
Changing the limited char set seems an odd and low-usage use case. If it's a necessary one, I would either recommend removing the current autocomplete one, or adding one to change the char-set per key-press. I don't think that optional delegate method is currently necessary however.
I think this is more confusing. You're essentially combining three of my delegate methods into one super-method that I find significantly less clear. The dev, if they just want the input from the user, has to implement your method, know to return
This proposal already does full choice cell hashing and not what you say here.
Your 5 points would be a good addition to the proposal because it works today. Though, it shouldn't, and is probably considered a Ford bug? So it may merit more discussion. Additional text about changes in the spec would have to be held until that spec change occurs. |
|
For reference for tonights meeting: Accepted changes:SDLScreenManager
SDLChoiceSetDelegate
SDLKeyboardDelegate
Other
|
|
First to make sure I'm correctly following, the core mobile API as I'm reading it allows you to:
I agree that an extra Manager API that allows me to combine steps 1-2, and optionally 3, for dynamic data would be very convenient; moving too far beyond that and I'm worried that either complexity will increase, or supported functionality will be hidden.
Maybe I'm fundamentally misunderstanding the API but I'm still unclear on this one: On that point (re-hashing the old debate,) I find it confusing to have multiple types essentially name Choice[Collection]: the SDLChoiceCellGroup (that actually corresponds to core's 'ChoiceSet') and the ChoiceSet (corresponding to core's 'Interaction').
Not a showstopper to me, but I find it unintuitive to have to use a delegate as a method parameter AND and a property in order to detect a completed interaction.
Not completely sure I'm following the conversation here, but agree this should be handled independently from the Interaction's response. Perhaps it could even be sidelined and then handled more generally - some kind of optional 'KeyboardInputProcessor' that could be used in the future if/when additional keyboard input fields are supported?
The proposal states |
To clarify for everyone (sorry for the length here), a a. If you need more than 100 choices in a presentation. This is not a recommended use-case. Now, this use-case isn't necessary, and I fully admit that. We can remove it, and only use the (2) use-case above. That does work in all cases (outside of > 100 cells). The reason I have not already done so was the discussion on the previous revision of this proposal regarding having both "static" and "dynamic" cells. This mimics that with different naming. Without this
I don't think using it will be too unintuitive. A choice set doesn't necessarily have a keyboard attached (whereas it does necessarily need the other delegate), only a
While I agree having it in a separate delegate is the way to go – that is my proposal, after all – I disagree that we need a new proposal for handling keyboard input, this proposal's discussion has gone on too long as it is IMO. Regardless, I think such a class would overcomplicate things for what we have and be less intuitive than my proposal.
Gotcha, that downside is from an earlier version of this revision. You are correct, the intention is from that comment. |
That's not true because the choices still need to referenced separately. Interactions with <100 choices are fully manageable with dynamic sets with no additional effort. I don't know of anyone who's using interactions with more than 100 choices and I cannot think of a good reason to do that due to driver distraction. If it's really demanded to be able to have intearctions with more than 100 choices than the API that was already proposed should be used allowing choice array >100 items, treating them as static choice sets. That would keep it more transparent to the developer without dealing with groups for >100 choices. Anyway increasing the limitations for the next major release would solve this problem anyway.
Not sure what you mean. I compared the old and the new SDLChoiceSet object and cannot find the difference or a reason for keeping the delegate inside.
Skipping the rest of the paragraph and saying all of that doesn't apply. The manager layer wouldn't use the still remaining interactionmode enum from the proxy layer. There's no spec change required. BOTH is still available with the proposed methods but in a different and much simpler and cleaner way because they request only data which is necessary for the mode requested.
Same here. See above reasons. You're reasons don't apply. BOTH is still available.
Deciding on the use cases is at the developers decision. I'm proposing to keep the potential possibility especially because it's so easy to do so.
I'm following OnKeyboardInput notification and *all the event types thinking it's easily done by using the event type as a parameter of the method. I don't see all of them supported by your methods. Here's the list for reference: <element name="KEYPRESS" />
<element name="ENTRY_SUBMITTED" />
<element name="ENTRY_VOICE" />
<element name="ENTRY_CANCELLED" />
<element name="ENTRY_ABORTED" />Regarding hashing choices I'm sorry for my note. I found that explained very implicitly in the proposal. I would appreciate this to be noted down specifically. The fact that Ford already supports optional VR commands is an intentional feature, not a bug. |
There's no reason for moving it outside. It's a required parameter on all interactions involving a choice set.
I believe my proposal is the best suited and clearest and I stand by that.
Okay, I'll add that we should add a new optional keyboard delegate method
As noted in the proposal:
I didn't fix up the downsides section properly, which made it confusing, as Nick pointed out. It appears to only be implict in this proposal, as you note. But, that is the method of this version of the proposal. |
|
The Steering Committee voted to defer this proposal, keeping it in review to allow more time for SDLC Members to review and discuss. A good summary of the items in discussion to reference can be found in this comment. It was also noted that the next vote on this proposal will likely be pushed to the 2018-05-23 Steering Committee meeting due to the author's upcoming schedule. Implementation timing will need to be assessed once a version of the proposal has been agreed upon. |
|
The Steering Committee voted to defer this proposal, keeping it in review to allow more time for SDLC Members and the proposal's author to review and discuss. |
|
Below are five points of changes proposed including pros/cons/reasons. These points are basically a summary of open items from above comments. 1. Choice groupI support the authors consideration fo removing the group class. The options are:
I propose to do option 1 (following authors consideration). App developers should not present >100 choices anyway... 2. No delegate in the setI propose to remove the delegate property from the choice set class. Instead it should be requested in the method call (see comment) Pros:
Cons:
3. Keyboard delegateI propose to use the following delegate (which is a compromise) @protocol SDLKeyboardDelegate
/** Sent upon ENTRY_SUBMITTED (but not ENTRY_VOICE) */
- (void)userDidSubmitText:(NSString *)text;
/** This will be sent if the keyboard event ENTRY_CANCELLED or ENTRY_ABORTED is sent */
- (void)userDidCancelInputWithReason:(SDLKeyboardEvent)event;
/** Sent upon *any* keyboard event */
- (void)keyboardDidSendEvent:(nonnull SDLKeyboardEvent)event text:(nullable NSString *)text;
@endReason:
4. Custom keyboard configuration (incl. auto complete and limited charset)I propose to not add custom keyboard and any other keyboard specific delegate method and use the keyboard delegate protocol proposed above Reason:
Instead the app should update the keyboard properties through the manager 5. present methodsI propose to use all the -(void)presentKeyboardWithTitle:(NSString *)title delegate:(id<SDLKeyboardDelegate>)delegate;Reason:
|
|
The Steering Committee has voted to accept this proposal with revisions. The revisions can be found in this comment. Revisions will include both the items in the |
|
@joeljfischer please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks! |
Hello SDL community,
The review of the revised "Mobile Choice Set Manager" proposal begins now and runs through May 1, 2018. The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/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:
#475
Reviews of the original "Mobile Choice Set Manager" proposal can be found on this issue: #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:
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
theresa@livio.io
The text was updated successfully, but these errors were encountered: