Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Accepted with Revisions] SDL 0216 - Widget support #664

Closed
jordynmackool opened this issue Jan 23, 2019 · 55 comments
Closed

[Accepted with Revisions] SDL 0216 - Widget support #664

jordynmackool opened this issue Jan 23, 2019 · 55 comments

Comments

@jordynmackool
Copy link
Contributor

jordynmackool commented Jan 23, 2019

Hello SDL community,

The review of the revised "SDL 0216 - Widget Support" begins now and runs through May 14, 2019. Previous reviews of this proposal took place March 20 - April 9, 2019 and January 23 - February 26, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0216-widget-support.md

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

#664

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

@jordynmackool jordynmackool changed the title [SDL 0216] - Widget support SDL 0216 - Widget support Jan 23, 2019
@Jack-Byrne

This comment has been minimized.

@Jack-Byrne

This comment has been minimized.

@Jack-Byrne

This comment has been minimized.

@Jack-Byrne

This comment has been minimized.

@Jack-Byrne

This comment has been minimized.

@Jack-Byrne

This comment has been minimized.

@kshala-ford

This comment has been minimized.

@BrettyWhite

This comment has been minimized.

@kshala-ford

This comment has been minimized.

@BrettyWhite

This comment has been minimized.

@joeygrover

This comment has been minimized.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jan 29, 2019
@jordynmackool
Copy link
Contributor Author

The Steering Committee voted to reject this proposal on 2019-01-29 based on the amount of feedback given in the comments. The author plans to rework the proposal in a new format and resubmit the updated proposal for review.

@jordynmackool jordynmackool changed the title SDL 0216 - Widget support [Rejected] SDL 0216 - Widget support Jan 30, 2019
@jordynmackool jordynmackool reopened this Feb 20, 2019
@jordynmackool jordynmackool changed the title [Rejected] SDL 0216 - Widget support SDL 0216 - Widget support (revisions) Feb 20, 2019
@jordynmackool jordynmackool changed the title SDL 0216 - Widget support (revisions) SDL 0216 - Widget support Feb 20, 2019
@smartdevicelink smartdevicelink unlocked this conversation Feb 20, 2019
@joeljfischer

This comment has been minimized.

@kshala-ford

This comment has been minimized.

@joeljfischer
Copy link
Contributor

joeljfischer commented Apr 9, 2019

  1. I'm strongly against using AppHMIType here. I think using AppServiceType is much more useful for a few reasons. First, with the hmi type system, the head unit only knows of two possible active apps: media and navigation. This precludes the possibility of the head unit having a single "weather" widget (or when app services expands, a "voice assistant" widget, etc.) with the user's favorite service. I think using app services here gives all the same benefits, plus more (such as automatic redirection of other RPCs like SendLocation, and remote control ButtonPress).

    I'm having a bit of trouble fully parsing your latest comment on this issue, but it appears that your solution is to limit apps to essentially one widget unless they support multiple AppHMITypes. I would actually be more comfortable with simply limiting apps to a single widget period for now, until we can better figure out a way to support multiple widgets. For example, in the current proposal if an app is a MESSAGING HMI type and an INFORMATION HMI type, and provides widgets for both, how does the system know which widget to present in the situations I provided S2 or D1 above (copied below as well)?

There's are basically two possible user experiences for widgets: static placement and dynamic placement. Static placement is when the user selects a widget to be placed in a slot and covers several possible use-cases:

a. S1 - The user selects from a menu of all possible widgets to place it into a slot.
b. S2 - The user selects the app they want to place into a slot and the system determines which widget from the app to actually place into that slot.

Dynamic placement covers several possible use-cases:

a. D1 - The system chooses which widget to place in a slot based on an app being recent (e.g. the user went from an app screen to the main screen and the system wants to place that app's widget in a slot. Similar to iOS and Android recent app screens but with widgets.
b. D2 - The system chooses which widget to place in a slot based on the active app for a given app service type. e.g. There's a slot for the active navigation / music / weather app and the system places the active app's widget into that slot.

I think it might be simpler to only allow one widget per app (per display) for now, and expand later in a separate proposal when we can think through it in a more focused manner (we'd probably have to remove the system capability for sending back max screens / windows for now so that OEMs don't just change the value and we end up with a Wild West of OEM UX).

Alternatives:

a. Adding a new RPC to change the primary window / screen for a display per window / screen type.
b. Using a DeleteScreen / CreateScreen RPC to swap out for a PrimaryWidget ID

I don't think anything else would work at all, and (b) would suck UX-wise pretty bad (e.g. the currently displayed primary widget is removed from its slot on the head unit and the new one isn't added back into that slot). I think (a) is the only viable alternative.

So, my suggestion is this:

<function name="CreateScreen" messagetype="request" since="5.x">
    <!-- Everything that's already there -->
    <param name="appServiceAssociation" type="AppServiceType" mandatory="false">
        <description>If provided, this widget should be used in dynamic situations when this app is the active app for the service provided. e.g. If the app is the active "media" app service, this is the widget you want displayed on the system if the system only picks one for a "media" widget slot. This overrides the `MakePrimaryScreen` RPC, which is used in dynamic situations in which the system wants only one widget representing your app. If the app has not provided a manifest for this service type already, this RPC will be rejected with resultCode `DISALLOWED`. If this parameter is not provided, the widget will not be associated with a service.</description>
    </param>
</function>

<function name="MakePrimaryWidget" messageType="request" since="5.x">
    <param name="screenID" type="Integer" mandatory="true">
        <description>When sent, the widget associated with the screenID provided will become the primary widget for this app. If the system wishes to show only one widget from this app, and this request doesn't involve an active app service, or the app does not provide a widget associated with an app service (see `appServiceAssociation` in `CreateScreen`, then the widget specified here will be used.</description>
    </param>
</function>

While it's not ideal to use a separate RPC for this functionality, all other considerations I could think of had large downsides. The managers could hide away this RPC and make it easy for app developers to use.

New Issue (17): It appears that a widget cannot use the SubscribeButton RPC. Does that mean that a MEDIA widget cannot provide play / pause buttons on their widget? Is that something we should include?

@theresalech theresalech changed the title [In Review] SDL 0216 - Widget support [Returned for Revisions] SDL 0216 - Widget support Apr 10, 2019
@theresalech
Copy link
Contributor

The Steering Committee voted to return this proposal for revisions, as the author has been working on updating the proposal to include the agreed upon items (1, 2, 3, 4, 5, 6, 7, 9, 10, 11, 12, 13, 14) in PR #698. The Steering Committee will continue to discuss and work through the open items (8, 15, 16, 17) once the revised proposal is back in review.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Apr 10, 2019
@theresalech theresalech changed the title [Returned for Revisions] SDL 0216 - Widget support [In Review] SDL 0216 - Widget support May 8, 2019
@theresalech
Copy link
Contributor

The author has revised this proposal to include the Steering Committee's requested revisions, as listed in this comment. The revised proposal is now in review until 2019-05-14.

@smartdevicelink smartdevicelink unlocked this conversation May 8, 2019
@joeljfischer
Copy link
Contributor

joeljfischer commented May 9, 2019

|8. I remain in favor of "mirror" terminology.

|16. I'm in favor of your serviceType solution for dynamic widgets. It permits app types like weather, and in the future voice assistants, or VOIP. It also allows deeper integration into the system (navigation, media, etc.). And it will allow us to create automatic widgets for developers if they so desire using service data.

It solves a lot of problems, though it won't solve the problem of pulling one widget per app if, for example, you wanted to use a "recent apps" screen using widgets. Some possible solutions I thought of:

  1. Add a primaryWidget flag in addition to the serviceType parameter.
  2. Ignore that use case for now.
  3. Tell devs / core that the first widget they create will be the one used in that use case.

I think I lean to (3). If they DeleteScreen that widget, the 2nd (or next) widget created would be used.

|17. I didn't see any response on this one. What do you think about widget button subscriptions for media apps?

|18. As a note, it appears that mandatory parameters are added to existing HMI_API RPCs (such as OnAppActivated. Therefore, this change would be a major version change for Core to 6.0. I did not see this noted in the proposal.

|19. Adding dayColorScheme and nightColorScheme to Show may present some problems. Currently, the developer is only allowed to update their color scheme when they change templates using SetDisplayLayout. I would assume the same restriction would apply when they moved over to Show? e.g. The core implementors must reject a Show request that updates dayColorScheme or nightColorScheme when layout is not also set.

|20. I don't see it mentioned in the proposal under the Window capabilities section, but the DisplayCapability struct should only update with changed parameters. I'm unsure if the WindowCapability struct should do the same.

|21. I don't believe there's a way to see if soft buttons support text, and it appears that your example widget designs don't support images (and this may be common). I think this is probably a very good time to add the ability to add a new TextFieldName for soft button text (to determine length of soft button strings), and to add a new SoftButtonCapabilities parameter, textSupported. It will probably be important for widget soft buttons.

|22.

Moving window metadata will require more effort from OEMs and app consumers to implement this feature. The metadata needs to be sent twice, in the responses but also in the system capability notification.

Can you explain this more? I assume you mean the RegisterAppInterfaceResponse?

@kshala-ford
Copy link
Contributor

|8 All I can offer is to rename the parameter to duplicateFromWindowID. Again: We duplicate the request for the window. Mirror implies the exact same presentation of the data and last "mirror" is too close to mirror projection features.

|16 my biggest concern with using the service enum is the lack of permission management. I could not find any reference to policies with app services. To my understanding a media app could easily publish weather service data? I believe we anyway need to keep AppHMIType aligned with AppServiceType. It would be very helpful if you or Joey could shed some light on this topic. If there is no alignment here I would rather see this proposal to be accepted without this additional feature until we have found a solution.

It also allows deeper integration into the system (navigation, media, etc.)

Can you explain this deeper integration?

And it will allow us to create automatic widgets for developers if they so desire using service data.

Same here? More explanation please. Are you planning to auto-create widgets from the libraries which get filled with service data?

It solves a lot of problems, though it won't solve the problem of pulling one widget per app if...

I don't think a primary widget is needed. Also I don't want to add a widget specific parameter to CreateWindow which in future is supposed to be used for main windows as well. We could add to PredefinedWindows new elements like PRIMARY_WIDGET. The ID could be 1 or any other ID that we specify. The primary widget can optionally be associated with a service type. If the developer creates a widget with this ID it would be selected to appear in the recent app widget area except the widget is already visible as a service based widget or because the user has pinned the widget to be constantly visible. It all depends on how the HMI implements widgets; all we should do is to give a hint to the HMI. The developer cannot create another "primary widget" as it would fail with INVALID_ID.

|17 I saw your note but I had no chance to respond to it outside of the proposal. (Hard) button subscription is a system RPC. As a media app when you subscribe to PLAY_PAUSE you will receive hard button press on > || if you're the active source. In SYNC3 (and I think sdl_hmi does so, too) we add some kind of hard coded soft button in the center of the media template. We would not change anything to SDL regarding button subscription.

|18 that's a mistake. The description mentions "if this param is not included...". Consider the parameters for OnAppActivated and OnAppDeactivated as optional.

|19 Why would they? I don't see any need to do so? It could be easily abused by setting the same template. Why add a barrier if it's so easy to work around it?

|20 I'm not sure what you mean. Right now only the window that changes should be reported. See here

|21

I don't believe there's a way to see if soft buttons support text

I think until now we expect soft buttons to always support text.

and it appears that your example widget designs don't support images (and this may be common)

Or course they do support graphics. I don't see why widgets shouldn't support graphics.

I think this is probably a very good time to add

I have focused on restructuring the existing capabilities without adding new elements. I don't see the need or importance for your proposed addition but you can feel free to create another proposal for it. The current widget proposal has a lot of content which is not in scope of widgets and I don't want to add more things to it which are not related to widgets.

|22 RAI response and SetDisplayLayout response. The OEM has to provide display capabilities and window capabilities in two different ways at least until core 6.0. I think this is more effort. Of course it's not too much work but I wanted everyone to understand that the old and new structure must be supported by OEMs.

@joeljfischer
Copy link
Contributor

|16

my biggest concern with using the service enum is the lack of permission management. I could not find any reference to policies with app services. To my understanding a media app could easily publish weather service data?

I just double checked the developer portal, and there creating app services does require requesting permissions via policies.

Can you explain this deeper integration?

Sure, I just meant a more formalized way to describe the "current system" media, navigation, weather, etc. app. This is similar to what currently exists in Sync 3 with the current media app data being presented on the Home and Audio screens, but more formalized via service data, which prevents the possible case of the media app going to a different template and then the media data is no longer mainField1 / 2 / etc. (as an example).

Are you planning to auto-create widgets from the libraries which get filled with service data?

I think that would certainly be possible and even relatively easy if there were a proposal to do so in the future.

We could add to PredefinedWindows new elements like PRIMARY_WIDGET.

The main downside I see to this, as opposed to my other suggestion of using the "first" created, and if the "first" is deleted then falling back to the "second" created, is that if the PRIMARY_WIDGET is deleted, and the developer wants another current widget to take its place (let's say it has id 2), then the developer needs to delete and re-create that widget into the PRIMARY_WIDGET slot. It's not ideal, but it's also not terrible. I think I can live with it.

|17. The concern that I have here is the ability for a widget to display the "soft" subscription buttons that e.g. Sync displays (this usually happens on the main window when the subscription happens). I'm fine with not doing that and simply having the developer make soft buttons that do the same thing.

|18. 👍

|19. The barrier can't be worked around because Core prevents it from changing the color scheme if you set the same template that is already in use.

|20. I see that note now. 👍

|21.

and it appears that your example widget designs don't support images (and this may be common)

Or course they do support graphics. I don't see why widgets shouldn't support graphics.

I typed "images" when I meant "text" 🤦‍♂

I do think that this is an important thing for good widget support. If developers have no way of knowing that a soft button doesn't support text then they may end up with creating soft buttons that work on some systems but not others. These features are important for widgets to work well on a wide variety of systems, and not just one OEM's. If OEM 1 supports text in a widget soft button and OEM 2 does not, and the developer tests only on OEM 2's system, then the app's support for OEM 1's system will be poor or broken.

Because widgets makes an implicit change to have soft buttons that don't support text, there should be a corresponding capabilities change to describe whether a soft button supports text. This is a small change but I believe it is important.

|22. 👍

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented May 13, 2019

  1. Couple of things:

i) Core will validate against policies the app service type an app is allowed to publish. After this, if a published media service app sends weather data in the OnAppServiceData notification, it will be up to the consumer app to know that data is wrong.

ii)

Are you planning to auto-create widgets from the libraries which get filled with service data?

This is already possible when using the HMI as an app service consumer and we have started implementing this as a feature into the Generic HMI.

Edit: misunderstood the question.

iii) CreateWindow RPC has a param widgetType of type AppServiceType. Note that AppServiceType is marked as Documentation only since the app services RPCs use type String for all of the service types. Should widgetType be changed to be type String to keep consistency?

The reason for this in app services was that Core could not validate enums for future app service types that an older module does not know about. That part of app services does not really apply to widgets so if AppServiceType is kept as the type for widgetType, then the Documentation tag should be removed from AppServiceType in the spec.

@joeygrover
Copy link
Member

Just wanted to say thanks @kshala-ford for all the hard work you've been putting into this proposal. For me at least, the latest revision has a very understandable and comprehensive description of the solution.

| 8. I think this works. The argument we had was that the param originally sounded like it was duplicating the window not the update to the window; that's why mirror made more sense with that param name. But the suggestion helps clear that up. Other suggestions (in case you like one better, I'm indifferent at this point with at least the suggested renaming) duplicateUpdatesFromWindowID, copyUpdatesFromWindowID, duplicateWindowIDUpdates, duplicateUpdatesFrom, duplicateUpdatesFromID

| 16. I think something like PRIMARY_WDIGET or a flag for primary of that window type would be the best option. It gives the developer the clear instruction that this is what will be used and not rely on module integrations that may perform differently. If a primary window is deleted, it will not be replaced until the developer recreates a primary window. (Using window in general here in case there are new window types beyond MAIN and WIDGET that would follow a similar use case as WIDGET)

| 19. I believe in the proposal for template colors we specifically stated that the updates should only occur when a template is being changed. I would have to find the specific call out, but I know it was a discussion point. Another option would be creating a smaller struct that held both params to specifically call out that it should only happen during a layout change. (Ignore the name, it's not great)

<function name="Show" functionID="ShowID" messagetype="request" since="1.0">
 :
 :
   <param name="layoutUpdate" type="LayoutUpdate" mandatory="false">
    <description>
        If included the window should update the supplied layout configurations (layout type and color scheme)
    </description>
  </param>
</function>

<struct name="LayoutUpdate">
  <param name="layout" type="String" maxlength="500" mandatory="true">
    <description>
        Predefined or dynamically created window layout.
        Currently only predefined window layouts are defined.
    </description>
  </param>

  <param name="dayColorScheme" type="TemplateColorScheme" mandatory="false" />
  <param name="nightColorScheme" type="TemplateColorScheme" mandatory="false" />
</struct>

| 21. So we have to answer are widget SoftButtons required to support text? If yes, then they follow the same guidelines as other soft buttons on the system and we could supply a proposal later to have a system wide change. However, if there is a case we believe that will only support images/graphics, then we HAVE to include a new param in the SoftButtonCapabilities struct. Forcing a deve into a trial and error situation is never a good practice. It is a simple addition that doesn't have too much of an effect if not included.

<struct name="SoftButtonCapabilities" since="2.0">
: 
: 
    <param name="imageSupported" type="Boolean" mandatory="true">
        <description>The button supports referencing a static or dynamic image.</description>
    </param>

+    <param name="textSupported" type="Boolean" mandatory="false"  since="X.X">
+        <description>
+                The button supports the use of text. 
+                If not included, the default value should be considered true that the button will support text.
+        </description>
+    </param>
<struct>

@kshala-ford
Copy link
Contributor

I'm leaving out numbers we have agreed on. (17. 18. 20. 22.)

@JackLivio Regarding 16 iii) We can make it of type String, though enum would be handy for the app developer to know what to put into the parameter. Do we know about service types supported by the HMI? Is there some capabilities with a list of supported services?

|8. Can we agree on "duplicateUpdatesFromWindowID"?

|16 I think the actual use case is to have recent apps with widgets. Therefore I would suggest a new "PredefinedWindows" called RECENT_WIDGET named after the app name. The benefit of a predefined window is that the HMI can precreate it and return it in display capabilities after registration. I think this name fits better than having a primary widget. This predefined recent widget solves the problem of deleting and creating as it makes it obsolete. A new show can overwrite everything on the recent widget. What are your thoughts?

|19. In the color scheme proposal I found:

A SetDisplayLayout request with the same layout as is currently displayed will result in the current template to change color."

Either we do Joeys suggestion or we make "layoutUpdate" parameter optional.

| 21 Joeys suggestion of the parameter looks reasonable. If that's all we need we can easily accept this one for the revision.

@joeljfischer
Copy link
Contributor

joeljfischer commented May 14, 2019

|19.

In the color scheme proposal I found:

A SetDisplayLayout request with the same layout as is currently displayed will result in the current template to change color."

That is true, however it then says in the next sentence:

SDL Core should track the number of attempted SetDisplayLayout requests with the current template and REJECT any beyond the first with the reason "Using SetDisplayLayout to change the color scheme may only be done once."

So there may have been a little confusion there. The last sentence I quoted is what is implemented. I think we're in agreement that we'd have to do the same thing with the color scheme in Show as mentioned.

@kshala-ford
Copy link
Contributor

Yes. I'm sorry I haven't read and understand this sentence in the first place.

|19 still: I think I just found another issue with color scheme and the fact that it requires a template change. In the widget proposal there is this:

The color scheme parameters are related to the app's color scheme. Setting a new color scheme parameter will change the color scheme of all windows of the app. Color scheme parameters will ignore the window ID parameter.

saying that color scheme should be applied to the app and to all windows, but the new layout will only be applied to the specified window (not to all windows).

Here are my suggestions (in hope we can agree to a solution for today):
a. Have color scheme per window. The RAI color scheme will be applied to the main window and will be the default for new windows/widgets. Show with new color scheme is always window specific and doesn't update the defaults. In this case I would accept Joey's suggestion.
b. Allow color scheme change without template change. A single color scheme is applied to all windows. The rule to REJECT would be removed. In this case I would keep the params as they are ("layout" param needs to be mandatory="false" though).

@Jack-Byrne
Copy link
Contributor

@kshala-ford

16 iii) If the enum is preferred then we could just remove the documentation tag from AppServiceType. Also there are no capabilities for what AppServiceTypes the HMI supports but it could be inferred from the rpc spec version core is using.

@kshala-ford
Copy link
Contributor

Thank you @JackLivio but I believe your original concern is valid, therefore in the RPC spec the parameter type should be changed to String. In the library implementation we can offer the enum though. This would ensure modern apps to allow use new enum values on older vehicles (e.g. VOIP might be added in future and an app uses it but some vehicles won't support VOIP).

@kshala-ford
Copy link
Contributor

Sorry for the spam but I wanted to inform you about the PR #729 which includes revisions as we have found an agreement to all items:

|8 Rename duplicateWindowID to duplicateUpdatesFromWindowID

|16 Add PRIMARY_WIDGET to PredefinedWindows. This predefined widget is used by the HMI whenever a single widget needs to represent the whole app (e.g. recent apps widget).

|16 iii) Change "widgetType" type from AppServiceType to String

|17 No action.

|18 Made windowID to optional for OnAppActivated and OnAppDeactivated

|19 Templates and color schemes are set with a struct (as Joey's suggestion). The struct is called TemplateConfiguration.

|20 no action

|21 textSupported flag is added to SoftButtonCapabilities (thank you Joey for your suggestion)

|22 no action

@theresalech
Copy link
Contributor

The Steering Committee voted to accept this proposal with the agreed upon revisions, described in this comment and included in PR #729.

@theresalech theresalech changed the title [In Review] SDL 0216 - Widget support [Accepted with Revisions] SDL 0216 - Widget support May 15, 2019
@smartdevicelink smartdevicelink locked and limited conversation to collaborators May 15, 2019
@theresalech
Copy link
Contributor

theresalech commented May 20, 2019

Proposal updated to reflect agreed upon revisions, issues entered:
Java Suite
iOS
Core
RPC
SDL HMI

@theresalech theresalech added the hmi label Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants