smartdevicelink/sdl_evolution

[Accepted with Revisions] Revise SDL-0238 Keyboard Enhancements

jordynmackool opened this issue · 7 comments

jordynmackool commented

Hello SDL community,

The review of "Revise SDL-0238 Keyboard Enhancements" begins now and continues through February 02, 2021.

This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0238.

The pull request outlining the revisions under review is available here:

#1117

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

#1119

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,
Jordyn Mackool

Program Manager - Livio
jordyn@livio.io

joeljfischer commented

1. It's a little hard to keep track of differences with everything next to each other as it is. I think KeyboardProperties.customKeys has two descriptions right now.

2. I think that to prevent confusion between KeyboardCapabilities and KeyboardCapability we could perhaps change KeyboardCapability to KeyboardConfiguration?

jacobkeeler commented

@joeljfischer

  1. customKeys does have two descriptions, but this was also the case before the revision. There are other parameters in the spec with multiple <description> tags (see limitedCharacterList) and these are just treated as multiline descriptions. That said, I could just change this to use a single multiline <description> tag, if that's preferable.
  2. I think I was following the pattern used by app services for this situation (AppServiceCapabilities vs. AppServiceCapability), though it doesn't look like this naming scheme is used elsewhere in the spec. I still think this struct should include "capability" in its name, so perhaps KeyboardLayoutCapability (or KeyboardLayoutCapabilities) would be preferable?
joeljfischer commented

1. I think that would probably be preferable. I hadn't noticed anything with two descriptions before, but I guess it does happen. It's a bit odd though.

2. I'm okay with that.

jacobkeeler commented

Alright, so as of now the additional revisions planned are:

  1. Change customKeys to use a single <description> tag
  2. Rename KeyboardCapability to KeyboardLayoutCapability
jordynmackool commented

The Steering Committee voted on 2021-02-02 to accept the revised proposal with the revisions summarized in this comment.

jordynmackool commented

@jacobkeeler please advise when the PR has been updated to reflect the agreed-upon revisions. I'll then merge the PR so the proposal is up to date, and comment on the already created issues for the impacted repositories. Thanks!

jordynmackool commented

The proposal has been updated based on the Steering Committee's agreed-upon revisions. Comments have been left on implementation issues to reference revisions: