Skip to content

Feature AppLaunch #737

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.

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

Merged
merged 10 commits into from
Aug 5, 2016
Merged

Feature AppLaunch #737

merged 10 commits into from
Aug 5, 2016

Conversation

seruhii
Copy link
Contributor

@seruhii seruhii commented Aug 3, 2016

Feature allows SDL to launch apps on connected devices.

Related to: APPLINK-24892

@okozlovlux, @Kozoriz, @LuxoftAKutsan, @vlantonov, @wolfylambova22, @VVeremjova, please review.

@vlantonov
Copy link
Contributor

@LevchenkoS Is this pull request rebased to the latest commit in develop branch?

@seruhii
Copy link
Contributor Author

seruhii commented Aug 3, 2016

@vlantonov, no, it's for release/4.2.0, based on release/4.1.0

@vlantonov
Copy link
Contributor

@LevchenkoS Please try to rebase to the latest commit of release/4.2.0 This pull request contains commits from other people while it is supposed to contain only your commits, related to APPLINK-24892

@seruhii
Copy link
Contributor Author

seruhii commented Aug 3, 2016

@vlantonov, sorry, but it should contains them. Please, look at email that I was sent to you and other reviewers.

@vlantonov
Copy link
Contributor

@LevchenkoS No, it shouldn't. These commits are from other pull request and are already reviewed and merged. However they appear as changes you have made and thus will be reviewed again. I repeat - please rebase your pull request to the latest commit in release/4.2.0

@vlantonov
Copy link
Contributor

@LevchenkoS As far as I understand from your explaination those commits are cherry-picked and that is why they should be here. So this is OK.

@@ -696,7 +701,11 @@ bool ResumeCtrl::CheckDelayAfterIgnOn() {
return seconds_from_sdl_start <= wait_time;
}

<<<<<<< HEAD:src/components/application_manager/src/resumption/resume_ctrl.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

@LevchenkoS remove merge issues

Copy link
Contributor Author

@seruhii seruhii Aug 3, 2016

Choose a reason for hiding this comment

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

@LuxoftAKutsan, fixed in f6ebfe5.

@wolfylambova22
Copy link

Reviewed.

* @param application is application witch HMI Level is need to restore
* @return true if success, otherwise return false
*/
bool RestoreAppHMIState(app_mngr::ApplicationSharedPtr application);
Copy link
Contributor

Choose a reason for hiding this comment

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

@LevchenkoS Please add OVERRIDE here and in the other methods inherited from ResumeCtrl

Copy link
Contributor Author

@seruhii seruhii Aug 3, 2016

Choose a reason for hiding this comment

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

@vlantonov, fixed in f6ebfe5.

Vladislav Antonov and others added 2 commits August 3, 2016 17:54
Related issue : APPLINK-24892
Applaunch functionality in TransportMaanger and ConnectionHAndler implementation

Related issue : APPLINK-24892
@codecov-io
Copy link

codecov-io commented Aug 4, 2016

Current coverage is 68.86% (diff: 75.39%)

Merging #737 into release/4.2.0 will increase coverage by 0.31%

@@           release/4.2.0       #737   diff @@
===============================================
  Files                313        331    +18   
  Lines              22583      23366   +783   
  Methods                0          0          
  Messages               0          0          
  Branches               0          0          
===============================================
+ Hits               15479      16090   +611   
- Misses              7104       7276   +172   
  Partials               0          0          

Powered by Codecov. Last update edbd60e...8b74854

hmi_language_handler_.Init(last_state);
if (false == load_capabilities_from_file()) {
LOG4CXX_ERROR(logger_, "file hmi_capabilities.json was not loaded");
} else {
LOG4CXX_INFO(logger_, "file hmi_capabilities.json was loaded");
LOG4CXX_ERROR(logger_, "file hmi_capabilities.json was loaded");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is just info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kozoriz, fixed in 1c2cf9c.

#include "application_manager/application.h"
#include "resumption/last_state.h"

namespace resumption {
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect namespaces
test -> components -> resumprion_test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kozoriz, fixed in f6ebfe5.

LuxoftAKutsan and others added 6 commits August 4, 2016 17:30
Related issue : APPLINK-24892
Related issue : APPLINK-24892
`ResumeCtrl` was divided onto
interface and implementation.

Related to: APPLINK-24892
`HMICapabilities` was divided onto
interface and implementation.

Related to: APPLINK-24892
Reduced dependence of `ApplicationManager`
from `ResumeCtrl` and `HMICapabilities`.

Related to: APPLINK-24892
@Kozoriz
Copy link
Contributor

Kozoriz commented Aug 5, 2016

Reviewed

namespace application_manager {
class Application;
typedef utils::SharedPtr<const Application> ApplicationConstSharedPtr;
} // namesapce application_manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@seruhii seruhii Aug 5, 2016

Choose a reason for hiding this comment

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

@VVeremjova, fixed in 9926c88.

@vlantonov
Copy link
Contributor

@LevchenkoS Reviewed.

@@ -94,7 +98,7 @@ class HMICapabilitiesTest : public ::testing::Test {
resumption::LastState last_state_;
MockApplicationManagerSettings mock_application_manager_settings_;
utils::SharedPtr<HMICapabilitiesForTesting> hmi_capabilities_test;
const std::string kFileName = "hmi_capabilities.json";
std::string file_name_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@LevchenkoS You could leave const here

Copy link
Contributor Author

@seruhii seruhii Aug 5, 2016

Choose a reason for hiding this comment

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

@VVeremjova, fixed in 8e37c0c.

@kozlov-oleksandr
Copy link

@LevchenkoS , reviewed

@VVeremjova
Copy link
Contributor

@LevchenkoS Reviewed

virtual const uint16_t max_number_of_ios_device() const = 0;
virtual const uint16_t wait_time_between_apps() const = 0;
virtual const bool enable_app_launch_ios() const = 0;
virtual uint32_t resumption_delay_after_ign() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@LevchenkoS
either add const to return value or remove const in others

Copy link
Contributor Author

@seruhii seruhii Aug 5, 2016

Choose a reason for hiding this comment

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

@LuxoftAKutsan, fixed in c9dfbce.

@LuxoftAKutsan
Copy link
Contributor

Reviewed

LuxoftAKutsan and others added 2 commits August 5, 2016 15:40
Related issue : APPLINK-24892
Has been done:
- Fixed coding style;
- Fixed header guard naming;
- Fixed copyright comments.

Related to: APPLINK-24892
@anosach-luxoft anosach-luxoft merged commit 5b98543 into smartdevicelink:release/4.2.0 Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants