[0285] ShowConstantTBT Description Update #1581

kboskin · 2020-12-09T20:00:47Z

Fixes #1573

This PR is [ready] for review.

Related PR's

Core

Risk

This PR makes [minor] API changes.

Testing Plan

I have run the unit tests with this PR
I have tested this PR against Core and verified the behavior
I have tested Android

Unit Tests

Added unit tests for the new structures

Summary

Changes according to #1573

CLA

I have signed the CLA

- Implement changes according to the latest specification

kboskin · 2020-12-09T20:01:24Z

@santanamk ready for the Ford review

santhanamk

@KostyaBoss I have reviewed this PR. The code looks good. Please review whenever you get the chance, and make changes as needed.

santhanamk · 2020-12-15T22:05:49Z

...android/src/androidTest/java/com/smartdevicelink/test/rpc/requests/ShowConstantTbtTests.java

@@ -4,7 +4,7 @@
 import com.smartdevicelink.protocol.enums.FunctionID;


@KostyaBoss I would revert these changes in ShowConstantTbtTests.java. They are valid changes, but are out of scope of the PR.

santhanamk · 2020-12-15T22:06:49Z

...rc/androidTest/java/com/smartdevicelink/test/rpc/responses/ShowConstantTbtResponseTests.java

@@ -3,7 +3,7 @@
 import com.smartdevicelink.marshal.JsonRPCMarshaller;
 import com.smartdevicelink.protocol.enums.FunctionID;
 import com.smartdevicelink.proxy.RPCMessage;


@KostyaBoss I would revert these changes in ShowConstantTbtResponseTests.java. This PR only deals with the ShowConstantTbtRequest so these changes are out of scope of the PR.

santhanamk · 2020-12-15T22:08:05Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbtResponse.java

@@ -44,23 +44,31 @@
 *
 * @since SmartDeviceLink 2.0
 */
-public class ShowConstantTbtResponse extends RPCResponse {


@KostyaBoss I would revert these changes in ShowConstantTbtResponse.java. This PR only deals with the ShowConstantTbtRequest so these changes are out of scope of the PR.

santhanamk · 2020-12-15T22:10:34Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java


    public static final String KEY_TEXT1 = "navigationText1";
    public static final String KEY_TEXT2 = "navigationText2";
    public static final String KEY_ETA = "eta";
    public static final String KEY_TOTAL_DISTANCE = "totalDistance";
-    public static final String KEY_MANEUVER_DISTANCE = "distanceToManeuver";


@KostyaBoss Lines 59-60 and 206-252 are good.

I would revert the other changes, since they are out of scope of the PR.

kboskin · 2020-12-20T09:56:09Z

@santhanamk implementing experience gained while fixing the 0236 - this will cause the crash of the generic tests, which are testing the structures under the hood. Do we still need to rollback these changes? (they are according to XML provided)

santhanamk · 2020-12-28T11:00:32Z

@santhanamk implementing experience gained while fixing the 0236 - this will cause the crash of the generic tests, which are testing the structures under the hood. Do we still need to rollback these changes? (they are according to XML provided)

@KostyaBoss I would roll back the changes in:
ShowConstantTbtTests.java,
ShowConstantTbtResponseTests.java,
ShowConstantTbtResponse.java.

I believe that the changes in these three files are out of the scope of the PR.

I went through and reviewed what needs to be changed in ShowConstantTbt.java. I would not rename the file since it is out of scope of the PR.

santhanamk

@KostyaBoss I have reviewed the PR. Please take a look at the suggestions, and make the changes as needed whenever you get the chance.

santhanamk · 2020-12-28T10:54:44Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

@@ -49,34 +50,33 @@
 * @see UpdateTurnList
 * @since SmartDeviceLink 2.0
 */
-public class ShowConstantTbt extends RPCRequest {
+public class ShowConstantTBT extends RPCRequest {


@KostyaBoss I would keep the name as ShowConstantTbt on line 53. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:55:06Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     */
-    public ShowConstantTbt() {
+    public ShowConstantTBT() {


@KostyaBoss I would keep the name as ShowConstantTbt on line 70. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:55:14Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

    public static final String KEY_MANEUVER_IMAGE = "turnIcon";
    public static final String KEY_NEXT_MANEUVER_IMAGE = "nextTurnIcon";
    public static final String KEY_MANEUVER_COMPLETE = "maneuverComplete";
    public static final String KEY_SOFT_BUTTONS = "softButtons";
    public static final String KEY_TIME_TO_DESTINATION = "timeToDestination";

    /**
-     * Constructs a new ShowConstantTbt object
+     * Constructs a new ShowConstantTBT object


@KostyaBoss I would keep the name as ShowConstantTbt on line 68. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:55:25Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     *
     * @param hash The Hashtable to use
     */
-    public ShowConstantTbt(Hashtable<String, Object> hash) {
+    public ShowConstantTBT(Hashtable<String, Object> hash) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 79. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:55:33Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

        super(FunctionID.SHOW_CONSTANT_TBT.toString());
    }

    /**
-     * Constructs a new ShowConstantTbt object indicated by the Hashtable parameter
-     * <p>
+     * Constructs a new ShowConstantTBT object indicated by the Hashtable parameter


@KostyaBoss I would keep the name as ShowConstantTbt on line 75. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:56:51Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     */
-    public ShowConstantTbt setDistanceToManeuver(Double distanceToManeuver) {
-        setParameters(KEY_MANEUVER_DISTANCE, distanceToManeuver);
+    public ShowConstantTBT setDistanceToManeuver(Double distanceToManeuver) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 212. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:57:11Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     */
-    public ShowConstantTbt setDistanceToManeuverScale(Double distanceToManeuverScale) {
-        setParameters(KEY_MANEUVER_DISTANCE_SCALE, distanceToManeuverScale);
+    public ShowConstantTBT setDistanceToManeuverScale(Double distanceToManeuverScale) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 237. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:57:27Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

@@ -251,7 +259,7 @@ public Double getDistanceToManeuverScale() {
     *
     * @param maneuverComplete a Boolean value
     */
-    public ShowConstantTbt setManeuverComplete(Boolean maneuverComplete) {
+    public ShowConstantTBT setManeuverComplete(Boolean maneuverComplete) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 262. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:57:38Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

@@ -273,7 +281,7 @@ public Boolean getManeuverComplete() {
     *
     * @param softButtons a List<SoftButton> value
     */
-    public ShowConstantTbt setSoftButtons(List<SoftButton> softButtons) {
+    public ShowConstantTBT setSoftButtons(List<SoftButton> softButtons) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 284. Changing the name of the class is out of scope of the PR.

santhanamk · 2020-12-28T10:57:48Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

@@ -289,7 +297,7 @@ public ShowConstantTbt setSoftButtons(List<SoftButton> softButtons) {
        return (List<SoftButton>) getObject(SoftButton.class, KEY_SOFT_BUTTONS);
    }

-    public ShowConstantTbt setTimeToDestination(String timeToDestination) {
+    public ShowConstantTBT setTimeToDestination(String timeToDestination) {


@KostyaBoss I would keep the name as ShowConstantTbt on line 300. Changing the name of the class is out of scope of the PR.

kboskin · 2020-12-29T14:51:40Z

@santhanamk I am sorry for the big delay providing this answer but if we will rollback these changes, what we know from the other pr's - the generic internal tests, which are testing the RPC in the documentation will fail. These changes - according to XML file. Do we still need to rollback?

santhanamk · 2021-01-05T00:03:11Z

@santhanamk I am sorry for the big delay providing this answer but if we will rollback these changes, what we know from the other pr's - the generic internal tests, which are testing the RPC in the documentation will fail. These changes - according to XML file. Do we still need to rollback?

@KostyaBoss So I spoke with Michael Crimando about this. He wrote the proposal.

This is purely a description change, and nothing else. You just need to change the descriptions for those two parameters distanceToManeuver and distanceToManeuverScale in ShowConstantTbt.java. No other changes are needed.

- Changes rollback

codecov · 2021-01-06T11:50:37Z

Codecov Report

Merging #1581 (c1dd605) into develop (def46af) will increase coverage by 0.29%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #1581      +/-   ##
=============================================
+ Coverage      56.09%   56.39%   +0.29%     
- Complexity      5021     5076      +55     
=============================================
  Files            515      521       +6     
  Lines          22297    22621     +324     
  Branches        2777     2822      +45     
=============================================
+ Hits           12508    12756     +248     
- Misses          8848     8882      +34     
- Partials         941      983      +42

Impacted Files	Coverage Δ	Complexity Δ
...com/smartdevicelink/proxy/rpc/ShowConstantTbt.java	`100.00% <ø> (ø)`	`24.00 <0.00> (ø)`
...tdevicelink/managers/screen/menu/VoiceCommand.java	`59.09% <0.00%> (-40.91%)`	`8.00% <0.00%> (+1.00%)`	⬇️
...proxy/rpc/listeners/OnMultipleRequestListener.java	`35.71% <0.00%> (-14.29%)`	`1.00% <0.00%> (-1.00%)`
.../main/java/com/smartdevicelink/util/DebugTool.java	`15.05% <0.00%> (-2.16%)`	`11.00% <0.00%> (-1.00%)`
...smartdevicelink/managers/file/BaseFileManager.java	`66.84% <0.00%> (-2.13%)`	`30.00% <0.00%> (+1.00%)`	⬇️
...nk/managers/screen/SoftButtonReplaceOperation.java	`44.87% <0.00%> (-0.47%)`	`18.00% <0.00%> (-3.00%)`
...rs/screen/choiceset/PresentChoiceSetOperation.java	`41.21% <0.00%> (-0.26%)`	`17.00% <0.00%> (ø%)`
...managers/screen/TextAndGraphicUpdateOperation.java	`71.15% <0.00%> (-0.12%)`	`117.00% <0.00%> (+1.00%)`	⬇️
.../com/smartdevicelink/protocol/SdlProtocolBase.java	`15.58% <0.00%> (-0.04%)`	`22.00% <0.00%> (ø%)`
...elink/managers/lifecycle/BaseLifecycleManager.java	`14.78% <0.00%> (-0.03%)`	`11.00% <0.00%> (ø%)`
... and 12 more

kboskin · 2021-01-06T20:18:41Z

@santhanamk I've updated the pr, please, re-review

santhanamk

@KostyaBoss I reviewed the changes for the PR. I have a few more suggestions. They are pretty minor. The idea is just to change the descriptions for the parameters, and nothing else. Please take a look at the suggestions whenever you get the chance.

santhanamk · 2021-01-06T23:38:26Z

...android/src/androidTest/java/com/smartdevicelink/test/rpc/requests/ShowConstantTbtTests.java

@@ -30,7 +30,7 @@

 /**
 * This is a unit test class for the SmartDeviceLink library project class :
- * {@link com.smartdevicelink.proxy.rpc.ShowConstantTbt}
+ * {@link ShowConstantTbt}


@KostyaBoss On line 33 I would remove it and change it back to {@link com.smartdevicelink.proxy.rpc.ShowConstantTbt}

The idea is that nothing should be changed in this class.

santhanamk · 2021-01-06T23:39:38Z

...android/src/androidTest/java/com/smartdevicelink/test/rpc/requests/ShowConstantTbtTests.java

@@ -71,8 +71,8 @@ protected JSONObject getExpectedParameters(int sdlVersion) {
            result.put(ShowConstantTbt.KEY_SOFT_BUTTONS, TestValues.JSON_SOFTBUTTONS);
            result.put(ShowConstantTbt.KEY_ETA, TestValues.GENERAL_STRING);
            result.put(ShowConstantTbt.KEY_MANEUVER_COMPLETE, true);
-            result.put(ShowConstantTbt.KEY_MANEUVER_DISTANCE, TestValues.GENERAL_DOUBLE);
-            result.put(ShowConstantTbt.KEY_MANEUVER_DISTANCE_SCALE, TestValues.GENERAL_DOUBLE);
+            result.put(ShowConstantTbt.KEY_DISTANCE_TO_MANEUVER, TestValues.GENERAL_DOUBLE);


@KostyaBoss On line 74 I would rename KEY_DISTANCE_TO_MANEUVER back to its old variable name KEY_MANEUVER_DISTANCE. You can apply this change throughout the class.

santhanamk · 2021-01-06T23:40:04Z

...android/src/androidTest/java/com/smartdevicelink/test/rpc/requests/ShowConstantTbtTests.java

-            result.put(ShowConstantTbt.KEY_MANEUVER_DISTANCE, TestValues.GENERAL_DOUBLE);
-            result.put(ShowConstantTbt.KEY_MANEUVER_DISTANCE_SCALE, TestValues.GENERAL_DOUBLE);
+            result.put(ShowConstantTbt.KEY_DISTANCE_TO_MANEUVER, TestValues.GENERAL_DOUBLE);
+            result.put(ShowConstantTbt.KEY_DISTANCE_TO_MANEUVER_SCALE, TestValues.GENERAL_DOUBLE);


@KostyaBoss On line 75 I would rename KEY_DISTANCE_TO_MANEUVER_SCALE back to its old variable name KEY_MANEUVER_DISTANCE_SCALE. You can apply this change throughout the class.

santhanamk · 2021-01-06T23:41:09Z

...rc/androidTest/java/com/smartdevicelink/test/rpc/responses/ShowConstantTbtResponseTests.java

@@ -21,7 +21,7 @@

 /**
 * This is a unit test class for the SmartDeviceLink library project class :
- * {@link com.smartdevicelink.proxy.rpc.ShowConstantTbtResponse}
+ * {@link ShowConstantTbtResponse}


@KostyaBoss I would delete line 24 and replace it with its old line {@link com.smartdevicelink.proxy.rpc.ShowConstantTbtResponse}

santhanamk · 2021-01-06T23:42:46Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

@@ -33,6 +33,7 @@

 import com.smartdevicelink.protocol.enums.FunctionID;
 import com.smartdevicelink.proxy.RPCRequest;
+import com.smartdevicelink.util.SdlDataTypeConverter;


@KostyaBoss I would remove this import on line 36 import com.smartdevicelink.util.SdlDataTypeConverter;.

(The idea is to just make the description changes and nothing else)

santhanamk · 2021-01-06T23:50:23Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     */
    public Double getDistanceToManeuverScale() {
-        return getDouble(KEY_MANEUVER_DISTANCE_SCALE);
+        Object object = getParameters(KEY_DISTANCE_TO_MANEUVER_SCALE);


@KostyaBoss I would delete lines 252-253 and replace it with the old code return getDouble(KEY_MANEUVER_DISTANCE_SCALE);.

santhanamk · 2021-01-06T23:51:10Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbtResponse.java

@@ -46,19 +46,27 @@
 */
 public class ShowConstantTbtResponse extends RPCResponse {

+    /**


@KostyaBoss I would delete the comment block on lines 49-51. The idea is that nothing should be changed in this file.

santhanamk · 2021-01-06T23:51:25Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbtResponse.java

    public ShowConstantTbtResponse() {
        super(FunctionID.SHOW_CONSTANT_TBT.toString());
    }

+    /**


@KostyaBoss I would delete the comment block on lines 56-60. The idea is that nothing should be changed in this file.

santhanamk · 2021-01-06T23:52:02Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbtResponse.java

    public ShowConstantTbtResponse(Hashtable<String, Object> hash) {
        super(hash);
    }

    /**
     * Constructs a new ShowConstantTbtResponse object
     *
-     * @param success    whether the request is successfully processed
-     * @param resultCode whether the request is successfully processed
+     * @param success whether the request is successfully processed


@KostyaBoss I would delete lines 68-69 and bring back the old formatted code.

santhanamk · 2021-01-06T23:53:16Z

base/src/main/java/com/smartdevicelink/proxy/rpc/ShowConstantTbt.java

     */
    public Double getDistanceToManeuver() {
-        return getDouble(KEY_MANEUVER_DISTANCE);
+        Object object = getParameters(KEY_DISTANCE_TO_MANEUVER);


@KostyaBoss I would delete lines 226 and 227 and replace it with the old code return getDouble(KEY_MANEUVER_DISTANCE);.

- Fix showConstantTbt

- Changes rollback

kboskin · 2021-01-11T10:47:45Z

@santhanamk rollbacked the required changes according to your request and double-checked everything, all the comments seem to be processed. Please, let me know if some additional changes are required there

santhanamk

@KostyaBoss Code looks good. I have approved the PR.

Once you have tested against Core and HMI (if applicable), please put the links for those PRs. Please mark the PR as ready for review, and you can tag Livio once the testing is done on your end.

kboskin · 2021-01-11T20:24:05Z

@bilal-alsharifi , @jordynmackool This pr is ready for the Livio review

[0285]

ab048ce

- Implement changes according to the latest specification

kboskin changed the base branch from master to develop December 9, 2020 20:01

santhanamk reviewed Dec 15, 2020

View reviewed changes

santhanamk reviewed Dec 28, 2020

View reviewed changes

[0285]

a0b0380

- Changes rollback

santhanamk reviewed Jan 6, 2021

View reviewed changes

kboskin added 2 commits January 11, 2021 12:26

[0285]

cd2c28c

- Fix showConstantTbt

[0285]

c1dd605

- Changes rollback

santhanamk approved these changes Jan 11, 2021

View reviewed changes

kboskin marked this pull request as ready for review January 11, 2021 20:24

bilal-alsharifi approved these changes Jan 12, 2021

View reviewed changes

bilal-alsharifi merged commit 69cc4b8 into smartdevicelink:develop Jan 12, 2021

		@@ -4,7 +4,7 @@
		import com.smartdevicelink.protocol.enums.FunctionID;

[0285] ShowConstantTBT Description Update #1581

[0285] ShowConstantTBT Description Update #1581

Conversation

kboskin commented Dec 9, 2020 • edited Loading

Related PR's

Risk

Testing Plan

Testing Plan

Unit Tests

Summary

CLA

kboskin commented Dec 9, 2020

santhanamk left a 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

kboskin commented Dec 20, 2020

santhanamk commented Dec 28, 2020

santhanamk left a 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

kboskin commented Dec 29, 2020

santhanamk commented Jan 5, 2021

codecov bot commented Jan 6, 2021 • edited Loading

Codecov Report

kboskin commented Jan 6, 2021

santhanamk left a 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

kboskin commented Jan 11, 2021

santhanamk left a comment

Choose a reason for hiding this comment

kboskin commented Jan 11, 2021

kboskin commented Dec 9, 2020 •

edited

Loading

codecov bot commented Jan 6, 2021 •

edited

Loading