Feature/close application rpc #5

IGapchuk · 2018-12-11T13:01:27Z

Implements smartdevicelink#1931

This PR is ready for review.

Risk

This PR makes major API changes.

Testing Plan

Unit tests.

Summary

This PR provides a new RPC called CloseApplication, which can be used by an app
to send itself into HMI_NONE.

A registered application can send this RPC to transit from any HMI level
to HMI_NONE. The application will receive an OnHMIStatus notification
which then leads to remove the lock screen from the phone app. Different
to unregister or re-register the application stays registered but will

CLA

I have signed the CLA

This PR provides a new RPC called CloseApplication, which can be used by an app to send itself into HMI_NONE. A registered application sends CloseApplicationRequest to SDL. SDL will checks if specified application is registered and it is not in HMI level NONE. After that, SDL will sends ActivateAppRequest to HMI with HMI level parameter as NONE. When SDL will received ActivateAppResponse from HMI, if result code will be as SUCCESS or WARNING, SDL will sets application(which has sent this request) HMI level NONE and sends OnHMIStatusNotification to Mobile with HMI level NONE. After that SDL will sends CloseApplicationResponse to Mobile.

AKalinich-Luxoft · 2018-12-26T10:48:19Z

...pc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/close_application_request.h

+    *
+    * @param message Incoming SmartObject message
+    */
+  CloseApplicationRequest(const app_mngr::commands::MessageSharedPtr& message,


@IGapchuk please update description as your ctor contains more parameters than described

@AKalinich-Luxoft Updated in 552836a.

AKalinich-Luxoft · 2018-12-26T10:48:58Z

...pc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/close_application_request.h

+  void on_event(const app_mngr::event_engine::Event& event) FINAL;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CloseApplicationRequest);


@IGapchuk as I remember this macro should be in the end of private section

@AKalinich-Luxoft Moved in 552836a.

AKalinich-Luxoft · 2018-12-26T10:51:03Z

...pc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/close_application_request.h

+  DISALLOW_COPY_AND_ASSIGN(CloseApplicationRequest);
+
+  /**
+    * @brief Send Basic Communication Request to HMI.


@IGapchuk sends ActivateApp request to HMI

@AKalinich-Luxoft Fixed in 552836a.

AKalinich-Luxoft · 2018-12-26T10:51:30Z

...c_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/close_application_response.h

+   *
+   * @param message - Incoming SmartObject message
+   **/
+  CloseApplicationResponse(const app_mngr::commands::MessageSharedPtr& message,


@IGapchuk update description

@AKalinich-Luxoft Updated 552836a.

AKalinich-Luxoft · 2018-12-26T10:52:26Z

...lication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/close_application_request.cc

+    return;
+  }
+
+  const bool sent_success =


@IGapchuk just sending_result

@AKalinich-Luxoft Renamed in 552836a.

AKalinich-Luxoft · 2018-12-26T10:54:19Z

...lication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/close_application_request.cc

+void CloseApplicationRequest::on_event(const event_engine::Event& event) {
+  using namespace helpers;
+  LOG4CXX_AUTO_TRACE(logger_);
+  const auto message = event.smart_object();


@IGapchuk move this line down to the place of first usage

@AKalinich-Luxoft Moved in 552836a.

AKalinich-Luxoft · 2018-12-26T10:58:57Z

...lication_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/close_application_request.cc

+      SendResponse(false, mobile_apis::Result::INVALID_DATA);
+      return;
+    }
+    application->set_hmi_level(mobile_apis::HMILevel::HMI_NONE);


@IGapchuk I think you must do this via state controller otherwise you will get an inconsistent state of application against state controller. For example check how it works for ApplicationManagerImpl::ActivateApplication. You definitely should have something similar for your case

@AKalinich-Luxoft Inside ApplicationImpl::set_hmi_level calls SetRegularState method from state controller and sets the specified hmi_state.

AKalinich-Luxoft · 2018-12-26T11:03:37Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+}
+
+TEST_F(CloseApplicationRequestTest,
+       CloseApplicationRequestSendsResponse_GENERIC_ERROR) {


@IGapchuk please use naming pattern "WhatIsTested_TestScenario_ExpectedResult" pattern for your unit tests

@AKalinich-Luxoft Renamed in 552836a.

mkorniichuk · 2019-01-02T12:35:19Z

src/components/interfaces/MOBILE_API.xml

+
+    <function name="CloseApplication" functionID="CloseApplicationID" messagetype="response" since="5.0">
+        <param name="success" type="Boolean" platform="documentation" mandatory="true">
+            <description> true if successful; false, if failed </description>


@IGapchuk without such description sense of parameter will still be clear.

mkorniichuk · 2019-01-02T12:36:58Z

src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/mobile_command_factory.cc

+      return Command::CommandSource::SOURCE_MOBILE == source
+                 ? factory.GetCreator<commands::CloseApplicationRequest>()
+                 : factory.GetCreator<commands::CloseApplicationResponse>();
+    } break;


@IGapchuk Please, check indentation and do we really need break here?

mkorniichuk · 2019-01-02T12:43:37Z

src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/mobile_command_factory.cc

@@ -396,6 +398,12 @@ CommandCreator& MobileCommandFactory::get_creator_factory(
      using app_mngr::commands::Command;
      return factory.GetCreator<commands::GenericResponse>();
    }
+    case mobile_apis::FunctionID::CloseApplicationID: {
+      using app_mngr::commands::Command;
+      return Command::CommandSource::SOURCE_MOBILE == source


@IGapchuk please, consider filtering by messageType not by commandSource

mkorniichuk · 2019-01-02T13:09:07Z

...n_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_response_test.cc

+
+TEST_F(CloseApplicationResponseTest, CloseApplicationResponse_SUCCESS) {
+  using namespace am;
+  using namespace mobile_apis;


@IGapchuk i believe, this namespace is used only for Result. So, please consider removing mobile_apis in result_code declaration or removing this using and using of mobile_apis:: for Result (I believe, the second option is preferrable).

mkorniichuk · 2019-01-02T13:11:19Z

...n_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_response_test.cc

+
+namespace {
+const uint32_t kConnectionKey = 1u;
+const uint32_t kCorrelationId = 2u;


@IGapchuk kCorrelationId is not used here.

mkorniichuk · 2019-01-02T14:00:24Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+
+  EXPECT_CALL(
+      mock_rpc_service_,
+      ManageMobileCommand(_, am::commands::Command::CommandSource::SOURCE_SDL))


@IGapchuk please, remove am:: or using namespace am

mkorniichuk · 2019-01-02T14:01:25Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+
+  EXPECT_CALL(
+      mock_rpc_service_,
+      ManageMobileCommand(_, am::commands::Command::CommandSource::SOURCE_SDL))


@IGapchuk please, remove am:: or using namespace am

mkorniichuk · 2019-01-02T14:02:09Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+
+  EXPECT_CALL(
+      mock_rpc_service_,
+      ManageMobileCommand(_, am::commands::Command::CommandSource::SOURCE_SDL))


@IGapchuk please, remove am:: or using namespace am

mkorniichuk · 2019-01-02T14:03:40Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+  MockAppPtr mock_app(CreateMockApp());
+
+  EXPECT_CALL(app_mngr_, application(kConnectionKey))
+      .WillOnce(Return(mock_app));


@IGapchuk can't you simply use mock_app_?

mkorniichuk · 2019-01-02T14:09:13Z

...on_manager/rpc_plugins/sdl_rpc_plugin/test/commands/mobile/close_application_request_test.cc

+
+  void DeffaultRequestRunCheck(const bool success_send_to_hmi) {
+    EXPECT_CALL(app_mngr_, application(kConnectionKey))
+        .WillOnce(Return(mock_app_));


@IGapchuk Please, double check this Return, since in SetUp section there is already ON_CALL which returns mock_app_ by default

IGapchuk · 2019-06-27T11:25:30Z

Not actual because of smartdevicelink#2948

IGapchuk added 2 commits December 11, 2018 15:04

Unit tests.

c579aa3

IGapchuk force-pushed the feature/close_application_rpc branch from e06b6d6 to c579aa3 Compare December 11, 2018 13:05

IGapchuk requested review from AByzhynar, AKalinich-Luxoft, ZhdanovP and mkorniichuk December 11, 2018 13:06

AKalinich-Luxoft requested changes Dec 26, 2018

View reviewed changes

Answer to PR comments.

552836a

AKalinich-Luxoft approved these changes Dec 26, 2018

View reviewed changes

mkorniichuk suggested changes Jan 2, 2019

View reviewed changes

IGapchuk closed this Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/close application rpc #5

Feature/close application rpc #5

IGapchuk commented Dec 11, 2018 •

edited

Loading

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

AKalinich-Luxoft Dec 26, 2018

IGapchuk Dec 26, 2018

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

mkorniichuk Jan 2, 2019

IGapchuk commented Jun 27, 2019

Feature/close application rpc #5

Feature/close application rpc #5

Conversation

IGapchuk commented Dec 11, 2018 • edited Loading

Risk

Testing Plan

Summary

CLA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk commented Jun 27, 2019

IGapchuk commented Dec 11, 2018 •

edited

Loading