[Accepted with Revisions] SDL 0238 - Keyboard Enhancements

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0238 - Keyboard Enhancements" begins now and runs through July 16, 2019. The last review of this proposal took place June 5 - June 18, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0238-Keyboard-Enhancements.md

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

#754

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
theresa@livio.io

[joeljfischer]

1. I think the first point and addition is clear and very meaningful.

2. If I'm understanding this right, this is to allow the ability to remove a character from use once it's been pressed by the user? e.g. A key button can only be pressed once? The proposal is rather unclear on what exactly this is.

3. I'm a bit more skeptical of the third option. Is this really a needed change? Looking at mobile keyboards, this is not an approach they tend to go with.

4. The following KeyboardCapabilities struct is incorrect:

<param name="configurableKeys" type="ConfigurableKeyboards" minsize="0" maxsize="10" maxlength="10" array="true" mandatory="false" since="X.X" >

a. maxLength is a String property, not an array property. It should be removed.
b. minSize should be 1 for optional array params.

5. The Android and iOS changes need to note changes to the ScreenManager which may not be insignificant to support the new KeyboardCapabilities.

[ashwink11]

Thank you for reviewing this proposal.

2. If I'm understanding this right, this is to allow the ability to remove a character from use once it's been pressed by the user?

Could you please let me know, which section are you referring too?

3. Yes, Mobile keyboards do not follow this approach. However, they do have multiple keyboard layouts, which might not be the case for IVI's.
   This approach makes it possible to have one configurable keyboard layout and App can configure as per their requirement.

4. I will change it as per the comment.

[joeljfischer]

2. The masking keyboard input feature. The proposal doesn't really describe what this is.

[ashwink11]

2. This is used for hiding entered characters. If certain app expects input from the user for authentication, they would like to hide characters. For example, password entry or PIN entry for authentication.

Currently, there is no way in mobile API to achieve this.

[joeygrover]

Overall, good changes that will make the keyboard feature much more useable. Just a few minor notes.

| 6. "KeyboardCapabilities" struct should be returned with "RegisterAppInterface" response. I would recommend adding this as a system capability. Not all apps will use the keyboard functionality and we have started to move capabilities into that request as is. Edit: Or as mentioned, moved into the ScreenCapability when implemented.
| 7.
<param name="supportedKeyboardLayouts" type="KeyboardLayout" minsize="0" maxsize="10" array="true" mandatory="false" since="X.X" >
Is there any reasoning behind a max limit of 10? Min size should also be 1 for optional parameters. No reason to include an empty array.

| 8.
<param name="configurableKeys" type="ConfigurableKeyboards" minsize="0" maxsize="10" maxlength="10" array="true" mandatory="false" since="X.X" >
Including Joel's previous remarks, I would ask the same question as 7, how was the max size determined?

| 9. Not necessary for acceptance, but the RPC spec additions should add all their descriptions where available. This will keep they consistent between platforms.

| 10. I'm struggling to see the use case for USER_CHOICE_INPUT_KEY_MASK.

[joeljfischer]

6. @joeygrover I was going to comment the same thing, but I saw it in the Alternatives Considered. It should probably move up to the Proposed Solution.

2 / 10. The only use case I can see is for cloud app login. However, I'm not sure that masking is something we should really do in a vehicle setting. The vehicle is already a pretty private setting; the passenger could just watch what the driver is typing in. @joeygrover The user choice would be for the user who's typing in their password on a vehicle keyboard to be able to toggle a secure entry field to see what they're typing. However, I'm more skeptical of the feature as a whole.

[joeygrover]

| 2/10 Ah, I misread, I understand now. I can see both arguments. Introducing this feature will put some burden on developers on how to handle older generations that do not support the masking in general. Which forces them to create additional app flows.

[ashwink11]

2. When entering any credentials, the user would expect that the entered characters will be masked, as it is done in other devices or on websites. Its an established practice for entering authentication details. The current proposal will enable apps to use this capability.

Also, we have been in discussions with some of the App Partners, who requested it. They considered it as one of the Security Issues, as entered credentials could be seen by vehicle passengers or someone standing outside a stationary vehicle.

6. Agreed. I will move this to Screen Capability.

7. As suggested, I will change "minsize" to 1.
   "maxsize" is used just to define an upper limit. We observed that there are only 3 keyboard layouts implemented in IVI for existing SDL enabled systems. So, to be within comfortable limits, "maxsize" is defined as 10. We can also set "maxsize" to 4 and increase this as and when a new Keyboard layout is added in enum "KeyboardLayout".

8. As suggested, I will change "minsize" to 1.
   We observed that there are only 3 to 5 special characters in keyboard layouts implemented in IVI for existing SDL enabled systems. We expect that there won't be more than 10 configurable keys in any Keyboard layout.

9. I agree. For Apps supporting old IVI, they will need to define the alternate flow. This feature will be needed for Apps which would like to use IVI for Authentication and as @joeljfischer mentioned for Cloud Apps login.

[theresalech]

The Steering Committee voted to defer this proposal, keeping it in review until our next meeting on 2019-06-18, to allow the author time to respond to the comments left on the review issue.

[ashwink11]

@joeygrover @joeljfischer I made changes in the proposal as per review comments.

Regarding point 5, I did not find screen capability usage in Screen Managers. I checked both develop and master branch for both platforms. Will Screen Capabilities would be included in Screen Managers?
What changes do you foresee in screen managers with respect to keyboard Capabilities?

[joeljfischer]

2.

as entered credentials could be seen by vehicle passengers or someone standing outside a stationary vehicle.

They can just watch the user enter it on the keyboard. This provides virtually no security. However, I understand that perception can be important.

7. The maxSize should be 99, or 9999. There's no reason to define an upper limit. It can only cause backward compatibility problems in the future if we need to change it.

10. Why do we need the KeyboardEvent for telling developers when input was masked / unmasked? Developers can't do anything in response to it, so there's really no point in notifying them.

[joeljfischer]

@ashwink11 Because the proposal is still in-review, your PR cannot be merged until after next Tuesday's meeting, when the PR will be returned for revisions.

The screen capability does not exist yet, it is part of a yet-to-be-implemented proposal (Widgets). The Screen Manager will have to be updated to account for the new keyboard properties in the PerformInteractionManager, primarily.

[ashwink11]

7. Is "maxsize" a mandatory parameter? If it's not, maybe we can remove it.
   "maxsize" to 99 is very large value for a number of keyboard layouts.

The Screen Manager will have to be updated to account for the new keyboard properties in the PerformInteractionManager, primarily.

As of now, "PerformInteractionManager" is part of the Example App. I guess we need not mention changes in Sample App in the proposal. Right?

[joeljfischer]

As of now, "PerformInteractionManager" is part of the Example App. I guess we need not mention changes in Sample App in the proposal. Right?

Sorry, I mis-wrote. The file is the SDLChoiceSetManager in the library.

[ashwink11]

5. Changes in ScreenManager with respect to KeyboardCapabilities cannot be determined as of now, as System capability is not yet implemented.

The Screen Manager will have to be updated to account for the new keyboard properties...

SDLScreenManager has setter method for SDLKeyboardProperties which in turn sets Keyboard properties in SDLChoiceSetManager. So, Keyboard properties are configured by mobile App and changes are propagated to SDLChoiceSetManager.

I think, only SDLKeyboardProperties needs changes for new parameters. I do not see any changes needed in Screen Manager.

10. It was added to make app aware if the intended effect of Keyboard Properties is applied. If it is not enabled, the app can decide if they want to continue with use case or use alternate flow for it.

[BrettyWhite]

@ashwink11 The managers take into account capabilities returned and then typically will dynamically configure properties based on what's available. Note how, for example, the SDLChoiceSetManager utilizes the displayCapabilities. The goal of the manager layer is to remove that complication from the developer while still performing the required logic.

[theresalech]

The Steering Committee voted to return this proposal for revisions, so that the author can revise based on the comments left on this issue. It was also specified that as the SDLChoiceSetManager has delegate callbacks and defaults that directly relate to Keyboard Properties (so it can do searches in keyboard, etc.), the SDLChoiceSetManager will need to take these changes into account. The Ford team agreed to work with the author on incorporating these changes.

[theresalech]

The author has revised this proposal based on Steering Committee feedback, and the revised proposal is now in review until July 16, 2019.

[joeljfischer]

7/8. The maxSize is a mandatory parameter. I think this should be comfortably updated to 1000. Think of if we add new keyboards for international users.

11. I think that <param name="numOfKeys" type="Integer" mandatory="true"/> should be renamed numConfigurableKeys.

12.

If the number of keys in "customizeKeys" array is more than customizable keys allowed, the SDL core should respond with "INVALID_DATA" and the info string should include a detailed message, that "customizeKeys exceeds the number of customizable keys in this Layout".

This should probably just be a warning and the first characters that would fit should be added.

13.

HMI should show default characters in the remaining customizable Keys if "customizeKeys" array is less than or equal to the customizable keys allowed.

Probably should add something like "that don't duplicate the app's special characters"?

14.

If a certain special character is not supported by the system the HMI should send "WARNING" response, with "info" text as " some symbols might not be supported by system". This will give a chance to use symbols that are supported and also inform the app about the system not supporting certain characters.

Because this is a WARNING, I would assume that would mean that the remaining characters would be shown.

[ashwink11]

7/8/11. I will make the changes as mentioned.

This should probably just be a warning and the first characters that would fit should be added.

In case, first characters that would fit are added in the keyboard, the app would not know, which characters are shown and which ones are not shown.
Also, to mitigate the above-mentioned issue if we think of returning character list which is added in the keyboard, it would introduce unnecessary complexity in HMI, core, and proxy.

It would be easier if the App receives INVALID_DATA, so as to be sure and control what's being shown on HMI Keyboard.

13. I will make the changes as mentioned.

14. Yes, remaining characters will be shown.

[theresalech]

The Steering Committee voted to accept this proposal with the revisions outlined in this comment from the author. The project maintainer also agreed to the author's point regarding INVALID_DATA (12).

[theresalech]

@ashwink11 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!

[theresalech]

Author has updated proposal to incorporate revisions, and implementation issues have been entered:

• Core
• RPC
• SDL HMI
• Generic HMI
• iOS
• Java Suite
• Server
• Manticore

[theresalech]

JavaScript Suite Issue: smartdevicelink/sdl_javascript_suite#340