Fixed response for DeleteCommand request #579
Conversation
| delete(commandsList[menuID]); | ||
| SDL.SDLController.onDeleteSubMenu(menuID); | ||
| SDL.OptionsView.commands.refreshItems(); | ||
| if (commandsList[menuID].length == 0) { |
There was a problem hiding this comment.
Why are we checking length is zero? commandsList[menuID] needs to be deleted when we respond success to DeleteSubMenu.
Currently if I create a submenu, add a command to it, and delete the submenu I get success... but other issues are created. Now I cannot create a sub menu with the same ID as the first because "it already exists".
There was a problem hiding this comment.
@iCollin we should keep commandsList[menuID] element in the map if it still contains any child node (which can be Command or another SubMenu). This is necessary because SDL is responsible for a recursive deleting of the tree hierarchy of these nodes and in case if HMI is not able to find menuID in the map then it will respond with INVALID_ID or does not respond at all (ex see steps from the linked issue).
I mean that commandList[menuID] will be deleted by the next request from SDL, but before that element should stay in the map.
Meanwhile, could you please describe the issue with the submenu you faced with? Is it reproducible on develop?
There was a problem hiding this comment.
The issue is that we tell if a submenu exists by checking if commandList[menuID] exists, so we cannot both keep it and tell Core it is deleted. Here is where the SDL HMI sends INVALID_ID.
Interesting conundrum. The SDL HMI cannot truly delete the submenu until all its children are deleted, and Core only deletes the children once the SDL HMI has replied SUCCESS to the DeleteSubmenu request.
This is part of the pain of tracking commands in two places, I don't know if there is an easy solution for this. The best solution I can think of is to have Core trust the HMI to delete the submenu's children, and not send RPCs for the child menus and commands. Am I understanding the issue correctly? Can you think of a better solution?
There was a problem hiding this comment.
@iCollin right, it's possible to fix from SDL side and remove this logic from there (which solves that problem), but I believe it violates the requirement we have for SDL:
Preconditions:
- Application with <appID> is registered on SDL
- Submenu with <menuID> is created.
- Menu command with <cmdID> is registered.
-
Action:
- App -> SDL: DeleteSubmenu (<menuID>)
Expected:
- SDL -> HMI: UI.DeleteSubMenu (menuID, appID)
- SDL -> HMI: VR.DeleteCommand (cmdID, "Command", grammarID, appID)
- HMI removes VR-command with <cmdID> and <grammarID>
- HMI -> SDL: VR.DeleteCommand (SUCCESS)
- SDL -> HMI: UI.DeleteCommand(appID,cmdID)
- HMI removes UI-command with <cmdID> for <appID>
- HMI -> SDL: UI.DeleteCommand (SUCCESS)
Description:
In case mobile application sends DeleteSubMenu request for a subMenu with menuID, SDL must initiate on HMI an appropriate:
1) UI.DeleteSubMenu
2) VR and/or UI.DeleteCommand() in case any of this relates to the sub-menu to be removed.
Note: In case a UI-command is a part of sub-menu, VR-portion of this command must be removed too (if exists).
So most likely we should play with an HMI-side fix to solve the regression caused by that fix.
There was a problem hiding this comment.
Interesting there is no mention of the HMI -> SDL: UI DeleteSubMenu Response, maybe we can modify SDL Core to delete children of a menu before it deletes the parent?
Another solution could be the HMI tracking dead menuItems in another list, but I don't like that idea.
There was a problem hiding this comment.
@iCollin I think we can update SDL behavior as now it deletes parent node before its child nodes which is confusing HMI. In case if SDL will send requests in the correct order, this fix won't be required anymore.
I submitted an issue smartdevicelink/sdl_core#3747 to OpenSDL.
SDL fix will be prepared and linked to a new issue
There was a problem hiding this comment.
@iCollin fix from SDL side is ready - smartdevicelink/sdl_core#3748
|
Closing this PR. Fix will be provided from SDL side in scope of smartdevicelink/sdl_core#3747 |
Fixes #569
This PR is ready for review.
Testing Plan
SDL and HMI are started;mobile app is registered and activated;
SubMenu item is added
AddSubMenu("menuID":1, "menuName":"Submenu Name 1")
Nested submenu item are added (each submenu is nested in the previous submenu)
Send AddSubMenu(parentID=1, "menuID":2, "menuName":"Submenu Name 2")
Send AddSubMenu(parentID=2, "menuID":3, "menuName":"Submenu Name 3")
Send AddSubMenu(parentID=3, "menuID":4, "menuName":"Submenu Name 4")
Added AddCommand item to nested SubMenu
Send AddCommand("menuParams: { "parentID:4, "menuName":"Item to Add 1", "position":0)
Delete submenu item (item is menu-tree branch)
Send DeleteSubMenu("menuID":4)
Observe expected 'HMI→SDL: UI.DeleteCommand (resultCode = SUCCESS)'
Summary
Fixed response from HMI to SDL on DeleteCommand request when delete the top level submenu.
CLA