Feature/CloseApplication RPC #2730
Conversation
IGapchuk
requested review from
LuxoftAKutsan,
AKalinich-Luxoft,
BSolonenko,
Itileda,
mked-luxoft,
a user,
Ypostolov and
ZhdanovP
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
from
e33eb61 to
ea79d01
Compare
| * @brief Return HMI application id. | ||
| * @return hmi_app_id as uint32_t. | ||
| */ | ||
| uint32_t HMIAppId(); |
There was a problem hiding this comment.
@IGapchuk if it is getter, use appropriate coding style. Also make it const
There was a problem hiding this comment.
@IGapchuk you added this getter in base class. Looks like it widely used functionality. Please replace all places where this getter could be used.
There was a problem hiding this comment.
@LuxoftAKutsan All implamentation has changed in da8a1b0.
| app_mngr::ApplicationManager& application_manager, | ||
| app_mngr::rpc_service::RPCService& rpc_service, | ||
| app_mngr::HMICapabilities& hmi_capabilities, | ||
| policy::PolicyHandlerInterface& policy_handle); |
There was a problem hiding this comment.
@IGapchuk policy_handler
|
|
||
| void CloseApplicationRequest::Run() { | ||
| LOG4CXX_AUTO_TRACE(logger_); | ||
| using namespace application_manager; |
There was a problem hiding this comment.
@IGapchuk move using before trace
| if (!app) { | ||
| LOG4CXX_WARN(logger_, | ||
| "Application with hmi_app_id " << hmi_app_id | ||
| << " isn't found."); |
There was a problem hiding this comment.
@IGapchuk 'not found'
| return; | ||
| } | ||
|
|
||
| hmi_apis::Common_HMILevel::eType app_hmi_level = |
| const smart_objects::SmartObject& message = event.smart_object(); | ||
|
|
||
| switch (event.id()) { | ||
| case hmi_apis::FunctionID::BasicCommunication_CloseApplication: { |
There was a problem hiding this comment.
@IGapchuk you have only one case here. Please use If
| } | ||
| } break; | ||
| default: { | ||
| LOG4CXX_ERROR(logger_, "Unknown event "); |
There was a problem hiding this comment.
@IGapchuk I would DCHECK this condition. You should never get wrong events
| if (hmi_apis::Common_Result::SUCCESS == result_code) { | ||
| if (message.keyExists(strings::app_id)) { | ||
| const uint32_t app_id = | ||
| static_cast<uint32_t>(message[strings::app_id].asInt()); |
There was a problem hiding this comment.
@IGapchuk can you use asUInt?
| return 0; | ||
| } | ||
| return static_cast<uint32_t>( | ||
| so[strings::msg_params][strings::application][strings::app_id].asUInt()); |
There was a problem hiding this comment.
@IGapchuk please make this function more effective. You getting so[strings::msg_params] 4 times here!
You can use temporary variables (const ref) to access already checked internals of so
| @@ -3603,6 +3603,23 @@ | |||
| </function> | |||
| <function name="DialNumber" messagetype="response"> | |||
| </function> | |||
| <function name="CloseApplication" messagetype="request"> | |||
There was a problem hiding this comment.
@IGapchuk shouldn't request contain app_id?
Please add @KhrystynaDubovyk to review
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
from
ea79d01 to
da8a1b0
Compare
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
2 times, most recently
from
1a745b2 to
f0c68ec
Compare
| /** | ||
| * @brief Execute command | ||
| */ | ||
| virtual void Run() FINAL; |
There was a problem hiding this comment.
@IGapchuk virtual keyword is redundant
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| void CloseApplicationRequest::Run() { | ||
| LOG4CXX_AUTO_TRACE(logger_); | ||
|
|
||
| ApplicationSharedPtr application = |
There was a problem hiding this comment.
@IGapchuk good place for auto
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| return; | ||
| } | ||
|
|
||
| const mobile_apis::HMILevel::eType app_hmi_level = application->hmi_level(); |
There was a problem hiding this comment.
@IGapchuk good place for auto
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
|
|
||
| uint32_t activate_app_corr_id = 0; | ||
| const bool sent_success = SendBCActivateApp( | ||
| application, hmi_apis::Common_HMILevel::NONE, true, activate_app_corr_id); |
There was a problem hiding this comment.
@IGapchuk make activate_app_corr_id be a return code of SendBCActivateApp function. It may be 0, if it is not successful, or it may return pair.
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| application, hmi_apis::Common_HMILevel::NONE, true, activate_app_corr_id); | ||
|
|
||
| if (sent_success) { | ||
| subscribe_on_event(hmi_apis::FunctionID::BasicCommunication_ActivateApp, |
There was a problem hiding this comment.
@IGapchuk can we subscribe on this event inside SendBCActivateApp ?
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| activate_app_corr_id); | ||
| } else { | ||
| LOG4CXX_ERROR(logger_, "Unable to send BC.ActivateApp"); | ||
| SendResponse(false, mobile_apis::Result::IGNORED); |
There was a problem hiding this comment.
@IGapchuk is it ignored? I would say that it is some error.
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| void CloseApplicationRequest::on_event(const event_engine::Event& event) { | ||
| using namespace helpers; | ||
| LOG4CXX_AUTO_TRACE(logger_); | ||
| const smart_objects::SmartObject& message = event.smart_object(); |
There was a problem hiding this comment.
@IGapchuk good place for auto
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| hmi_apis::Common_Result::WARNINGS); | ||
|
|
||
| auto application = application_manager_.application(connection_key()); | ||
| if (success) { |
There was a problem hiding this comment.
@IGapchuk I would also check application pointer.
And if success != true, you do not need to look for application
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| bool CloseApplicationRequest::SendBCActivateApp( | ||
| ApplicationConstSharedPtr app, | ||
| hmi_apis::Common_HMILevel::eType hmi_level, | ||
| bool send_policy_priority, |
There was a problem hiding this comment.
@IGapchuk make variables that may be const - const
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
| bool send_policy_priority, | ||
| uint32_t& activate_app_corr_id) { | ||
| LOG4CXX_AUTO_TRACE(logger_); | ||
| smart_objects::SmartObjectSPtr bc_activate_app_request = |
There was a problem hiding this comment.
@IGapchuk good place for auto
There was a problem hiding this comment.
@LuxoftAKutsan Fixed in 4257eff.
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
from
f0c68ec to
b232e7d
Compare
|
Rebuild required |
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
from
0e8aa4a to
8c09bf6
Compare
| TEST_F(CloseApplicationRequestTest, | ||
| CloseApplicationRequestSendsResponse_IGNORED) { | ||
| using namespace am; | ||
| auto message = CreateCloseAppMessage(); |
There was a problem hiding this comment.
@IGapchuk We can move creation message and response_message to SetUp method I think.
| .WillOnce(Return(mock_app_)); | ||
| EXPECT_CALL(*mock_app_, hmi_level()) | ||
| .WillOnce(Return(mobile_apis::HMILevel::eType::HMI_FULL)); | ||
|
|
There was a problem hiding this comment.
@IGapchuk Seems that we can add additional method in class and move this code
EXPECT_CALL(app_mngr_, application(kConnectionKey)) .WillOnce(Return(mock_app_)); EXPECT_CALL(*mock_app_, hmi_level()) .WillOnce(Return(mobile_apis::HMILevel::eType::HMI_FULL)); ON_CALL(*am::MockMessageHelper::message_helper_mock(), GetBCActivateAppRequestToHMI(_, _, _, _, _, _)) .WillByDefault( Return(std::make_shared<SmartObject>(smart_objects::SmartType_Null))); EXPECT_CALL(mock_rpc_service_, ManageHMICommand(_)).WillOnce(Return(true));
there
Is it possible?
| CloseApplicationCommandPtr command_; | ||
| }; | ||
|
|
||
| TEST_F(CloseApplicationResponseTest, CloseApplicationResponse_SUCCESS) { |
|
@IGapchuk typo in commit message |
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.
IGapchuk
force-pushed
the
feature/close_application_rpc
branch
from
e06b6d6 to
c579aa3
Compare
Implements #1931
This PR is ready for review.
Risk
This PR makes major API changes.
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
not be used as the preferred mobile-nav application anymore.
Tasks Remaining:
CLA