[Withdrawn] SDL 0320 - Add screen stack management module
theresalech opened this issue · 10 comments
Hello SDL community,
The review of "SDL 0320 - Add screen stack management module" begins now and runs through September 8, 2020. The proposal is available here:
Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:
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
1. In your proposed solution, you don't mention the existing screen manager sub-managers for managing menus, templates, and perform interactions. You only mention one new manager that manages all of them. How do these relate.
2. You mention notifyScreenChanged one time but don't explain it and don't have any RPC changes for it. I believe a number of MOBILE_API and HMI_API changes are missing.
3. You provide no app library API for this screen stack manager.
4. Is the sequence for the app library, the app integration, or both? It's unclear.
5. You don't describe how the app library is supposed to know what the first layout is. You note that the app can send a SetDisplayLayout of MEDIA, but most media apps don't need to do that because the app starts on MEDIA.
6. You mention keepLevel but don't define it.
7. Sequence.5\8 changes behavior of the menu. It currently just shows the menu, but now it sends an OnButtonPress (MENU) and doesn't show the menu. This means that an app needs to modify their behavior based on the type of head unit they're connected to. This complicates app and app library code.
8. There are a lot of "hidden" changes in this proposal that are not described, such as changes to the Menu, PerformInteraction, and TextAndGraphic managers that add a lot of complexity.
9. In sequence.12 you say that the app is the one that needs to hide the PerformInteraction screen, but don't describe the mechanism. Do you mean a CancelInteraction?
10. Saying "Potential Downsides" is "None" is disingenous. This is a very complicated proposal that modifies core behavior and communication on both Core, App Libraries, and HMI. The app library screen managers will become far more complicated with many managers needing to have multiple, completely different state machines for communicating with head units before this proposal and with this proposal. The managers will also have to communicate with the "screen task manager" to handle pushing and popping stack items. In addition, this will be a big change to Core and HMI to handle apps that both do and don't subscribe to the back and menu button with very different behavior (stacking menu / stacking perform interaction / changing screens automatically vs. waiting for the app lib to do so). I believe that all of these downsides need to be listed out.
11. One possible alternative is to do this entirely on the app library side. Another possible alternative would be to only add the ButtonName for the back button. Another possible alternative is to only apply this change to templates and not to menus or perform interactions. I would like to see all of these possible alternatives listed out in detail in this proposal with pros and cons, as they have vastly different implementation costs and feature sets, but generally would solve this issue.
@joeljfischer -san,
4.
It is for app library.
5.
If the app starts on MEDIA, how the app displays the screen?
We think that it is necessary to apply.
7.
We did not know what you said.
Have the specification of the behavior of menu been changed?
What documentation should we refer?
Could you please teach us where to refer?
@joeljfischer -san,
We will talk about this matter internally, and we will add the comment if needed.
6.
We will talk about this matter internally, and we will add the comment or remove the "keeplevel" if needed.
9.
What does the meaning "mechanism"? Is it reason?
We think it notes the sequence to perform the contents described in No.12.
9. By mechanism I mean what API or code is called to do this action.
When pushing the menu item, the HMI sends an onCommand notification to the app, and then the app determines the operation. After that, you cannot return to the menu screen, but you will return to the first template screen.
My suggestion for a simple way to address this issue would be to add a new "actionContext" parameter to the AddCommand request. This context param would let the HMI know if it should return to the app template (MAIN systemContext), or keep the menu open (MENU systemContext).
Example:
<function name="AddCommand" functionID="AddCommandID" messagetype="request" since="1.0">
<description>
Adds a command to the in application menu.
Either menuParams or vrCommands must be provided.
</description>
<param name="actionContext" type="SystemContext" mandatory="false">
<description>Navigation action after command is selected. Only accepts MENU and MAIN.</description>
</param>
</function>
Note that not all of the enums in SystemContext are valid for this use case (HMI_OBSCURED, VR_SESSION, ALERT) so I would request feedback on creating a new enum specifically for this use case vs reusing the existing enum.
-
This might be covered in some of Joel's comments but I a would advise against soley relying on the app to manage the menu navigation via a MENU subscribed button. Requiring several RPC's to open and close a menu will cause performance to be worse than the way menus are navigated today.
Even if the DeleteSubMenu is notified from the SDL app library while the submenu is displayed, it is rejected.
In my opinion changing this behavior would lead to a bad user experience if a submenu was deleted while a user was viewing/interacting with the menu.
- The user pushes the submenu button. Then, the HU sends the OnCommand notification to the app.
Is this requesting a change to the current behavior of a submenu selection? Currently selecting a submenu should not trigger an OnCommand.
-
The proposal does not mention deprecating or removing the original methods for the HMI to navigate through the menu (OnSystemContext). Is SDL Core supposed to support both types of menu navigation?
-
There are no updates to the capabilities communicated to the app. How is an app to know if it is connected to a head unit that supports the new proposed menu navigation?
The Steering Committee voted to keep this proposal in review, to allow time for discussion to continue on the review issue.
@joeljfischer -san,
We think that there are two Media template.
One of them is Media with progressbar, the other is Media without progressbar.
Which Media template the SDL uses when the app starts on Media?
@JackLivio -san
We think so.
However, this behavior is the current SDL's behavior.
@Akihiro-Miyazaki, the PM is waiting for all items to be addressed by the author before responding. It's becoming difficult to follow the open items when they are addressed in various comments and out of order. Can you please address the remaining open items (1, 2, 3, 6, 8, 9, 10, 11, 12, 13, 15, 16, 17, 18) in one comment and in chronological order, so that the PM and all SDLC members reviewing can have a clearer understanding of the discussion? Thank you!
@joeljfischer -san,
I'm sorry for my misunderstanding.
I will modify to change from setPerformInteraction to setCancelInteraction in sequence.12.
This proposal has been withdrawn per the author's request.