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] SDL 0268 - Main Menu Updating and Pagination #883

Closed
theresalech opened this issue Dec 4, 2019 · 15 comments
Closed

[Accepted] SDL 0268 - Main Menu Updating and Pagination #883

theresalech opened this issue Dec 4, 2019 · 15 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Dec 4, 2019

Hello SDL community,

The review of the revised "SDL 0268 - Main Menu Updating and Pagination" proposal begins now and runs through January 14, 2020. The original review of "SDL 0268 - Main Menu Updating and Pagination" took place December 4 - 10, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0268-main-menu-updating.md

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

#883

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

@Jack-Byrne
Copy link
Contributor

Since UpdateCommand is a notification shouldn't it be named OnUpdateCommand?

@kshala-ford
Copy link
Contributor

kshala-ford commented Dec 10, 2019

@joeljfischer I do support improvements to the app menu. Especially the performance to create the graphical app menu is not the best. I have some doubts to the suggested method though but let me try to start with some feedback to the proposal.

  1. As suggested by @JackLivio the notification should be OnUpdateCommand and OnUpdateSubmenu.

  2. <param name="updateArtwork" type="Bool" mandatory="false"> I don't think the param should be called udpateArtwork. Instead it should be updateIcon? Strictly speaking I need to ignore SDL-0267 but with the other proposal in mind updateImages might be the best naming if the parameter means that all images related to the command/submenu should be updated/uploaded.

  3. <param name="updateSubCells" type="Bool" mandatory="false"> from the RPC spec we are talking about commands and not sub cells. Therefore the naming should be updateSubCommands or similar.

  4. There's an accepted proposal that allows referring to non-existing images. SDL-0042 [Accepted] SDL 0042 - SDL must transfer RPC’s with invalid image reference parameters to the HMI #128 which would be a very good tool for your proposal. Apps would add commands and sub menus referring to images that are not uploaded with PutFile. Instead of asking to updateArtwork we could create a new RPC notification like OnFileRequest which tells the app that it's now time to upload the file.

  5. With regards to the first bullet point of your alternative considered I understand that it's not easy to find a solution with VR grammars attached to commands. I suppose VR grammars are already added separately with a separate cmdID?

Actually I am not sure if the proposal is the best solution to the performance issues. Downsides of the proposed solution are, that with pagination I expect the command list is set in pages. With the proposal you would still need to add all commands per menu. Saying if you have 100 commands in the main app menu you still need to send 100 AddCommand requests.

An alternative would be to allow apps to say how many commands should be contained in a menu (e.g. extending AddSubMenu with a param like numberOfEntries). Then the app should be notified what entries should be uploaded allowing the app to be more agile with the content it needs to fetch.

@joeljfischer
Copy link
Contributor

joeljfischer commented Dec 10, 2019

1. Yep, you're correct.

2. I'm okay with updateIcon(s) or updateImage(s).

3. I was considering the implementation of SDL-0148, which could have additional submenus beneath submenus. I know it's not implemented yet, but it is accepted, so I was trying to have more generic naming that could fit both sub-commands and sub-menus. What do you think?

4. I re-reviewed this proposal, and your alternative would work for artworks, however it doesn't work for sub-cells, and it's not extensible. I can add it as an alternative, if you'd like.

5. I'm sorry, I've read your question a few times and I'm not understanding it. Could you elaborate or re-phrase?

6.

Actually I am not sure if the proposal is the best solution to the performance issues. Downsides of the proposed solution are, that with pagination I expect the command list is set in pages. With the proposal you would still need to add all commands per menu.

This is true, however, it's not possible to support VR without sending the entire menu at once without pagination. If the menu doesn't have VR, then sending 100 AddCommands with only text, while sizable, is small enough that, in my opinion, it's not worth it to paginate. In other words, the three "bad" or "large" parts of a 100 item menu are images, sub-menus, and VR. This proposal solves the first two, but the design of the last requires the entire textual menu to be sent at once.

@kshala-ford
Copy link
Contributor

  1. would be a single RPC that would work for commands, submenus and even choices. I understand it doesn't support sub-cells but honestly I believe "updateSubCell" is basically equal to an "OnCommand" for sub menus. So I would suggest to use "OnCommand" also for sub menus or introduce a new RPC for "OnSubMenu".

  2. I'm saying that AddCommand supports VR only commands. So the question is: Does the menu manager automatically separate VR commands from menu commands?

  3. The downside of adding all commands in a (sub)menu is painful if you need to load the list from a backend because you need to load the whole list and then add all commands. Pagination per menu is an improvement and if we can add the ability with little effort I believe we should consider it especially when we are making changes in this area anyway.

  4. Should "updateSubCell" be true when selecting a sub menu? Or only when the menu is empty?

@joeljfischer
Copy link
Contributor

4. I disagree that this should be tied to OnCommand, which I'll address in 7.

5. Yes, there is a MenuManager and a VoiceCommandManager that address these separately.

6.

The downside of adding all commands in a (sub)menu is painful if you need to load the list from a backend because you need to load the whole list and then add all commands.

I don't see the relevance. I'm saying that sending 100 text-only AddCommands is going to be a relatively minimal amount of data – compared to sending an image, for example. It's worse for performance and usability, IMO, to send 10 commands, then 10 more, then 10 more, than it is to simply send all 100. I say this due to (a) the latency of sending the request / response with the new data, (b) reaching out to the backend, (c) the possibility of stale data. Sending 100 text-only commands is simply not very much data.

Second, it bears repeating that this breaks menu items with VR. That's a bad enough downside alone that it's not worth going down that path.

7. updateSubCells is intended to be true if the AddSubMenu command has no AddCommands with their parentID attached to that AddSubMenu, at whatever point the HMI wants those sub-commands. The HMI could choose to send the OnUpdateSubMenu with updateSubCells = true if (a) the user taps on the cell (as you suggested in your latest comment on 4), (b) if the user scrolls to a place where the AddSubMenu cell is onscreen, (c) the HMI could choose that all sub-commands be sent, but not sub-commands of those sub-commands (when SDL-0148 is implemented). There are probably other ways that the HMI could implement it.

@kshala-ford
Copy link
Contributor

@joeljfischer my suggestion is to add OnUpdateFile which asks the app to upload a file of a given name. This indicates that the app can use file names in RPCs such as AddCommand, AddSubMenu and Choice that are not uploaded at the time of using these RPCs a according to SDL0042. Also my suggestion is to add OnSubMenu which is send to the app when tapping on a SubMenu. This RPC notification should have a bool parameter like updateSubCells that behaves as you describe in 7.

This suggestion provides the exact same result as in the proposal but adds support for choices and is generic for any place where images like widgets and graphics. It also adds support to notify when a submenu entry is pressed. It eliminates the need to extend AddCommand and AddSubMenu with hasIcon because it uses existing features from SDL0042

@joeljfischer
Copy link
Contributor

There are a few downsides to what you said:

Also my suggestion is to add OnSubMenu which is send to the app when tapping on a SubMenu. This RPC notification should have a bool parameter like updateSubCells that behaves as you describe in 7.

This means that the HMI can only have the menu be sent when the sub-menu cell is tapped and not in the other circumstances I mentioned above in 7.

This suggestion provides the exact same result as in the proposal but adds support for choices and is generic for any place where images like widgets and graphics.

We would need to add additional capabilities items. This isn't a big downside and is doable.


With that said, I was not aware of the implementation of SDL-0042 in Core 5.0. That certainly opens up these additional options. I think that altering OnUpdateCommand to OnUpdateFile is okay with me, barring any additional issues I see in implementation. However, I do not want to restrict HMIs to only ask for the submenu when the submenu cell is selected. To me, that's too restrictive. I prefer my original OnUpdateSubMenu, without the updateArtwork parameter that can be sent when the submenu cell is tapped or at any other time the HMI deems necessary.

@kshala-ford
Copy link
Contributor

kshala-ford commented Dec 10, 2019

I prefer my original OnUpdateSubMenu, without the updateArtwork parameter that can be sent when the submenu cell is tapped or at any other time the HMI deems necessary.

👍 that's a good point. I agree to this.

I want to make things clear regarding the mobile API changes:

<function name="OnUpdateFile" functionID="OnUpdateFileID" messagetype="notification" since="x.x">
    <description>This notification tells an app to upload and update a file with a given name.</description>

    <param name="fileName" type="String" maxlength="255" mandatory="true">
        <description>File reference name.</description>
    </param>
</function>

<function name="OnUpdateSubMenu" functionID="OnUpdateSubMenuID" messagetype="notification" since="x.x">
    <description>This notification tells an app to update the AddSubMenu or its 'sub' AddCommand and AddSubMenus with the requested data</description>

    <param name="menuID" type="Integer" minvalue="0" maxvalue="2000000000" mandatory="true">
        <description>This menuID must match a menuID in the current menu structure</description>
    </param>

    <param name="updateSubCells" type="Bool" mandatory="false">
        <description>If not set, assume false. If true, the app should send AddCommands with parentIDs matching the menuID. These AddCommands will then be attached to the submenu and displayed if the submenu is selected.</description>
    </param>
</function>

<struct name="WindowCapability" since="6.0">
    <!-- New Parameters -->
    <param name="supportsDynamicMenus" type="Bool" mandatory="false" since="x.x">
        <description>If true, the head unit will send UpdateCommand and UpdateSubMenu notifications, and it will support updating menu cells by sending the same cmdID / menuID with new data. If not set, assume false.</description>
    </param>
</struct>

Different to what's proposed, the params hasIcon should not be added to AddCommand and AddSubMenu. This is my understanding to what we both prefer, do you agree?

Regarding the HMI API I just realize that you suggest requests and responses. Should they be notifications instead with direction HMI -> SDL?

@joeljfischer
Copy link
Contributor

OnUpdateFile.fileName maxlength should be 65535 because that's Image.value's maxlength. Besides that, I believe I'm good with your OnUpdateFile and modification to OnUpdateSubMenu.

Additional parameters would need to be added to WindowCapability, since more than 1 parameter will be added, I will add a struct to contain them. It will be something like:

<struct name="DynamicUpdateCapabilities" since="X.X">
  <param name="supportsDynamicSubMenus" type="Bool" mandatory="false" since="X.X">
    <description>If true, then OnUpdateSubMenu notifications will be sent from the module requesting AddCommands with parentIDs matching the menuID.</description>
  </param>
  <param name="supportsDynamicPerformInteractionImages" type="Bool" mandatory="false" since="X.X">
    <description>If true, then OnFileUpdate notifications will be sent from the module requesting PutFile information for PerformInteraction / CreateInteractionChoiceSet images.</description>
  </param>
  <param name="supportsDynamicMenuImages" type="Bool" mandatory="false" since="X.X">
    <description>If true, then OnFileUpdate notifications will be sent from the module requesting PutFile information for AddCommand and AddSubMenu icons.</description>
  </param>
</struct>

<struct name="WindowCapability" since="6.0">
    <!-- New Parameters -->
    <param name="dynamicUpdateCapabilities" type="DynamicUpdateCapabilities" mandatory="false" since="x.x">
        <description>Contains capabilities for dynamic updating features such as image and sub-menu requests from the head unit.</description>
    </param>
</struct>

Regarding the HMI API I just realize that you suggest requests and responses. Should they be notifications instead with direction HMI -> SDL?

I don't remember why I chose the request-response pair. It seems to me that it could be a notification.

@kshala-ford
Copy link
Contributor

Getting close to the meeting... what do you think about the idea to make the capabilities an ImageName array which could be called dynamicImageUpdateSupported

<param name="dynamicImageUpdateSupported" type="ImageType" array="true" mandatory="false" minsize="1">

@joeljfischer
Copy link
Contributor

I think you mean ImageFieldName and not ImageType. I considered a similar structure, however, it doesn't seem to work for AddSubMenu.menuIcon. The ImageFieldName.menuIcon description says that it relates to SetGlobalProperties.menuIcon, not the AddSubMenu version. Perhaps cmdIcon relates to both? It doesn't state that in the description though.

Second, we don't have defined behavior for fields outside of AddCommand, AddSubMenu and Choice. Implementing this for primaryGraphic for example, isn't super obvious. Should the managers always only send the Image struct and never upload the image until the head unit asks for it in that situation? I didn't want to try to define that in this proposal.

@theresalech theresalech changed the title [In Review] SDL 0268 - Main Menu Updating and Pagination [Returned for Revisions] SDL 0268 - Main Menu Updating and Pagination Dec 11, 2019
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will update based on this comment and this comment, and also work to make WindowCapability more flexible. It was also noted on the call that PutFile has a maximum length of 255.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Dec 11, 2019
@theresalech theresalech changed the title [Returned for Revisions] SDL 0268 - Main Menu Updating and Pagination [In Review] SDL 0268 - Main Menu Updating and Pagination Dec 18, 2019
@theresalech
Copy link
Contributor Author

The author has revised this proposal to incorporate the requested revisions from the Steering Committee. The revised proposal is now in review and will be voted upon during the 2020-01-14 meeting.

@smartdevicelink smartdevicelink unlocked this conversation Dec 18, 2019
@theresalech theresalech changed the title [In Review] SDL 0268 - Main Menu Updating and Pagination [Accepted] SDL 0268 - Main Menu Updating and Pagination Jan 15, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee fully agreed to accept this proposal.

@smartdevicelink smartdevicelink locked as resolved and limited conversation to collaborators Jan 15, 2020
@theresalech
Copy link
Contributor Author

theresalech commented Jan 15, 2020

Implementation Issues Entered:
RPC
Core
SDL HMI
Generic HMI
iOS
Java Suite
JavaScript Suite

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

4 participants