[Accepted with Revisions] SDL 0216 Revisions - Widget Support

[theresalech]

Hello SDL community,

The review of "SDL 0216 - Widget Support (Revision)" begins now and runs through July 30, 2019. This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0216.

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

#788

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

#792

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]

Thanks for the proposal. We definitely agree that reusing DISPLAY is going to be an issue here, and it will need to be changed. I do think, however, that DISPLAYS is going to cause issues because it's so close to DISPLAY. What do you think about WINDOW as an alternative?

I think we also have a hesitation to not including SystemCapabilityManager changes in the Fall release because if we add DISPLAYS/WINDOW now, then we're locking ourselves in with that API, and we can't modify its behavior in a future release without causing a breaking change.

For example, if we released without the SystemCapabilityManager change, then a developer would have to check the connection's RPC spec version, and if it's < 6.0, use DISPLAY, and if >= 6.0, use DISPLAYS/WINDOW. I think we'd like to avoid that if we can.

Also, we do have an additional few weeks for app library implementation that we don't have for Core, so I think we can get the needed changes in there.

My suggestion for the SystemCapabilityManager is this: Change DISPLAYS to WINDOW, and deprecate the Android DISPLAY parameter. Then, when developers request the WINDOW capability, the SystemCapabilityManager will coerce the data from the DisplayCapabilities (if on a < 6.0.0 connection) into a DisplayCapability object and simply return that. It should be relatively simple and clean. What do you think?

[kshala-ford]

@joeljfischer thank you for the review. This proposal revision is very specific to the change on the RPC level. This proposal never had the depth needed to specify how the managers should be changed. For sure we can discuss the manager changes and I want to note the effort on #789 which is WIP (at the time writing this comment). I believe many questions are/will be answered in the new proposal but let's keep focus on the naming clash.

I do think, however, that DISPLAYS is going to cause issues because it's so close to DISPLAY. What do you think about WINDOW as an alternative?

The root system capability is an array of display capabilities. Planning long term (with multi display support) avoiding another clash I decided to rename it to DISPLAYS. The managers can be extended to provide convenient APIs for fetching window capabilities (out of scope though here but should be in #789).

I think we also have a hesitation to not including SystemCapabilityManager changes in the Fall release because if we add DISPLAYS/WINDOW now, then we're locking ourselves in with that API, and we can't modify its behavior in a future release without causing a breaking change.

This is the reason why #789 is being created. Keep in mind that DISPLAY already exist and due for code delivery was last Friday... We already locked ourselves but we do have a very little time left for modifications. I don't think anyone wants SystemCapabilityManager changes the way they are described in this proposal by just The window managers should be refactored to read window capabilities notifications as well as the deprecated parameters.. I mean... there are no "window managers". SDL 0216 proposal is not qualified to demand SystemCapabilityManager changes.

Reviewing the sdl_java_suite and sdl_ios repositories the major clash that has a risk of a breaking change was identified in the sdl_java_suite that extended SystemCapabilityType outside of the RPC specification which is really risky... #789 tries to resolve this issue but it requires the rename to an unused enum value (DISPLAYS). I hope it resolves some hesitation as I'm trying to avoid (at least minimize risk of) breaking changes.

For example, if we released without the SystemCapabilityManager change, then a developer would have to check the connection's RPC spec version, and if it's < 6.0, use DISPLAY, and if >= 6.0, use DISPLAYS/WINDOW. I think we'd like to avoid that if we can.

#789 will resolve this versioning issue by converting new DisplayCapability and old DisplayCapabilities objects dependent of the existance of RegisterAppInterfaceResponse.displayCapabilities or SetDisplayLayoutResponse.displayCapabilities or occurrences of OnSystemCapabilityUpdated.systemCapability.displayCapabilities or GetSystemCapabilityResponse.systemCapability.displayCapabilities. Conversion will be bi-directional providing forward and backward compatibility. For this proposal this will be out of scope but I hope this brief description and details described in #789 are helpful to understand.

Also, we do have an additional few weeks for app library implementation that we don't have for Core, so I think we can get the needed changes in there.

Excellent. #789 is of a high priority for me and I'm trying to get it review ready asap. I don't think the solution is very complex, it's rather easy. However it should be discussed which is why #789 exist. I am confident that the fall release for the libraries could include #789.

My suggestion for the SystemCapabilityManager is this: ...

The suggestion is in scope of #789 rather than this proposal. I would suggest to use DISPLAYS because the RPC returns an array of display capabilities. Out of scope here but for #789 my suggestion would be to include convenient methods to fetch window capabilities of a specific window ID instead of fetching them all in one method.

[joeygrover]

The changes submitted are what makes manager layer changes out of scope. The original proposal states the manager layer should be updated; that is why the specific line was changed to say manager layer modifications would come in a subsequent proposal. The feature itself will be near useless and at best unstable and confusing for app developers trying to navigate both the manager and RPC layers that have direct impacts on the same elements both in the library and on the module. This is largely due to the fact that at this point it is near impossible to use the SDL app libraries without the manager layers. I oppose creating such an API and poor developer experience. Therefore, I would advise to put a note in this PR change request stating that the aforementioned subsequent proposal must be agreed upon and included simultaneously with the original proposal in the proxy release. The Core related projects would not be affected by the new proposal so they will not be tied to its inclusion.

[kshala-ford]

The changes submitted are what makes manager layer changes out of scope. The original proposal states the manager layer should be updated; that is why the specific line was changed to say manager layer modifications would come in a subsequent proposal.

@joeygrover I'm sorry but you never replied with an assessment if you really expect manager changes with this very little and incorrect note in the original proposal. Could you please provide some feedback in to how the managers should be changed? I don't believe anyone want the managers to be messed up with changes not agreed by the steering committee. In fact the old sentence was mentioning window managers that don't exist. Technically the original proposal does not say anything that the system capability managers need a change. I'm really sorry but that single sentence should not be taken seriously compared to how much the project maintainers love to dig deep into the details of a proposal. The proposal was SC accepted in May and everyone had enough time to place the concern about the insufficient requirements for manager changes. Can we please stop going round in circles and try to find a solution? Thank you.

The feature itself will be near useless and at best unstable and confusing for app developers trying to navigate both the manager and RPC layers that have direct impacts on the same elements both in the library and on the module. This is largely due to the fact that at this point it is near impossible to use the SDL app libraries without the manager layers.

This is exaggregated and wrong because we are still able to use RPCs. Anyways this is not the point at all.

I oppose creating such an API and poor developer experience.

Me neither. I am on your side but this is what we have agreed in the original proposal but let's face the facts. There is no specification to the managers regarding widgets #789 was initiated the moment we faced the importance of the manager changes.

Therefore, I would advise to put a note in this PR change request stating that the aforementioned subsequent proposal must be agreed upon and included simultaneously with the original proposal in the proxy release. The Core related projects would not be affected by the new proposal so they will not be tied to its inclusion.

I agree with regards to release dates. However, the currently provided library code that includes all the RPCs can already be reviewed and tested. It may also be needed to test Core.

[joeygrover]

I don't disagree that the original proposal lacked the depth really required to make the changes to the manager layer, but in regards to the system capability manager it simply impossible to ignore its direct impact. This is a point we will have to agree to disagree on.

I am asking for the note and simultaneous inclusion with a secondary proposal. This was the solution which you seem to agree with, correct? As stated the core projects would not be affected and can be reviewed with the simple RPC additions in the proxy, but the proxy layer shouldn't be released without the manager layer additions however we decide on them in my opinion.

[joeljfischer]

I think we're in agreement that the manager changes will need to happen before the next proxy release, which just leaves the naming of the enum between DISPLAYS and WINDOW. I think I'm okay with DISPLAYS for future-proofing reasons, so long as DISPLAY is deprecated in the manager update before the next release.

[theresalech]

The Steering Committee voted to accept this proposal with revisions. The revisions will include specifying that managers will need to be updated to support the release of this feature within the proxy libraries, and this update will include the deprecating of DISPLAY. It was also noted during the meeting that the proposal describing manager implementations is review ready (#789), and determined that this new proposal would be brought into review and voted on during the 2019-08-06 Steering Committee Meeting.

[theresalech]

@kshala-ford please let me know once PR #788 has been updated to reflect the Steering Committee's agreed upon revisions. I'll then merge the PR so the proposal file is up to date, and make note of these changes on the respective implementation issues for SDL 0216. Thanks!

[theresalech]

PR has been updated to include agreed upon revisions, then merged so proposal markdown file is up to date. Comments have been left on implementation issues to call out these revisions:
SDL HMI
iOS
RPC Spec
Java Suite
Core