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.

Sign up for GitHub

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

Fixed response for DeleteCommand request #579

Closed
wants to merge 1 commit into from
Closed

Fixed response for DeleteCommand request #579

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/model/sdl/Abstract/AppModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,9 +832,11 @@ SDL.ABSAppModel = Em.Object.extend(
}

if (menuID in commandsList) {
delete(commandsList[menuID]);
SDL.SDLController.onDeleteSubMenu(menuID);
SDL.OptionsView.commands.refreshItems();
if (commandsList[menuID].length == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@AKalinich-Luxoft AKalinich-Luxoft Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iCollin fix from SDL side is ready - smartdevicelink/sdl_core#3748

delete(commandsList[menuID]);
SDL.SDLController.onDeleteSubMenu(menuID);
SDL.OptionsView.commands.refreshItems()
}
return SDL.SDLModel.data.resultCode.SUCCESS;
}

Expand Down