SDL restores AddCommands in the order they were created (#2515)

* SDL restores AddCommands in the order they were created

SDL generates and assigns internal_id for each AddCommand from mobile app
and restore AddCommands by this internal_id

* Fix message helper unit tests

* Fix resume controller unit tests

* Fix cmd_id in SendAddVRCommandToHMI

* fixup! SDL restores AddCommands in the order they were created

* Go to the next command if command id missing

Co-authored-by: Ira Lytvynenko (GitHub) <ILytvynenko@luxoft.com>
Co-authored-by: Yevhenii Dementieiev (GitHub) <57259850+ydementieiev@users.noreply.github.com>

9 files changed, 148 insertions, 32 deletions

diff --git a/src/components/application_manager/include/application_manager/application_data_impl.h b/src/components/application_manager/include/application_manager/application_data_impl.h
index 5c4f868fc9..5f62c8d5b5 100644
--- a/src/components/application_manager/include/application_manager/application_data_impl.h
+++ b/src/components/application_manager/include/application_manager/application_data_impl.h
@@ -156,20 +156,25 @@ class DynamicApplicationDataImpl : public virtual Application {
       const mobile_apis::MenuLayout::eType layout) const OVERRIDE;
 
   /*
-   * @brief Adds a command to the in application menu
+   * @brief Adds a command to the application menu
+   * @param[in] internal_id Internal consecutive command id
+   * @param[in] command Command to add
    */
-  void AddCommand(uint32_t cmd_id, const smart_objects::SmartObject& command);
+  void AddCommand(const uint32_t internal_id,
+                  const smart_objects::SmartObject& command);
 
   /*
    * @brief Deletes all commands from the application menu with the specified
    * command id
+   * @param[in] cmd_id Command id
    */
-  void RemoveCommand(uint32_t cmd_id);
+  void RemoveCommand(const uint32_t cmd_id);
 
   /*
    * @brief Finds command with the specified command id
+   * @param[in] cmd_id Command id
    */
-  smart_objects::SmartObject* FindCommand(uint32_t cmd_id);
+  smart_objects::SmartObject* FindCommand(const uint32_t cmd_id);
 
   /*
    * @brief Adds a menu to the application
diff --git a/src/components/application_manager/include/application_manager/commands/command_impl.h b/src/components/application_manager/include/application_manager/commands/command_impl.h
index a526fede6e..d57568dd6a 100644
--- a/src/components/application_manager/include/application_manager/commands/command_impl.h
+++ b/src/components/application_manager/include/application_manager/commands/command_impl.h
@@ -145,6 +145,16 @@ class CommandImpl : public Command {
    */
   void SetAllowedToTerminate(const bool allowed) OVERRIDE;
 
+  /**
+   * @brief Calculates command`s internal consecutive number
+   * for specified application used during resumption.
+   * This method is called when a new command is added.
+   * @param[in] app Application for wich a consecutive number is calculated
+   * @return internal consecutive number
+   */
+  static uint32_t CalcCommandInternalConsecutiveNumber(
+      application_manager::ApplicationConstSharedPtr app);
+
   // members
   static const int32_t hmi_protocol_type_;
   static const int32_t mobile_protocol_type_;
diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/add_command_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/add_command_request.cc
index a57d222765..1dbfc97458 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/add_command_request.cc
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/add_command_request.cc
@@ -160,7 +160,9 @@ void AddCommandRequest::Run() {
     return;
   }
 
-  app->AddCommand((*message_)[strings::msg_params][strings::cmd_id].asUInt(),
+  const uint32_t internal_consecutive_number = application_manager::commands::
+      CommandImpl::CalcCommandInternalConsecutiveNumber(app);
+  app->AddCommand(internal_consecutive_number,
                   (*message_)[strings::msg_params]);
 
   smart_objects::SmartObject ui_msg_params =
diff --git a/src/components/application_manager/src/application_data_impl.cc b/src/components/application_manager/src/application_data_impl.cc
index 2bef887901..9f30c00014 100644
--- a/src/components/application_manager/src/application_data_impl.cc
+++ b/src/components/application_manager/src/application_data_impl.cc
@@ -39,6 +39,25 @@
 namespace application_manager {
 CREATE_LOGGERPTR_GLOBAL(logger_, "ApplicationManager")
 
+namespace {
+struct CommandIdComparator {
+  CommandIdComparator(const std::string& key, const uint32_t id)
+      : key_(key), target_id_(id) {}
+
+  bool operator()(const CommandsMap::value_type& new_command) const {
+    smart_objects::SmartObject& command = *(new_command.second);
+    if (command.keyExists(key_)) {
+      return command[key_].asUInt() == target_id_;
+    }
+    return false;
+  }
+
+ private:
+  std::string key_;
+  uint32_t target_id_;
+};
+}  // namespace
+
 InitialApplicationDataImpl::InitialApplicationDataImpl()
     : app_types_(NULL)
     , vr_synonyms_(NULL)
@@ -724,28 +743,53 @@ void DynamicApplicationDataImpl::SetGlobalProperties(
 }
 
 void DynamicApplicationDataImpl::AddCommand(
-    uint32_t cmd_id, const smart_objects::SmartObject& command) {
+    const uint32_t internal_id, const smart_objects::SmartObject& command) {
   sync_primitives::AutoLock lock(commands_lock_ptr_);
-  CommandsMap::const_iterator it = commands_.find(cmd_id);
+
+  CommandsMap::const_iterator it = commands_.find(internal_id);
   if (commands_.end() == it) {
-    commands_[cmd_id] = new smart_objects::SmartObject(command);
+    commands_[internal_id] = new smart_objects::SmartObject(command);
+    LOG4CXX_DEBUG(logger_,
+                  "Command with internal number "
+                      << internal_id << " and id "
+                      << (*commands_[internal_id])[strings::cmd_id].asUInt()
+                      << " is added.");
   }
 }
 
-void DynamicApplicationDataImpl::RemoveCommand(uint32_t cmd_id) {
+void DynamicApplicationDataImpl::RemoveCommand(const uint32_t cmd_id) {
   sync_primitives::AutoLock lock(commands_lock_ptr_);
-  CommandsMap::iterator it = commands_.find(cmd_id);
-  if (commands_.end() != it) {
+
+  CommandIdComparator is_id_equal(strings::cmd_id, cmd_id);
+  CommandsMap::iterator it =
+      find_if(commands_.begin(), commands_.end(), is_id_equal);
+
+  if (it != commands_.end()) {
     delete it->second;
+    LOG4CXX_DEBUG(logger_,
+                  "Command with internal number " << (it->first) << " and id "
+                                                  << cmd_id << " is removed.");
     commands_.erase(it);
+
+    return;
   }
+  LOG4CXX_WARN(
+      logger_,
+      "Command with id " << cmd_id << " is not found. Removal skipped.");
 }
 
 smart_objects::SmartObject* DynamicApplicationDataImpl::FindCommand(
-    uint32_t cmd_id) {
+    const uint32_t cmd_id) {
   sync_primitives::AutoLock lock(commands_lock_ptr_);
-  CommandsMap::const_iterator it = commands_.find(cmd_id);
+
+  CommandIdComparator is_id_equal(strings::cmd_id, cmd_id);
+  CommandsMap::const_iterator it =
+      find_if(commands_.begin(), commands_.end(), is_id_equal);
+
   if (it != commands_.end()) {
+    LOG4CXX_DEBUG(logger_,
+                  "Command with internal number " << (it->first) << " and id "
+                                                  << cmd_id << " is found.");
     return it->second;
   }
 
diff --git a/src/components/application_manager/src/commands/command_impl.cc b/src/components/application_manager/src/commands/command_impl.cc
index 8875895cb7..a9fbff5ece 100644
--- a/src/components/application_manager/src/commands/command_impl.cc
+++ b/src/components/application_manager/src/commands/command_impl.cc
@@ -209,5 +209,19 @@ bool CommandImpl::ReplaceHMIWithMobileAppId(
   return true;
 }
 
+uint32_t CommandImpl::CalcCommandInternalConsecutiveNumber(
+    ApplicationConstSharedPtr app) {
+  LOG4CXX_AUTO_TRACE(logger_);
+  const DataAccessor<CommandsMap> accessor = app->commands_map();
+  const CommandsMap& commands = accessor.GetData();
+
+  uint32_t last_command_number = 0u;
+  if (!commands.empty()) {
+    last_command_number = commands.rbegin()->first;
+  }
+
+  return last_command_number + 1;
+}
+
 }  // namespace commands
 }  // namespace application_manager
diff --git a/src/components/application_manager/src/message_helper/message_helper.cc b/src/components/application_manager/src/message_helper/message_helper.cc
index fb0c9f15bf..48cf1460f9 100644
--- a/src/components/application_manager/src/message_helper/message_helper.cc
+++ b/src/components/application_manager/src/message_helper/message_helper.cc
@@ -1434,7 +1434,13 @@ smart_objects::SmartObjectList MessageHelper::CreateAddCommandRequestToHMI(
 
       smart_objects::SmartObject msg_params =
           smart_objects::SmartObject(smart_objects::SmartType_Map);
-      msg_params[strings::cmd_id] = i->first;
+
+      if ((*i->second).keyExists(strings::cmd_id)) {
+        msg_params[strings::cmd_id] = (*i->second)[strings::cmd_id].asUInt();
+      } else {
+        LOG4CXX_ERROR(logger_, "Command ID is missing.");
+        continue;
+      }
       msg_params[strings::menu_params] = (*i->second)[strings::menu_params];
       msg_params[strings::app_id] = app->app_id();
 
@@ -1449,8 +1455,9 @@ smart_objects::SmartObjectList MessageHelper::CreateAddCommandRequestToHMI(
     }
 
     // VR Interface
-    if ((*i->second).keyExists(strings::vr_commands)) {
-      SendAddVRCommandToHMI(i->first,
+    if ((*i->second).keyExists(strings::vr_commands) &&
+        (*i->second).keyExists(strings::cmd_id)) {
+      SendAddVRCommandToHMI((*i->second)[strings::cmd_id].asUInt(),
                             (*i->second)[strings::vr_commands],
                             app->app_id(),
                             app_mngr);
diff --git a/src/components/application_manager/src/resumption/resume_ctrl_impl.cc b/src/components/application_manager/src/resumption/resume_ctrl_impl.cc
index 1be336c685..d5490c8730 100644
--- a/src/components/application_manager/src/resumption/resume_ctrl_impl.cc
+++ b/src/components/application_manager/src/resumption/resume_ctrl_impl.cc
@@ -840,15 +840,19 @@ void ResumeCtrlImpl::AddCommands(ApplicationSharedPtr application,
   if (saved_app.keyExists(strings::application_commands)) {
     const smart_objects::SmartObject& app_commands =
         saved_app[strings::application_commands];
-    for (size_t i = 0; i < app_commands.length(); ++i) {
-      const smart_objects::SmartObject& command = app_commands[i];
+
+    for (size_t cmd_num = 0; cmd_num < app_commands.length(); ++cmd_num) {
+      const smart_objects::SmartObject& command = app_commands[cmd_num];
       const uint32_t cmd_id = command[strings::cmd_id].asUInt();
       const bool is_resumption = true;
-
-      application->AddCommand(cmd_id, command);
+      application->AddCommand(
+          commands::CommandImpl::CalcCommandInternalConsecutiveNumber(
+              application),
+          command);
       application->help_prompt_manager().OnVrCommandAdded(
           cmd_id, command, is_resumption);
     }
+
     ProcessHMIRequests(MessageHelper::CreateAddCommandRequestToHMI(
         application, application_manager_));
   } else {
diff --git a/src/components/application_manager/test/message_helper/message_helper_test.cc b/src/components/application_manager/test/message_helper/message_helper_test.cc
index a174d74d5d..f85c569bdb 100644
--- a/src/components/application_manager/test/message_helper/message_helper_test.cc
+++ b/src/components/application_manager/test/message_helper/message_helper_test.cc
@@ -279,14 +279,21 @@ TEST(MessageHelperTestCreate,
 
   smart_objects::SmartObject& object = *smartObjectPtr;
 
+  const uint32_t app_id = 1u;
+  const std::string cmd_icon_value = "10";
+  const uint32_t cmd_id = 5u;
+  const uint32_t internal_id = 1u;
+
   object[strings::menu_params] = 1;
   object[strings::cmd_icon] = 1;
-  object[strings::cmd_icon][strings::value] = "10";
+  object[strings::cmd_icon][strings::value] = cmd_icon_value;
+  object[strings::cmd_id] = cmd_id;
 
-  vis.insert(std::pair<uint32_t, smart_objects::SmartObject*>(5, &object));
+  vis.insert(
+      std::pair<uint32_t, smart_objects::SmartObject*>(internal_id, &object));
 
   EXPECT_CALL(*appSharedMock, commands_map()).WillOnce(Return(data_accessor));
-  EXPECT_CALL(*appSharedMock, app_id()).WillOnce(Return(1u));
+  EXPECT_CALL(*appSharedMock, app_id()).WillOnce(Return(app_id));
   application_manager_test::MockApplicationManager mock_application_manager;
   smart_objects::SmartObjectList ptr =
       MessageHelper::CreateAddCommandRequestToHMI(appSharedMock,
@@ -299,14 +306,14 @@ TEST(MessageHelperTestCreate,
   int function_id = static_cast<int>(hmi_apis::FunctionID::UI_AddCommand);
 
   EXPECT_EQ(function_id, obj[strings::params][strings::function_id].asInt());
-  EXPECT_EQ(1u, obj[strings::msg_params][strings::app_id].asUInt());
-  EXPECT_EQ(5, obj[strings::msg_params][strings::cmd_id].asInt());
+  EXPECT_EQ(app_id, obj[strings::msg_params][strings::app_id].asUInt());
+  EXPECT_EQ(cmd_id, obj[strings::msg_params][strings::cmd_id].asUInt());
   EXPECT_EQ(object[strings::menu_params],
             obj[strings::msg_params][strings::menu_params]);
   EXPECT_EQ(object[strings::cmd_icon],
             obj[strings::msg_params][strings::cmd_icon]);
   EXPECT_EQ(
-      "10",
+      cmd_icon_value,
       obj[strings::msg_params][strings::cmd_icon][strings::value].asString());
 }
 
diff --git a/src/components/application_manager/test/resumption/resume_ctrl_test.cc b/src/components/application_manager/test/resumption/resume_ctrl_test.cc
index 909ebbbb92..da3cd934e4 100644
--- a/src/components/application_manager/test/resumption/resume_ctrl_test.cc
+++ b/src/components/application_manager/test/resumption/resume_ctrl_test.cc
@@ -385,9 +385,9 @@ TEST_F(ResumeCtrlTest, StartResumption_AppWithCommands) {
   GetInfoFromApp();
   smart_objects::SmartObject test_application_commands;
   smart_objects::SmartObject test_commands;
-  const uint32_t count_of_commands = 20;
+  const uint32_t count_of_commands = 20u;
 
-  for (uint32_t i = 0; i < count_of_commands; ++i) {
+  for (uint32_t i = 0u; i < count_of_commands; ++i) {
     test_commands[application_manager::strings::cmd_id] = i;
     test_application_commands[i] = test_commands;
   }
@@ -409,10 +409,33 @@ TEST_F(ResumeCtrlTest, StartResumption_AppWithCommands) {
   ON_CALL(*mock_app_, help_prompt_manager())
       .WillByDefault(ReturnRef(*mock_help_prompt_manager_));
 
-  for (uint32_t i = 0; i < count_of_commands; ++i) {
-    EXPECT_CALL(*mock_app_, AddCommand(i, test_application_commands[i]));
-    EXPECT_CALL(*mock_help_prompt_manager_,
-                OnVrCommandAdded(i, test_application_commands[i], true));
+  std::vector<application_manager::CommandsMap> command_vec(count_of_commands);
+  for (uint32_t count = 0u; count < count_of_commands; ++count) {
+    for (uint32_t i = 0u; i < count; ++i) {
+      command_vec[count].insert(
+          std::pair<uint32_t, smart_objects::SmartObject*>(
+              i + 1, &test_application_commands[i]));
+    };
+  }
+
+  uint32_t comm_n = 0u;
+  ON_CALL(*mock_app_, commands_map())
+      .WillByDefault(testing::Invoke(
+          [&]() -> DataAccessor<application_manager::CommandsMap> {
+            DataAccessor<application_manager::CommandsMap> data_accessor(
+                command_vec[comm_n], app_set_lock_ptr_);
+            ++comm_n;
+            return data_accessor;
+          }));
+
+  for (uint32_t cmd_id = 0u, internal_id = 1u; cmd_id < count_of_commands;
+       ++cmd_id, ++internal_id) {
+    EXPECT_CALL(*mock_app_,
+                AddCommand(internal_id, test_application_commands[cmd_id]));
+
+    EXPECT_CALL(
+        *mock_help_prompt_manager_,
+        OnVrCommandAdded(cmd_id, test_application_commands[cmd_id], true));
   }
 
   smart_objects::SmartObjectList requests;