Skip to content

Commit 611d350

Browse files
bilal-alsharifibilal-alsharifi
authored
Merge pull request #1691 from smartdevicelink/bugfix/issue_1676
Fix to avoid deleting and setting identical voice commands
2 parents 08737a9 + 106e3c8 commit 611d350

5 files changed

Lines changed: 136 additions & 20 deletions

File tree

‎android/sdl_android/src/androidTest/java/com/smartdevicelink/managers/screen/menu/VoiceCommandManagerTests.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import com.smartdevicelink.managers.CompletionListener;
4040
import com.smartdevicelink.managers.ISdl;
4141
import com.smartdevicelink.protocol.enums.FunctionID;
42+
import com.smartdevicelink.proxy.RPCRequest;
4243
import com.smartdevicelink.proxy.rpc.OnCommand;
4344
import com.smartdevicelink.proxy.rpc.OnHMIStatus;
4445
import com.smartdevicelink.proxy.rpc.enums.HMILevel;
@@ -55,6 +56,7 @@
5556
import java.util.ArrayList;
5657
import java.util.Arrays;
5758
import java.util.Collections;
59+
import java.util.HashMap;
5860
import java.util.List;
5961

6062
import static junit.framework.TestCase.assertEquals;
@@ -175,10 +177,12 @@ public void testHMINotReady() {
175177
public void testUpdatingCommands() {
176178
// Send a new single command, and test that its listener works, as it gets called from the VCM
177179
voiceCommandManager.setVoiceCommands(Collections.singletonList(command3));
180+
HashMap<RPCRequest, String> errorObject = new HashMap<>();
181+
voiceCommandManager.updateOperation.voiceCommandListener.updateVoiceCommands(voiceCommandManager.voiceCommands, errorObject);
178182

179183
// Fake onCommand - we want to make sure that we can pass back onCommand events to our VoiceCommand Objects
180184
OnCommand onCommand = new OnCommand();
181-
onCommand.setCmdID(command3.getCommandId());
185+
onCommand.setCmdID(voiceCommandManager.getVoiceCommands().get(voiceCommandManager.getVoiceCommands().indexOf(command3)).getCommandId());
182186
onCommand.setTriggerSource(TriggerSource.TS_VR); // these are voice commands
183187
commandListener.onNotified(onCommand); // send off the notification
184188

‎android/sdl_android/src/androidTest/java/com/smartdevicelink/managers/screen/menu/VoiceCommandUpdateOperationTests.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.junit.Before;
1515
import org.junit.Test;
1616
import org.junit.runner.RunWith;
17+
import org.mockito.ArgumentMatchers;
1718
import org.mockito.Mockito;
1819
import org.mockito.invocation.InvocationOnMock;
1920
import org.mockito.stubbing.Answer;
@@ -74,6 +75,7 @@ public void onVoiceCommandSelected() {
7475
List<VoiceCommand> deleteList = new ArrayList<>();
7576
List<VoiceCommand> addList = new ArrayList<>();
7677

78+
7779
@Before
7880
public void setup() {
7981
deleteList.clear();
@@ -294,4 +296,50 @@ public void updateVoiceCommands(List<VoiceCommand> newCurrentVoiceCommands, Hash
294296

295297
verify(listenerSpy, times(1)).updateVoiceCommands(any(List.class), any(HashMap.class));
296298
}
299+
300+
301+
302+
@Test
303+
public void testVoiceCommandsInListNotInSecondList() {
304+
VoiceCommand command1 = new VoiceCommand(Collections.singletonList("Command 1"), null);
305+
VoiceCommand command2 = new VoiceCommand(Collections.singletonList("Command 2"), null);
306+
VoiceCommand command3 = new VoiceCommand(Collections.singletonList("Command 3"), null);
307+
308+
VoiceCommand command1Clone = new VoiceCommand(Collections.singletonList("Command 1"), null);
309+
310+
List<VoiceCommand> voiceCommandList = new ArrayList<>();
311+
voiceCommandList.add(command1);
312+
voiceCommandList.add(command2);
313+
314+
List<VoiceCommand> voiceCommandList2 = new ArrayList<>();
315+
voiceCommandList2.add(command1Clone);
316+
voiceCommandList2.add(command3);
317+
VoiceCommandUpdateOperation voiceCommandUpdateOperation = new VoiceCommandUpdateOperation(internalInterface,null,null,null);
318+
319+
List<VoiceCommand> differencesList = voiceCommandUpdateOperation.voiceCommandsInListNotInSecondList(voiceCommandList, voiceCommandList2);
320+
assertEquals(differencesList.size(), 1);
321+
}
322+
323+
@Test
324+
public void testDelete() {
325+
internalInterface = mock(ISdl.class);
326+
327+
VoiceCommand command1 = new VoiceCommand(Collections.singletonList("Command 1"), null);
328+
VoiceCommand command2 = new VoiceCommand(Collections.singletonList("Command 2"), null);
329+
330+
VoiceCommand command1Clone = new VoiceCommand(Collections.singletonList("Command 1"), null);
331+
VoiceCommand command3 = new VoiceCommand(Collections.singletonList("Command 3"), null);
332+
333+
List<VoiceCommand> voiceCommandList = new ArrayList<>();
334+
voiceCommandList.add(command1);
335+
voiceCommandList.add(command2);
336+
337+
List<VoiceCommand> voiceCommandList2 = new ArrayList<>();
338+
voiceCommandList2.add(command1Clone);
339+
voiceCommandList2.add(command3);
340+
VoiceCommandUpdateOperation voiceCommandUpdateOperation = new VoiceCommandUpdateOperation(internalInterface, voiceCommandList, voiceCommandList2, null);
341+
voiceCommandUpdateOperation.onExecute();
342+
verify(internalInterface, times(1)).sendRPCs(ArgumentMatchers.<DeleteCommand>anyList(), any(OnMultipleRequestListener.class));
343+
}
344+
297345
}

‎base/src/main/java/com/smartdevicelink/managers/screen/menu/BaseVoiceCommandManager.java‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656

5757
abstract class BaseVoiceCommandManager extends BaseSubManager {
5858
private static final String TAG = "BaseVoiceCommandManager";
59-
List<VoiceCommand> voiceCommands, currentVoiceCommands, originalVoiceCommands;
59+
List<VoiceCommand> voiceCommands, currentVoiceCommands;
6060

6161
int lastVoiceCommandId;
6262
private static final int voiceCommandIdMin = 1900000000;
@@ -132,25 +132,36 @@ private void updateTransactionQueueSuspended() {
132132
public void setVoiceCommands(List<VoiceCommand> voiceCommands) {
133133

134134
// we actually need voice commands to set.
135-
if (voiceCommands == null || voiceCommands.equals(this.originalVoiceCommands)) {
136-
DebugTool.logInfo(TAG, "Voice commands list was null or matches the current voice commands");
135+
if (voiceCommands == null) {
136+
DebugTool.logInfo(TAG, "Voice commands list was null");
137137
return;
138138
}
139139

140-
List<VoiceCommand> validatedVoiceCommands = removeEmptyVoiceCommands(voiceCommands);
140+
// Clone voice commands
141+
this.voiceCommands = new ArrayList<>();
142+
for (VoiceCommand voiceCommand : voiceCommands) {
143+
if (voiceCommand == null) {
144+
continue;
145+
}
146+
this.voiceCommands.add(voiceCommand.clone());
147+
}
148+
149+
List<VoiceCommand> validatedVoiceCommands = removeEmptyVoiceCommands(this.voiceCommands);
141150

142-
if (validatedVoiceCommands.size() == 0) {
151+
if (validatedVoiceCommands.size() == 0 && voiceCommands.size() > 0) {
143152
DebugTool.logError(TAG, "New voice commands are invalid, skipping...");
153+
this.voiceCommands = null;
144154
return;
145155
}
146156

147157
if (!isVoiceCommandsUnique(validatedVoiceCommands)) {
148158
DebugTool.logError(TAG, "Not all voice command strings are unique across all voice commands. Voice commands will not be set.");
159+
this.voiceCommands = null;
149160
return;
150161
}
151162

152-
this.originalVoiceCommands = new ArrayList<>(voiceCommands);
153163
this.voiceCommands = validatedVoiceCommands;
164+
154165
updateIdsOnVoiceCommands(this.voiceCommands);
155166

156167
cleanTransactionQueue();
@@ -190,7 +201,7 @@ private void updatePendingOperations(List<VoiceCommand> newCurrentVoiceCommands)
190201
continue;
191202
}
192203
VoiceCommandUpdateOperation vcOperation = (VoiceCommandUpdateOperation) operation;
193-
vcOperation.oldVoiceCommands = newCurrentVoiceCommands;
204+
vcOperation.setOldVoiceCommands(newCurrentVoiceCommands);
194205
}
195206
}
196207

@@ -249,8 +260,8 @@ public void onNotified(RPCNotification notification) {
249260
@Override
250261
public void onNotified(RPCNotification notification) {
251262
OnCommand onCommand = (OnCommand) notification;
252-
if (voiceCommands != null && voiceCommands.size() > 0) {
253-
for (VoiceCommand command : voiceCommands) {
263+
if (currentVoiceCommands != null && currentVoiceCommands.size() > 0) {
264+
for (VoiceCommand command : currentVoiceCommands) {
254265
if (onCommand.getCmdID() == command.getCommandId()) {
255266
if (command.getVoiceCommandSelectionListener() != null) {
256267
command.getVoiceCommandSelectionListener().onVoiceCommandSelected();

‎base/src/main/java/com/smartdevicelink/managers/screen/menu/VoiceCommand.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,13 @@
3636
import androidx.annotation.Nullable;
3737

3838

39+
import com.smartdevicelink.util.DebugTool;
40+
3941
import java.util.ArrayList;
4042
import java.util.HashSet;
4143
import java.util.List;
4244

43-
public class VoiceCommand {
45+
public class VoiceCommand implements Cloneable {
4446

4547
/**
4648
* The strings the user can say to activate this voice command
@@ -140,9 +142,8 @@ private HashSet<String> removeDuplicateStrings(List<String> voiceCommands) {
140142
@Override
141143
public int hashCode() {
142144
int result = 1;
143-
result += Integer.rotateLeft(getCommandId(), 1);
144145
for (int i = 0; i < this.getVoiceCommands().size(); i++) {
145-
result += ((getVoiceCommands().get(i) == null) ? 0 : Integer.rotateLeft(getVoiceCommands().get(i).hashCode(), i + 2));
146+
result += ((getVoiceCommands().get(i) == null) ? 0 : Integer.rotateLeft(getVoiceCommands().get(i).hashCode(), i + 1));
146147
}
147148
return result;
148149
}
@@ -158,9 +159,27 @@ public boolean equals(Object o) {
158159
if (o == null) return false;
159160
// if this is the same memory address, it's the same
160161
if (this == o) return true;
161-
// if this is not an instance of SoftButtonObject, not the same
162+
// if this is not an instance of VoiceCommand, not the same
162163
if (!(o instanceof VoiceCommand)) return false;
163164
// return comparison
164165
return hashCode() == o.hashCode();
165166
}
167+
168+
/**
169+
* Creates a deep copy of the object
170+
*
171+
* @return deep copy of the object, null if an exception occurred
172+
*/
173+
@Override
174+
public VoiceCommand clone() {
175+
try {
176+
VoiceCommand clone = (VoiceCommand) super.clone();
177+
return clone;
178+
} catch (CloneNotSupportedException e) {
179+
if (DebugTool.isDebugEnabled()) {
180+
throw new RuntimeException("Clone not supported by super class");
181+
}
182+
}
183+
return null;
184+
}
166185
}

‎base/src/main/java/com/smartdevicelink/managers/screen/menu/VoiceCommandUpdateOperation.java‎

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ class VoiceCommandUpdateOperation extends Task {
2222
private List<VoiceCommand> pendingVoiceCommands;
2323
private List<DeleteCommand> deleteVoiceCommands;
2424
private List<AddCommand> addCommandsToSend;
25-
private VoiceCommandChangesListener voiceCommandListener;
25+
VoiceCommandChangesListener voiceCommandListener;
2626
private List<VoiceCommand> currentVoiceCommands;
2727
private HashMap<RPCRequest, String> errorObject;
2828

@@ -53,6 +53,15 @@ private void start() {
5353
onFinished();
5454
return;
5555
}
56+
// Check if a voiceCommand has already been uploaded and update its VoiceCommandSelectionListener to
57+
// prevent calling the wrong listener in a case where a voice command was uploaded and then its voiceCommandSelectionListener was updated in another upload.
58+
if (pendingVoiceCommands != null && pendingVoiceCommands.size() > 0) {
59+
for (VoiceCommand voiceCommand : pendingVoiceCommands) {
60+
if (currentVoiceCommands.contains(voiceCommand)) {
61+
currentVoiceCommands.get(currentVoiceCommands.indexOf(voiceCommand)).setVoiceCommandSelectionListener(voiceCommand.getVoiceCommandSelectionListener());
62+
}
63+
}
64+
}
5665

5766
sendDeleteCurrentVoiceCommands(new CompletionListener() {
5867
@Override
@@ -82,14 +91,23 @@ public void onComplete(boolean success2) {
8291

8392
private void sendDeleteCurrentVoiceCommands(final CompletionListener listener) {
8493

85-
if (oldVoiceCommands == null || oldVoiceCommands.isEmpty()) {
94+
if (oldVoiceCommands == null || oldVoiceCommands.size() == 0) {
8695
if (listener != null) {
8796
listener.onComplete(true);
8897
}
8998
return;
9099
}
91100

92-
deleteVoiceCommands = deleteCommandsForVoiceCommands(oldVoiceCommands);
101+
List<VoiceCommand> voiceCommandsToDelete = voiceCommandsInListNotInSecondList(oldVoiceCommands, pendingVoiceCommands);
102+
103+
if (voiceCommandsToDelete.size() == 0) {
104+
if (listener != null) {
105+
listener.onComplete(true);
106+
}
107+
return;
108+
}
109+
110+
deleteVoiceCommands = deleteCommandsForVoiceCommands(voiceCommandsToDelete);
93111

94112
internalInterface.get().sendRPCs(deleteVoiceCommands, new OnMultipleRequestListener() {
95113
@Override
@@ -156,14 +174,16 @@ private void removeCurrentVoiceCommand(Integer commandId) {
156174

157175
private void sendCurrentVoiceCommands(final CompletionListener listener) {
158176

159-
if (pendingVoiceCommands == null || pendingVoiceCommands.size() == 0) {
177+
List<VoiceCommand> voiceCommandsToAdd = voiceCommandsInListNotInSecondList(pendingVoiceCommands, oldVoiceCommands);
178+
179+
if (voiceCommandsToAdd.size() == 0) {
160180
if (listener != null) {
161-
listener.onComplete(true); // no voice commands to send doesnt mean that its an error
181+
listener.onComplete(true); // no voice commands to send doesn't mean that its an error
162182
}
163183
return;
164184
}
165185

166-
addCommandsToSend = addCommandsForVoiceCommands(pendingVoiceCommands);
186+
addCommandsToSend = addCommandsForVoiceCommands(voiceCommandsToAdd);
167187

168188
internalInterface.get().sendRPCs(addCommandsToSend, new OnMultipleRequestListener() {
169189
@Override
@@ -207,6 +227,20 @@ public void onResponse(int correlationId, RPCResponse response) {
207227
});
208228
}
209229

230+
List<VoiceCommand> voiceCommandsInListNotInSecondList(List<VoiceCommand> firstList, List<VoiceCommand> secondList) {
231+
if (secondList == null || secondList.size() == 0) {
232+
return firstList;
233+
}
234+
List<VoiceCommand> differencesList = new ArrayList<>(firstList);
235+
differencesList.removeAll(secondList);
236+
return differencesList;
237+
}
238+
239+
public void setOldVoiceCommands(List<VoiceCommand> oldVoiceCommands) {
240+
this.oldVoiceCommands = oldVoiceCommands;
241+
this.currentVoiceCommands = new ArrayList<>(oldVoiceCommands);
242+
}
243+
210244
// Create AddCommand List
211245

212246
List<AddCommand> addCommandsForVoiceCommands(List<VoiceCommand> voiceCommands) {

0 commit comments

Comments
 (0)