-
Notifications
You must be signed in to change notification settings - Fork 131
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
Feature/0293 oem android specific #1604
Feature/0293 oem android specific #1604
Conversation
@santhanamk could you please start Ford review? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaBoss I have reviewed this PR, and have given some suggestions. Please take a look whenever you get the chance.
base/src/main/java/com/smartdevicelink/transport/TransportConstants.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/protocol/enums/ControlFrameTags.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlBroadcastReceiver.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlBroadcastReceiver.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlBroadcastReceiver.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlBroadcastReceiver.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlBroadcastReceiver.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaBoss I have reviewed your latest commits, and have provided some feedback. Please take a look whenever you get the chance.
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaBoss I reviewed your latest commit, and provided more feedback. Please review everything whenever you get the chance.
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
@santhanamk there were additional changes made, please, re-review |
Codecov Report
@@ Coverage Diff @@
## develop #1604 +/- ##
=============================================
- Coverage 54.46% 54.14% -0.32%
- Complexity 5407 5433 +26
=============================================
Files 555 555
Lines 24746 25074 +328
Branches 3156 3230 +74
=============================================
+ Hits 13477 13577 +100
- Misses 10095 10307 +212
- Partials 1174 1190 +16
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaBoss I have reviewed this PR, and have given a few suggestions. Please take a look whenever you get the chance.
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/VehicleTypeHolder.java
Outdated
Show resolved
Hide resolved
@santhanamk Covered as much as possible, added a bunch of tests to cover new functions. There is also a failing test after merging the latest develop branch, related to the RemoteDisplayTests. Please, re-review, let me know if there is anything I should adjust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaBoss the SdlRemoteDisplayTests
passed for me. I left a few suggestions. Please review whenever you get a chance.
Can you also please take a look at my suggestions from 4-5 days ago, and see if anything needs to be changed there?
...sdl_android/src/androidTest/java/com/smartdevicelink/test/rpc/datatypes/VehicleTypeTest.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/androidTest/java/com/smartdevicelink/test/util/SdlAppInfoTests.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/androidTest/java/com/smartdevicelink/test/util/SdlAppInfoTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YaroslavLutsenko I have some more request changes.
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
javaSE/hello_sdl_java/src/main/java/com/smartdevicelink/java/SdlService.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YaroslavLutsenko I have been testing and I have some more requested changes. Could you also add some Java Docs to the newly added methods in this PR, an example would be:
/**
* This method will retrieve the Bluetooth Mac address from the transport record.
*
* @return a string containing the Bluetooth Mac address of the connected vehicle
*/
public String getBluetoothMacAddress() {
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/lifecycle/BaseLifecycleManager.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Outdated
Show resolved
Hide resolved
@JulianKast I have applied your suggestions. Also I added some Java Docs. Could you check them, pls? (bbbd73d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YaroslavLutsenko a few minor things for clean up
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/androidTest/java/com/smartdevicelink/test/util/SdlAppInfoTests.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/androidTest/java/com/smartdevicelink/test/util/SdlAppInfoTests.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/lifecycle/BaseLifecycleManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/lifecycle/BaseLifecycleManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/lifecycle/BaseLifecycleManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/protocol/SdlProtocolBase.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
Hi @JulianKast, thank you. Done (c5c0e44) |
android/sdl_android/src/main/java/com/smartdevicelink/util/SdlAppInfo.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some code clean up items that need to be taken care of especially in regards to NPEs. A few items need to be refactored to better align with the rest of the library and items that should be generalized so we can avoid adding hyper specific solutions.
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/util/AndroidTools.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/session/BaseSdlSession.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/transport/TransportConstants.java
Outdated
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/SdlRouterService.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Show resolved
Hide resolved
android/sdl_android/src/main/java/com/smartdevicelink/transport/utl/SdlDeviceListener.java
Outdated
Show resolved
Hide resolved
Review/0293 oem android specific
Hi, @joeygrover! Thank you for your feedback, I did several corrections. Could you check this pr again, please? |
Fixes #1588
This PR is [ready] for review.
Risk
This PR makes [minor] API changes.
Testing Plan
Core Tests
VehicleType
in the SDL receiver of App and decides if it should proceed or notstartService
msg and reads theVehicleData
from the response when theMultiplexTransport
has connected and executes exclusive app filtering logicTest report
Core version / branch / commit hash / module tested against
HMI name / version / branch / commit hash / module tested against
Unit Tests
AndroidToolsTests link
VehicleType
serialized and stored correctly in the shared preferences (persistent app storage).Expected behavior - serialization/deserialization works properly, the object is saved in the persistent storage
SdlAppInfoTests link
VehicleType
configured in the XML by the developer works and compares deserialized information with the object programmatically configured.Expected behavior - objects are identical
VehicleType
passed as an argument is present in the list ofVehicleType
retrieved from the mock XML file (XML configured by developer in future).Expected behavior - passed valid object is considered as present in the predefined list, XML deserialization works fine, the final deserialized list contains passed object
VehicleTypeTests link
Expected behavior - all the data, which was in the object should serialize/deserialize correctly
Summary
Changes according to #1588
CLA