Skip to content

Fix soft button object invalid states #1776

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.

Sign up for GitHub

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 20 commits into from
Feb 3, 2022
Merged

Fix soft button object invalid states #1776

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2445b37
Resolve #1774
noah-livio Dec 7, 2021
72e24b5
Add tests
noah-livio Dec 7, 2021
c9da956
Update Javadoc with throws annotations
noah-livio Dec 7, 2021
0aebb4e
Removing throwing of IllegalStateException
noah-livio Dec 14, 2021
b00cea5
Remove unused import
noah-livio Dec 14, 2021
8413d94
Make changes discussed in #1776 review
noah-livio Dec 15, 2021
f95aec3
Fix a small typo in the unit test javadoc
noah-livio Dec 15, 2021
648bdc3
Cleanup code and modify tests
noah-livio Dec 17, 2021
9a82059
Move states assignment back to original line
noah-livio Dec 17, 2021
053ac1b
Add duplicate name check to setStates()
noah-livio Dec 17, 2021
9d47512
Add AssertionErrors as per iOS alignment
noah-livio Jan 7, 2022
386027f
Remove redundant conditions and commented code
noah-livio Jan 7, 2022
75d1e1e
Add AssertionError to SoftButtonState
noah-livio Jan 7, 2022
d15498f
Add test for initialStateName check
noah-livio Jan 10, 2022
34c0622
Revert addtion of AssertionErrors
noah-livio Jan 11, 2022
528769f
Update SoftButtonManagerTests test
noah-livio Jan 20, 2022
e183f35
Update ScreenManagerTests test
noah-livio Jan 20, 2022
666f99a
Fixes from PR feedback
noah-livio Jan 26, 2022
14b7569
Review and standup feedback fixes
noah-livio Jan 28, 2022
39efd8e
PR feedback change
noah-livio Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions ..._android/src/androidTest/java/com/smartdevicelink/managers/screen/ScreenManagerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,15 @@ public void testSettingSoftButtonId() {
}
@Test
public void testAssigningIdsToSoftButtonObjects() {
SoftButtonState defaultState = new SoftButtonState("default", "hi", null);
SoftButtonObject sbo1, sbo2, sbo3, sbo4, sbo5;

// Case 1 - don't set id for any button (Manager should set ids automatically starting from 1 and up)
sbo1 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo2 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo3 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo4 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo5 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo1 = new SoftButtonObject(null, defaultState, null);
sbo2 = new SoftButtonObject(null, defaultState, null);
sbo3 = new SoftButtonObject(null, defaultState, null);
sbo4 = new SoftButtonObject(null, defaultState, null);
sbo5 = new SoftButtonObject(null, defaultState, null);
screenManager.checkAndAssignButtonIds(Arrays.asList(sbo1, sbo2, sbo3, sbo4, sbo5), BaseScreenManager.ManagerLocation.SOFTBUTTON_MANAGER);
assertEquals("SoftButtonObject id doesn't match the expected value", 1, sbo1.getButtonId());
assertEquals("SoftButtonObject id doesn't match the expected value", 2, sbo2.getButtonId());
Expand All @@ -234,15 +235,15 @@ public void testAssigningIdsToSoftButtonObjects() {


// Case 2 - Set ids for all buttons (Manager shouldn't alter the ids set by developer)
sbo1 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo1 = new SoftButtonObject(null, defaultState, null);
sbo1.setButtonId(100);
sbo2 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo2 = new SoftButtonObject(null, defaultState, null);
sbo2.setButtonId(200);
sbo3 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo3 = new SoftButtonObject(null, defaultState, null);
sbo3.setButtonId(300);
sbo4 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo4 = new SoftButtonObject(null, defaultState, null);
sbo4.setButtonId(400);
sbo5 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo5 = new SoftButtonObject(null, defaultState, null);
sbo5.setButtonId(500);
screenManager.checkAndAssignButtonIds(Arrays.asList(sbo1, sbo2, sbo3, sbo4, sbo5), BaseScreenManager.ManagerLocation.SOFTBUTTON_MANAGER);
assertEquals("SoftButtonObject id doesn't match the expected value", 100, sbo1.getButtonId());
Expand All @@ -253,13 +254,13 @@ public void testAssigningIdsToSoftButtonObjects() {


// Case 3 - Set ids for some buttons (Manager shouldn't alter the ids set by developer. And it should assign ids for the ones that don't have id)
sbo1 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo1 = new SoftButtonObject(null, defaultState, null);
sbo1.setButtonId(50);
sbo2 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo3 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo4 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo2 = new SoftButtonObject(null, defaultState, null);
sbo3 = new SoftButtonObject(null, defaultState, null);
sbo4 = new SoftButtonObject(null, defaultState, null);
sbo4.setButtonId(100);
sbo5 = new SoftButtonObject(null, Collections.EMPTY_LIST, null, null);
sbo5 = new SoftButtonObject(null, defaultState, null);
screenManager.checkAndAssignButtonIds(Arrays.asList(sbo1, sbo2, sbo3, sbo4, sbo5), BaseScreenManager.ManagerLocation.SOFTBUTTON_MANAGER);
assertEquals("SoftButtonObject id doesn't match the expected value", 50, sbo1.getButtonId());
assertEquals("SoftButtonObject id doesn't match the expected value", 101, sbo2.getButtonId());
Expand Down
101 changes: 97 additions & 4 deletions ...roid/src/androidTest/java/com/smartdevicelink/managers/screen/SoftButtonManagerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.Collections;
import java.util.List;


Copy link
Contributor

@RHenigan RHenigan Dec 16, 2021

Choose a reason for hiding this comment

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

just cleaning up white space.

import static junit.framework.TestCase.assertEquals;
import static junit.framework.TestCase.assertFalse;
import static junit.framework.TestCase.assertNull;
Expand Down Expand Up @@ -350,8 +351,8 @@ public void onEvent(SoftButtonObject softButtonObject, OnButtonEvent onButtonEve
softButtonStateList.add(softButtonState1);
softButtonStateList2.add(softButtonState1);
softButtonStateList2.add(softButtonState2);
softButtonObject1 = new SoftButtonObject("hi", softButtonStateList, "Hi", null);
softButtonObject2 = new SoftButtonObject("hi", softButtonStateList2, "Hi", null);
softButtonObject1 = new SoftButtonObject("hi", softButtonStateList, softButtonStateList.get(0).getName(), null);
softButtonObject2 = new SoftButtonObject("hi", softButtonStateList2, softButtonStateList2.get(0).getName(), null);
assertNotEquals(softButtonObject1, softButtonObject2);

// Case 5: SoftButtonStates are not the same, assertFalse
Expand All @@ -365,8 +366,8 @@ public void onEvent(SoftButtonObject softButtonObject, OnButtonEvent onButtonEve
assertNotEquals(softButtonObject1, softButtonObject2);

// Case 7: SoftButtonObject currentStateName not same, assertFalse
softButtonObject1 = new SoftButtonObject("hi", softButtonStateList, "Hi", null);
softButtonObject2 = new SoftButtonObject("hi", softButtonStateList, "Hi2", null);
softButtonObject1 = new SoftButtonObject("hi", softButtonStateList2, softButtonStateList2.get(0).getName(), null);
softButtonObject2 = new SoftButtonObject("hi", softButtonStateList2, softButtonStateList2.get(1).getName(), null);
assertNotEquals(softButtonObject1, softButtonObject2);
}

Expand Down Expand Up @@ -402,4 +403,96 @@ public void testSoftButtonStateEquals() {
softButtonState2 = new SoftButtonState("object1-state1", "o1s1", artwork1);
assertEquals(softButtonState1, softButtonState2);
}

/**
* Test constructing SoftButtonObject with an empty state list
*/
@Test
public void testConstructSoftButtonObjectWithEmptyStateList() {
List<SoftButtonState> stateList = new ArrayList<>();
SoftButtonObject softButtonObject = new SoftButtonObject("hello_there", stateList, "general_kenobi", null);
assertNull(softButtonObject.getStates());
}

/**
* Test constructing SoftButtonObject with an nonempty state list
*/
@Test
public void testConstructSoftButtonObjectWithNonEmptyStateList() {
List<SoftButtonState> stateList = new ArrayList<>();
SoftButtonState softButtonState = new SoftButtonState("general_kenobi", "General Kenobi", null);
stateList.add(softButtonState);
SoftButtonObject softButtonObject = new SoftButtonObject("hello_there", stateList, "general_kenobi", null);
assertEquals(stateList, softButtonObject.getStates());
}

/**
* Test constructing SoftButtonObject with an invalid initialStateName
*/
@Test
public void testConstructSoftButtonObjectWithInvalidInitialStateName() {
List<SoftButtonState> stateList = new ArrayList<>();
SoftButtonState softButtonState = new SoftButtonState("general_kenobi", "General Kenobi", null);
stateList.add(softButtonState);
SoftButtonObject softButtonObject = new SoftButtonObject("hello_there", stateList, "hello_there", null);
assertNull(softButtonObject.getStates());
}

/**
* Test assigning an empty state list to existing SoftButtonObject
*/
@Test
public void testAssignEmptyStateListToSoftButtonObject() {
List<SoftButtonState> nonEmptyStateList = new ArrayList<>();
List<SoftButtonState> emptyStateList = new ArrayList<>();
SoftButtonState softButtonState = new SoftButtonState("general_kenobi", "General Kenobi", null);
nonEmptyStateList.add(softButtonState);

SoftButtonObject softButtonObject = new SoftButtonObject("hello_there", nonEmptyStateList, "general_kenobi", null);

softButtonObject.setStates(emptyStateList);
assertEquals(nonEmptyStateList, softButtonObject.getStates());
}

/**
* Test assigning a nonempty state list to existing SoftButtonObject
*/
@Test
public void testAssignNonEmptyStateListToSoftButtonObject() {
List<SoftButtonState> stateList1 = new ArrayList<>();
SoftButtonState softButtonState1 = new SoftButtonState("hello_there", "Hello there", null);
stateList1.add(softButtonState1);

List<SoftButtonState> stateList2 = new ArrayList<>();
SoftButtonState softButtonState2 = new SoftButtonState("general_kenobi", "General Kenobi", null);
stateList2.add(softButtonState2);

SoftButtonObject softButtonObject = new SoftButtonObject("general_kenobi", stateList1, "hello_there", null);

softButtonObject.setStates(stateList2);

assertEquals(stateList2, softButtonObject.getStates());
}

/**
* Test assigning a state list with states that have the same name to existing SoftButtonObject
*/
@Test
public void testAssignSameNameStateListToSoftButtonObject() {
List<SoftButtonState> stateListUnique = new ArrayList<>();
SoftButtonState softButtonState1 = new SoftButtonState("hello_there", "Hello there", null);
stateListUnique.add(softButtonState1);

List<SoftButtonState> stateListDuplicateNames = new ArrayList<>();
SoftButtonState softButtonState2 = new SoftButtonState("general_kenobi", "General Kenobi", null);
stateListDuplicateNames.add(softButtonState2);
SoftButtonState softButtonState3 = new SoftButtonState("general_kenobi", "General Kenobi Again", null);
stateListDuplicateNames.add(softButtonState3);

SoftButtonObject softButtonObject = new SoftButtonObject("general_kenobi", stateListUnique, "hello_there", null);

softButtonObject.setStates(stateListDuplicateNames);

assertEquals(stateListUnique, softButtonObject.getStates());
}
}
41 changes: 36 additions & 5 deletions base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import com.smartdevicelink.proxy.rpc.OnButtonPress;
import com.smartdevicelink.proxy.rpc.SoftButton;
import com.smartdevicelink.util.DebugTool;

import java.util.Collections;
import java.util.List;

Expand Down Expand Up @@ -73,12 +72,24 @@ public class SoftButtonObject implements Cloneable{
*/
public SoftButtonObject(@NonNull String name, @NonNull List<SoftButtonState> states, @NonNull String initialStateName, OnEventListener onEventListener) {

// Make sure there aren't two states with the same name
if (hasTwoStatesOfSameName(states)) {
DebugTool.logError(TAG, "Two states have the same name in states list for soft button object");
return;
boolean repeatedStateNames = hasTwoStatesOfSameName(states);

boolean hasStateWithInitialName = false;
for (SoftButtonState state : states) {
if(state.getName().equals(initialStateName)) {
hasStateWithInitialName = true;
break;
}
}

if (repeatedStateNames) {
DebugTool.logError(TAG, "A SoftButtonObject must have states with different names.");
return;
}
if (!hasStateWithInitialName) {
DebugTool.logError(TAG, "A SoftButtonObject must have a state with initialStateName.");
return;
}
this.name = name;
this.states = states;
this.currentStateName = initialStateName;
Expand Down Expand Up @@ -264,6 +275,26 @@ public List<SoftButtonState> getStates() {
* @param states a list of the object's soft button states. <strong>states should be unique for every SoftButtonObject. A SoftButtonState instance cannot be reused for multiple SoftButtonObjects.</strong>
*/
public void setStates(@NonNull List<SoftButtonState> states) {

boolean repeatedStateNames = hasTwoStatesOfSameName(states);

if (repeatedStateNames) {
DebugTool.logError(TAG, "A SoftButtonObject must have states with different names.");
return;
}

boolean hasStateWithCurrentName = false;
for (SoftButtonState state : states) {
if(state.getName().equals(currentStateName)) {
hasStateWithCurrentNameName = true;
Copy link
Contributor

@RHenigan RHenigan Jan 28, 2022

Choose a reason for hiding this comment

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

Suggested change
hasStateWithCurrentNameName = true;
hasStateWithCurrentName = true;

Copy link
Contributor Author

@noah-livio noah-livio Jan 28, 2022

Choose a reason for hiding this comment

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

Good catch, and that'll be easy to fix

Copy link
Contributor

@RHenigan RHenigan Jan 28, 2022

Choose a reason for hiding this comment

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

Also to avoid confusion for the developer we will still want to set the states in the case the currentState is not present in the list, but we will still want to log the error.

We should test and confirm that the SoftButtonManager can handle this error case (trying to upload a SoftButton where the currentState is not in the list)

break;
}
}
if (!hasStateWithCurrentName) {
DebugTool.logError(TAG, "A SoftButtonObject must have a state with currentStateName.");
return;
}

this.states = states;
}

Expand Down