[Accepted with Revisions] SDL 0243 - Manager Update for DisplayCapability

[theresalech]

Hello SDL community,

The review of "SDL 0243 - Manager Update for DisplayCapability" begins now and runs through August 6, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0243-manager-update-display-capability.md

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

#797

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]

Happy to see this proposal come through. It seems well-designed and thought-out.

1. I'm not especially familiar with the Android SCM, but I'm not sure that this is quite correct:

Convert from DisplayCapabilities to DisplayCapability and vice versa

I must be missing it, but I don't see where the "visa versa" is described. I assume, however, that it's for if developers pull the deprecated DISPLAY etc. pseudo-capability. Is this necessary? I believe that Core will still be returning the DisplayCapabilities. If so, perhaps we can just convert from DisplayCapabilities to DisplayCapability and not visa versa.

2. In addition to - (nullable SDLWindowCapability *)windowWithID:(NSNumber<SDLInt> *)windowID; I would also suggest - (nullable SDLWindowCapability *)mainDisplayMainWindowCapabilities; since that's the primary use case.

[kshala-ford]

Thank you @joeljfischer. I really tried to assess the affected classes and effort but also provide at least some prototype grade code to quickly implement, all in a short amount of time. Therefore again please apologize if things look sort of incomplete or of a low quality in writing. I hope the overall concept is described well enough still.

|1 The Old>New conversion is clear so far I guess. The New>Old conversion is described only as some code in the bottom of the section Convert from DisplayCapabilities to DisplayCapability and vice versa method. I will copy the code from the proposal into this comment for easy reading (with the risk of a large comment...).

How to convert New to Old?

private DisplayCapabilities createDisplayCapabilities(String displayName, WindowCapability defaultMainWindow) {
    DisplayCapabilities convertedCapabilities = new DisplayCapabilities();
    convertedCapabilities.setDisplayType(DisplayType.SDL_GENERIC); //deprecated but it is mandatory...
    convertedCapabilities.setDisplayName(displayName);
    convertedCapabilities.setTextFields(defaultMainWindow.getTextFields());
    convertedCapabilities.setImageFields(defaultMainWindow.getImageFields());
    convertedCapabilities.setTemplatesAvailable(defaultMainWindow.getTemplatesAvailable());
    convertedCapabilities.setNumCustomPresetsAvailable(defaultMainWindow.getNumCustomPresetsAvailable());
    convertedCapabilities.setMediaClockFormats(Collections.singletonList(MediaClockFormats.CLOCK3)); // mandatory field...
    convertedCapabilities.setGraphicSupported(defaultMainWindow.getImageTypeSupported().contains(ImageType.DYNAMIC));
    
    return convertedCapabilities;

This method creates an old DisplayCapabilities with a specified display name and a windowCapability object of the default window. As an alternative the input param could be a DisplayCapability object instead. As this method is private it's actually a development detail.

When is this method used?

Whenever the library receives a system capability RPC with type DISPLAYS the method updateCachedDisplayCapabilityList is called on Java and sdl_updateCachedDisplayCapabilityList on iOS. At the end of this very method the following code is executed:

    setCapability(SystemCapabilityType.DISPLAYS, Collections.singletonList(newDefaultDisplayCapabilities));

    WindowCapability defaultMainWindowCapabilities = newDefaultDisplayCapabilities.getWindowCapabilities(PredefinedWindows.DEFAULT_WINDOW); // assume the function exist returning window capability of a specified window, PS: manager method getWindowCapability could be used instead
    // cover the deprecated capabilities for backward compatibility
    setCapability(SystemCapability.DISPLAY, createDisplayCapabilities(newDefaultDisplayCapabilities.getDisplayName(), defaultMainWindowCapabilities));
    setCapability(SystemCapability.BUTTON, defaultMainWindowCapabilities.getButtonCapabilities());
    setCapability(SystemCapability.SOFTBUTTON, defaultMainWindowCapabilities.getSoftButtonCapabilities());
}

This will ensure DISPLAY will be set to the latest DisplayCapability object that is converted to an old DisplayCapabilities object.

Why this effort?

The deprecated params (displayCapabilities etc.) are all non-mandatory. In the case an OEM chooses to upgrade to Core 6.0 and stops sending the capabilities in the RAI response the library can support backward compatibility. In case an app developer's app is using DisplayCapabilities but is lazy to upgrade to the new DisplayCapability all what's needed is to upgrade the library version to get a converted DisplayCapabilities object.

|2 Agree. The predefined window name is DEFAULT_WINDOW. should we call the method
-(SDLWindowCapability *)defaultMainWindow and public WindowCapability getDefaultMainWindow()?

[joeygrover]

Overall I think everything looks good with the proposal. I only have a few minor items.

3.

In order to avoid clashes with the mobile API the enum should not be extended any further without adding the enum values to the mobile API.

If we can't add values to that enum then it will not be possible get those capabilities outside the GetSystemCapability usage of the enum, from the Java Suite's SCM as it uses a generic getter. Though, I don't see us adding many if any in the future. I would prefer this language be altered in a way to not conflict with SDL-0079. The reason the items were added to the enum file was to provide a uniform API to developers to obtain different types of capabilities as well as creating the "fail fast" naming conflicts that we ran into. The method was modeled after Android's context.getSystemService(String) If the SystemCapability manager accepted values that were not included in the enum initially, but added later it would result in the issue we found with DISPLAY and developers would be experiencing runtime class cast exceptions. As a side note, we have added PRERECORDED_SPEECH in this next release, as it was a missing capability that has been in the iOS SystemCapabilityManager.

So maybe something like In order to avoid clashes with the mobile API the enum should only be extended with extreme caution when absolutely necessary or by adding the enum values to the RPC Spec.

The other option would be adding the other values to the enum, extending the GetSystemCapability method to return the values on the fly too.

4.

The screen managers will be affected by the deprecations hence should upgrade to the new display capability.

So to avoid any confusion/assumptions being made, this proposal does intend to have the screen manager and supporting managers, (SoftButtonManager for example), updated to use the new DISPLAYS capability when implemented, correct?

And it does NOT include changes to support multiple windows to the manager layer yet, which would have to be a later proposal, right?

[kshala-ford]

|3 I believe SDL-0079 is causing more trouble than it solves. The generic getter requires to change an enum which is specified somewhere else. The iOS SCM does not have this problem. It simply provides getters per capability type and it has the benefit of types avoiding any cast operation and issues during runtime. We should not continue to manipulate the mobile API types without changing the mobile API. I think this will be a critical issue implementing the proposal SDL-0234 Proxy Library RPC Generation. May be we should re-think how the SCM should work and improve this manager ...

|4 This is true. This proposal only includes SCM update and existing ScreenManager update. This is due to the need of this crucial change before releasing Core 6.0. It does not include manager APIs to create widgets. You could use the RPCs (CreateWindow, DeleteWindow, Show) manually but it's very inconvenient for developers. Not saying I haven't planned to propose a manager update for widgets.

Here's my idea in hope it's a good kick-off to think about:

1. My plan was to have the ScreenManager being extended with an additional constructor. This constructor params would match the CreateWindow params. With a specific window ID, the screen manager uses this window ID to send Show but also listens to DISPLAYS and window capabilities only for the specified window ID.
2. The lifecycle manager is extended to hold an array of widget (screen) managers.
3. The SDL lifecycle manager is extend with a createWidget method that also has params matching CreateWindow. the lifecycle manager sends the CreateWindow request and asynchronously returns an instance of ScreenManager if CreateWindow returns SUCCESS. The instance is also added to the array.
4. A method called deleteWidget has a param to a ScreenManager ref. A successful deletion will remove the instance from the array.

As of right now It'll be out of scope of this proposal. We can discuss widget screen managers separately or in this issue/proposal. I want to note that adding this additional feature can cause a much longer delay to the mobile releases. The risk is that nothing of this proposal will be released in the libraries together with Core 6.0.

[joeygrover]

3.

I believe SDL-0079 is causing more trouble than it solves.

I believe this is the first issue we've had.

The iOS SCM does not have this problem.

The iOS SCM should have actually followed the proposal or created an alternate proposal as it created a public API that differed from the proposal that introduced the manager.

May be we should re-think how the SCM should work and improve this manager ...

I think we both know that we have different opinions on the generic getters and likely are not going to reach a middle ground. I prefer the generic getters as it is a common pattern in Android as Java can be a very verbose language. I'm a proponent of creating features with less lines of code. I would also like to note that creating internal enums and RPCs was not introduced with with SDL-0079, there was already precedence in the Android SDL library for doing so.

However, this is not a hill I'm willing to die on. If the author can provide an alternative solution I will be open to it. The issue with not allowing the enum to be expanded internally is that it then hinders the public API for the Android SCM as using that expanded enum is the only way to access the capabilities through the manager. Therefore either the statement should be changed to not limit the existing API (which the author has expressed their dissent), or an alternative solution be provided. I believe the author could use the iOS SCM as a basis to provide the solution so that the managers would have matching public APIs and preventing the imposed limitations from breaking the current public API's future usage.

4. Sounds good. I just wanted to make sure everyone was on the same page. While I would really like for the screen manager to be updated to provide a more intuitive API for the widgets feature, I understand that this proposal does not include those changes, and there will have to be another proposal to discuss them.

[joeljfischer]

1. Thanks for the rundown of the why. That clears it up for me.

2. It would probably be a property? I'm not sure I like the defaultMainWindow naming since we're not returning a window. Perhaps defaultMainWindowCapability?

3. Another alternate solution would be to create a new SystemCapabilityManager specific enum type containing the SystemCapabilityTypes and the alternate ones not specified in the MOBILE_API, like SystemCapabilityManagerCapabilityType or something, y'know, less verbose.

With that said, I'm okay with it staying as is, because those are unlikely to expand outside of MOBILE_API changes. The GetSystemCapability pattern seems to be working well and we'll probably keep using it.

4. I have a few nitpicks with the plan laid out there (because when do I not) but overall it seems about right. I agree that it would be nice to have this in the next release, but understand that that is not going to be possible due to resources.

[theresalech]

The Steering Committee voted to defer this proposal, keeping it in review until our next meeting on 2019-08-20, to allow the author time to respond to the comments from the Project Maintainer.

[kshala-ford]

1. 👍

2. 👍 Okay. defaultMainWindowCapability it is

3. The proposal won't solve this topic but I'm OK to change the sentence to Joey's suggestion:

In order to avoid clashes with the mobile API the enum should only be extended with extreme caution when absolutely necessary or by adding the enum values to the RPC Spec.

4. 👍 @joeljfischer please feel free to send me your ideas to this feature. I will start to write a new proposal as soon as I can and would like to include your feedback from the beginning.

[bilal-alsharifi]

Thanks, @kshala-ford glad to see details for how the SystemCapabilityManager will be updated to handle new DISPLAYS capabilities. I just would like to add that we may need to also update the ScreenManager in this release to monitor only OnHmiStatus for the main display. The ScreenManager doesn't work well when OnHmiStatus for multiple windows are received.
Because currently, for example, if a developer is using the ScreenManager and if CreateWindow RPC is sent, the ScreenManager will receive OnHMIStatus with level NONE for the newly-created window. Which will block the ScreenManager from sending any SHOW RPCs until OnHMIStatus not NONE is received again.
There are multiple other scenarios where the ScreenManager will not work correctly because of the OnHmiStatus updates for widget windows. I am wondering if we can include that in this proposal as well.

[kshala-ford]

Hello @bilal-alsharifi. It's a very good point. You are right and I would rate this as a bug if the libraries would behave the way you described. I would consider this being a task to implement in smartdevicelink/sdl_ios#1350 and smartdevicelink/sdl_java_suite#1122 so that this issue is resolved from the beginning. The implementation for the ScreenManager should be to add early return in OnHMIStatus listener whenever windowID is not omitted and it's not set to 0 (PredefinedWindows.DEFAULT_WINDOW).

[bilal-alsharifi]

Thanks, @kshala-ford . That sounds good to me as well. We can include that in the widgets implementation PRs to ensure that ScreenManager works with widgets from the beginning.

[theresalech]

The Steering Committee voted to accept this proposal with the following revisions:

• In addition to- (nullable SDLWindowCapability *)windowWithID:(NSNumber<SDLInt> *)windowID; , include - (nullable SDLWindowCapability *)mainDisplayMainWindowCapabilities; with property defaultMainWindowCapability
• Change “In order to avoid clashes with the mobile API the enum should not be extended any further without adding the enum values to the mobile API.” to “In order to avoid clashes with the mobile API the enum should only be extended with extreme caution when absolutely necessary or by adding the enum values to the RPC Spec.”

[theresalech]

@kshala-ford please advise when a new PR has been entered to update the proposal to reflect the agreed upon revision. I'll then merge the PR so the proposal is up to date, and submit issues in the respective repositories for implementation. Thanks!

[theresalech]

Proposal has been updated to reflect agreed upon revisions, and implementation issues have been entered:
iOS
Java Suite