Skip to content

Commit 6522cd5

Browse files
JulianKastJulian Kast
JulianKast
and
Julian Kast
authored
Fix Choice Cells and Menu Cells do not take which properties are available into account for uniqueness (#1684)
* BaseChoiceSetManager, Strip away unsupported textFields from choices * Add image checking to ChoiceSetManager uniqueness check * MenuManager strip parameters away from uniqueness check if hmi does not support fields * added logic to check subcells properly * Add missing ! * Fix copy paste error with naming * Clone choiceCell when comparing for visual uniqueness * Clone menuCell when comparing for visual uniqueness * update comment * Add check for submenu * Refactor check for uniqueness * Add WindowCapability to menuManagerTest * Change menuCellsAreUnique to package private and add unit test * Update unit test * add menuManager test * Clean up BaseChoiceSetManager logic * Add more unit test * Modify menu manger unit test * Formatting and cleanup of unitTest * Added logic for : On RPC v7.1+ connections, we will no longer display duplicate menu cells if some property of the cell is different but that property isn't actually displayed on the head unit. * Remove testing logic that was not meant to be uploaded. * Fix not checking available ChoiceCell properties * Add null checks and remove added unitTest, * Fix subCell logic for MenuCells * Add unit test to menuManager * Remove testing code and add unit test for the choiceSetManager * Update comments/alignment * remove unused import Co-authored-by: Julian Kast <julian@livio.com>
1 parent 9e73aee commit 6522cd5

4 files changed

Lines changed: 340 additions & 21 deletions

File tree

‎android/sdl_android/src/androidTest/java/com/smartdevicelink/managers/screen/choiceset/ChoiceSetManagerTests.java‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,23 @@
4040
import com.smartdevicelink.managers.BaseSubManager;
4141
import com.smartdevicelink.managers.ISdl;
4242
import com.smartdevicelink.managers.file.FileManager;
43+
import com.smartdevicelink.proxy.rpc.ImageField;
4344
import com.smartdevicelink.proxy.rpc.KeyboardCapabilities;
4445
import com.smartdevicelink.proxy.rpc.KeyboardLayoutCapability;
4546
import com.smartdevicelink.proxy.rpc.KeyboardProperties;
4647
import com.smartdevicelink.proxy.rpc.SdlMsgVersion;
48+
import com.smartdevicelink.proxy.rpc.TextField;
4749
import com.smartdevicelink.proxy.rpc.WindowCapability;
4850
import com.smartdevicelink.proxy.rpc.enums.HMILevel;
51+
import com.smartdevicelink.proxy.rpc.enums.ImageFieldName;
4952
import com.smartdevicelink.proxy.rpc.enums.KeyboardInputMask;
5053
import com.smartdevicelink.proxy.rpc.enums.KeyboardLayout;
5154
import com.smartdevicelink.proxy.rpc.enums.KeypressMode;
5255
import com.smartdevicelink.proxy.rpc.enums.Language;
5356
import com.smartdevicelink.proxy.rpc.enums.SystemContext;
57+
import com.smartdevicelink.proxy.rpc.enums.TextFieldName;
5458
import com.smartdevicelink.proxy.rpc.enums.TriggerSource;
59+
import com.smartdevicelink.test.TestValues;
5560

5661
import org.junit.After;
5762
import org.junit.Before;
@@ -231,7 +236,8 @@ public void testAddUniqueNamesToCells() {
231236
ChoiceCell cell4 = new ChoiceCell("McDonalds", "4 mile away", null, null, null, null);
232237
ChoiceCell cell5 = new ChoiceCell("Starbucks", "5 mile away", null, null, null, null);
233238
ChoiceCell cell6 = new ChoiceCell("Meijer", "6 mile away", null, null, null, null);
234-
LinkedHashSet<ChoiceCell> cellList = new LinkedHashSet<>();
239+
List<ChoiceCell> cellList = new ArrayList<>();
240+
235241
cellList.add(cell1);
236242
cellList.add(cell2);
237243
cellList.add(cell3);
@@ -468,4 +474,51 @@ public void testDismissingQueuedKeyboard() {
468474
verify(testKeyboardOp, times(0)).dismissKeyboard();
469475
verify(testKeyboardOp2, times(1)).dismissKeyboard();
470476
}
477+
478+
@Test
479+
public void testUniquenessForAvailableFields() {
480+
WindowCapability windowCapability = new WindowCapability();
481+
TextField secondaryText = new TextField();
482+
secondaryText.setName(TextFieldName.secondaryText);
483+
TextField tertiaryText = new TextField();
484+
tertiaryText.setName(TextFieldName.tertiaryText);
485+
486+
List<TextField> textFields = new ArrayList<>();
487+
textFields.add(secondaryText);
488+
textFields.add(tertiaryText);
489+
windowCapability.setTextFields(textFields);
490+
491+
ImageField choiceImage = new ImageField();
492+
choiceImage.setName(ImageFieldName.choiceImage);
493+
ImageField choiceSecondaryImage = new ImageField();
494+
choiceSecondaryImage.setName(ImageFieldName.choiceSecondaryImage);
495+
List<ImageField> imageFieldList = new ArrayList<>();
496+
imageFieldList.add(choiceImage);
497+
imageFieldList.add(choiceSecondaryImage);
498+
windowCapability.setImageFields(imageFieldList);
499+
500+
csm.defaultMainWindowCapability = windowCapability;
501+
502+
ChoiceCell cell1 = new ChoiceCell("Item 1", "null", "tertiaryText", null, TestValues.GENERAL_ARTWORK, TestValues.GENERAL_ARTWORK);
503+
ChoiceCell cell2 = new ChoiceCell("Item 1", "null2", "tertiaryText2", null, null, null);
504+
List<ChoiceCell> choiceCellList = new ArrayList<>();
505+
choiceCellList.add(cell1);
506+
choiceCellList.add(cell2);
507+
508+
List<ChoiceCell> removedProperties = csm.removeUnusedProperties(choiceCellList);
509+
assertNotNull(removedProperties.get(0).getSecondaryText());
510+
511+
textFields.remove(secondaryText);
512+
textFields.remove(tertiaryText);
513+
imageFieldList.remove(choiceImage);
514+
imageFieldList.remove(choiceSecondaryImage);
515+
516+
removedProperties = csm.removeUnusedProperties(choiceCellList);
517+
csm.addUniqueNamesBasedOnStrippedCells(removedProperties, choiceCellList);
518+
assertEquals(choiceCellList.get(1).getUniqueText(), "Item 1 (2)");
519+
520+
521+
}
522+
523+
471524
}

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

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,22 @@
4343
import com.smartdevicelink.protocol.enums.FunctionID;
4444
import com.smartdevicelink.proxy.RPCRequest;
4545
import com.smartdevicelink.proxy.RPCResponse;
46+
import com.smartdevicelink.proxy.rpc.ImageField;
4647
import com.smartdevicelink.proxy.rpc.OnCommand;
4748
import com.smartdevicelink.proxy.rpc.OnHMIStatus;
4849
import com.smartdevicelink.proxy.rpc.SdlMsgVersion;
4950
import com.smartdevicelink.proxy.rpc.SetGlobalProperties;
51+
import com.smartdevicelink.proxy.rpc.TextField;
5052
import com.smartdevicelink.proxy.rpc.WindowCapability;
5153
import com.smartdevicelink.proxy.rpc.enums.FileType;
5254
import com.smartdevicelink.proxy.rpc.enums.HMILevel;
55+
import com.smartdevicelink.proxy.rpc.enums.ImageFieldName;
5356
import com.smartdevicelink.proxy.rpc.enums.MenuLayout;
5457
import com.smartdevicelink.proxy.rpc.enums.SystemContext;
58+
import com.smartdevicelink.proxy.rpc.enums.TextFieldName;
5559
import com.smartdevicelink.proxy.rpc.enums.TriggerSource;
5660
import com.smartdevicelink.proxy.rpc.listeners.OnRPCNotificationListener;
61+
import com.smartdevicelink.test.TestValues;
5762

5863
import org.junit.After;
5964
import org.junit.Before;
@@ -63,6 +68,7 @@
6368
import org.mockito.invocation.InvocationOnMock;
6469
import org.mockito.stubbing.Answer;
6570

71+
import java.util.ArrayList;
6672
import java.util.Arrays;
6773
import java.util.Collections;
6874
import java.util.List;
@@ -598,6 +604,111 @@ public void testAllowingNonUniqueTitles() {
598604
assertEquals(menuManager.menuCells.get(3).getSubCells().get(3).getUniqueTitle(), "A");
599605
}
600606

607+
@Test
608+
public void testUniquenessForAvailableFields() {
609+
WindowCapability windowCapability = new WindowCapability();
610+
TextField menuSubMenuSecondaryText = new TextField();
611+
menuSubMenuSecondaryText.setName(TextFieldName.menuSubMenuSecondaryText);
612+
TextField menuSubMenuTertiaryText = new TextField();
613+
menuSubMenuTertiaryText.setName(TextFieldName.menuSubMenuTertiaryText);
614+
TextField menuCommandSecondaryText = new TextField();
615+
menuCommandSecondaryText.setName(TextFieldName.menuCommandSecondaryText);
616+
TextField menuCommandTertiaryText = new TextField();
617+
menuCommandTertiaryText.setName(TextFieldName.menuCommandTertiaryText);
618+
List<TextField> textFields = new ArrayList<>();
619+
textFields.add(menuSubMenuSecondaryText);
620+
textFields.add(menuSubMenuTertiaryText);
621+
textFields.add(menuCommandSecondaryText);
622+
textFields.add(menuCommandTertiaryText);
623+
windowCapability.setTextFields(textFields);
624+
625+
ImageField cmdIcon = new ImageField();
626+
cmdIcon.setName(ImageFieldName.cmdIcon);
627+
ImageField menuSubMenuSecondaryImage = new ImageField();
628+
menuSubMenuSecondaryImage.setName(ImageFieldName.menuSubMenuSecondaryImage);
629+
ImageField menuCommandSecondaryImage = new ImageField();
630+
menuCommandSecondaryImage.setName(ImageFieldName.menuCommandSecondaryImage);
631+
List<ImageField> imageFieldList = new ArrayList<>();
632+
imageFieldList.add(cmdIcon);
633+
imageFieldList.add(menuSubMenuSecondaryImage);
634+
imageFieldList.add(menuCommandSecondaryImage);
635+
windowCapability.setImageFields(imageFieldList);
636+
menuManager.defaultMainWindowCapability = windowCapability;
637+
638+
assertNull(menuManager.removeUnusedProperties(null));
639+
640+
MenuCell cell1 = new MenuCell("Text1", "SecondaryText", "TText", TestValues.GENERAL_ARTWORK, TestValues.GENERAL_ARTWORK, null, new MenuSelectionListener() {
641+
@Override
642+
public void onTriggered(TriggerSource trigger) {
643+
644+
}
645+
});
646+
647+
MenuCell cell2 = new MenuCell("Text1", "SecondaryText2", "TText2", null, null, null, new MenuSelectionListener() {
648+
@Override
649+
public void onTriggered(TriggerSource trigger) {
650+
651+
}
652+
});
653+
654+
MenuCell subCell1 = new MenuCell("SubCell1", "Secondary Text", "TText", TestValues.GENERAL_ARTWORK, TestValues.GENERAL_ARTWORK, null, new MenuSelectionListener() {
655+
@Override
656+
public void onTriggered(TriggerSource trigger) {
657+
}
658+
});
659+
660+
MenuCell subCell2 = new MenuCell("SubCell1", "Secondary Text2", "TText2", null, null, null, new MenuSelectionListener() {
661+
@Override
662+
public void onTriggered(TriggerSource trigger) {
663+
}
664+
});
665+
666+
List<MenuCell> subCellList = new ArrayList<>();
667+
subCellList.add(subCell1);
668+
subCellList.add(subCell2);
669+
670+
671+
MenuCell cell3 = new MenuCell("Test Cell 3 (sub menu)", "SecondaryText", "TText", MenuLayout.LIST, TestValues.GENERAL_ARTWORK, TestValues.GENERAL_ARTWORK, subCellList);
672+
MenuCell cell4 = new MenuCell("Test Cell 3 (sub menu)", null, null, MenuLayout.LIST, null, null, subCellList);
673+
674+
List<MenuCell> menuCellList = new ArrayList<>();
675+
menuCellList.add(cell1);
676+
menuCellList.add(cell2);
677+
menuCellList.add(cell3);
678+
menuCellList.add(cell4);
679+
680+
List<MenuCell> removedProperties = menuManager.removeUnusedProperties(menuCellList);
681+
assertNotNull(removedProperties.get(0).getSecondaryText());
682+
menuManager.addUniqueNamesBasedOnStrippedCells(removedProperties, menuCellList);
683+
assertEquals(menuCellList.get(1).getUniqueTitle(), "Text1");
684+
685+
// Remove menuCommandSecondaryText as a supported TextField
686+
textFields.remove(menuCommandSecondaryText);
687+
textFields.remove(menuCommandTertiaryText);
688+
imageFieldList.remove(cmdIcon);
689+
imageFieldList.remove(menuCommandSecondaryImage);
690+
imageFieldList.remove(menuSubMenuSecondaryImage);
691+
textFields.remove(menuSubMenuSecondaryText);
692+
textFields.remove(menuSubMenuTertiaryText);
693+
textFields.remove(menuSubMenuSecondaryImage);
694+
695+
// Test removeUnusedProperties
696+
removedProperties = menuManager.removeUnusedProperties(menuCellList);
697+
assertNull(removedProperties.get(0).getSecondaryText());
698+
assertNull(removedProperties.get(0).getTertiaryText());
699+
700+
menuManager.addUniqueNamesBasedOnStrippedCells(removedProperties, menuCellList);
701+
assertEquals(menuCellList.get(1).getUniqueTitle(), "Text1 (2)");
702+
703+
// SubCell test
704+
assertEquals(menuCellList.get(3).getUniqueTitle(), "Test Cell 3 (sub menu) (2)");
705+
assertEquals(menuCellList.get(2).getSubCells().get(1).getUniqueTitle(), "SubCell1 (2)");
706+
707+
708+
}
709+
710+
711+
601712
// HELPERS
602713

603714
// Emulate what happens when Core sends OnHMIStatus notification
@@ -849,4 +960,5 @@ private List<MenuCell> createDynamicMenu6_forUniqueNamesTest() {
849960
return Arrays.asList(A, B, C, D);
850961
}
851962

963+
852964
}

‎base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java‎

Lines changed: 83 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.smartdevicelink.managers.BaseSubManager;
4343
import com.smartdevicelink.managers.CompletionListener;
4444
import com.smartdevicelink.managers.ISdl;
45+
import com.smartdevicelink.managers.ManagerUtility;
4546
import com.smartdevicelink.managers.file.FileManager;
4647
import com.smartdevicelink.managers.lifecycle.OnSystemCapabilityListener;
4748
import com.smartdevicelink.managers.lifecycle.SystemCapabilityManager;
@@ -54,18 +55,21 @@
5455
import com.smartdevicelink.proxy.rpc.OnHMIStatus;
5556
import com.smartdevicelink.proxy.rpc.WindowCapability;
5657
import com.smartdevicelink.proxy.rpc.enums.HMILevel;
58+
import com.smartdevicelink.proxy.rpc.enums.ImageFieldName;
5759
import com.smartdevicelink.proxy.rpc.enums.InteractionMode;
5860
import com.smartdevicelink.proxy.rpc.enums.KeyboardLayout;
5961
import com.smartdevicelink.proxy.rpc.enums.KeypressMode;
6062
import com.smartdevicelink.proxy.rpc.enums.Language;
6163
import com.smartdevicelink.proxy.rpc.enums.PredefinedWindows;
6264
import com.smartdevicelink.proxy.rpc.enums.SystemCapabilityType;
6365
import com.smartdevicelink.proxy.rpc.enums.SystemContext;
66+
import com.smartdevicelink.proxy.rpc.enums.TextFieldName;
6467
import com.smartdevicelink.proxy.rpc.enums.TriggerSource;
6568
import com.smartdevicelink.proxy.rpc.listeners.OnRPCNotificationListener;
6669
import com.smartdevicelink.util.DebugTool;
6770

6871
import java.lang.ref.WeakReference;
72+
import java.util.ArrayList;
6973
import java.util.HashMap;
7074
import java.util.HashSet;
7175
import java.util.LinkedHashSet;
@@ -540,7 +544,7 @@ HashSet<ChoiceCell> choicesToBeRemovedFromPendingWithArray(List<ChoiceCell> choi
540544
* E.g. Choices param contains 2 cells with text/title "Address" will be handled by updating the uniqueText/uniqueTitle of the second cell to "Address (2)".
541545
* @param choices The list of choiceCells to be uploaded.
542546
*/
543-
void addUniqueNamesToCells(LinkedHashSet<ChoiceCell> choices) {
547+
void addUniqueNamesToCells(List<ChoiceCell> choices) {
544548
HashMap<String, Integer> dictCounter = new HashMap<>();
545549

546550
for (ChoiceCell cell : choices) {
@@ -556,16 +560,80 @@ void addUniqueNamesToCells(LinkedHashSet<ChoiceCell> choices) {
556560
}
557561
}
558562

563+
void addUniqueNamesBasedOnStrippedCells(List<ChoiceCell> strippedCells, List<ChoiceCell> unstrippedCells) {
564+
if (strippedCells == null || unstrippedCells == null || strippedCells.size() != unstrippedCells.size()) {
565+
return;
566+
}
567+
// Tracks how many of each cell primary text there are so that we can append numbers to make each unique as necessary
568+
HashMap<ChoiceCell, Integer> dictCounter = new HashMap<>();
569+
for (int i = 0; i < strippedCells.size(); i++) {
570+
ChoiceCell cell = strippedCells.get(i);
571+
Integer counter = dictCounter.get(cell);
572+
if (counter != null) {
573+
counter++;
574+
dictCounter.put(cell, counter);
575+
} else {
576+
dictCounter.put(cell, 1);
577+
}
578+
579+
counter = dictCounter.get(cell);
580+
581+
if (counter > 1) {
582+
unstrippedCells.get(i).setUniqueText(unstrippedCells.get(i).getText() + " (" + counter + ")");
583+
}
584+
}
585+
}
586+
587+
private List<ChoiceCell> cloneChoiceCellList(List<ChoiceCell> originalList) {
588+
if (originalList == null) {
589+
return null;
590+
}
591+
592+
List<ChoiceCell> clone = new ArrayList<>();
593+
for (ChoiceCell choiceCell : originalList) {
594+
clone.add(choiceCell.clone());
595+
}
596+
return clone;
597+
}
598+
559599
private LinkedHashSet<ChoiceCell> getChoicesToBeUploadedWithArray(List<ChoiceCell> choices) {
560-
LinkedHashSet<ChoiceCell> choiceSet = new LinkedHashSet<>(choices);
561-
// If we're running on a connection < RPC 7.1, we need to de-duplicate cells because presenting them will fail if we have the same cell primary text.
600+
// Clone choices
601+
List<ChoiceCell> choicesClone = cloneChoiceCellList(choices);
562602
if (choices != null && internalInterface.getSdlMsgVersion() != null
563603
&& (internalInterface.getSdlMsgVersion().getMajorVersion() < 7
564604
|| (internalInterface.getSdlMsgVersion().getMajorVersion() == 7 && internalInterface.getSdlMsgVersion().getMinorVersion() == 0))) {
565-
addUniqueNamesToCells(choiceSet);
605+
// If we're on < RPC 7.1, all primary texts need to be unique, so we don't need to check removed properties and duplicate cells
606+
addUniqueNamesToCells(choicesClone);
607+
} else {
608+
List<ChoiceCell> strippedCellsClone = removeUnusedProperties(choicesClone);
609+
addUniqueNamesBasedOnStrippedCells(strippedCellsClone, choicesClone);
566610
}
567-
choiceSet.removeAll(preloadedChoices);
568-
return choiceSet;
611+
LinkedHashSet<ChoiceCell> choiceCloneLinkedHash = new LinkedHashSet<>(choicesClone);
612+
choiceCloneLinkedHash.removeAll(preloadedChoices);
613+
return choiceCloneLinkedHash;
614+
}
615+
616+
List<ChoiceCell> removeUnusedProperties(List<ChoiceCell> choiceCells) {
617+
List<ChoiceCell> strippedCellsClone = cloneChoiceCellList(choiceCells);
618+
//Clone Cells
619+
for (ChoiceCell cell : strippedCellsClone) {
620+
// Strip away fields that cannot be used to determine uniqueness visually including fields not supported by the HMI
621+
cell.setVoiceCommands(null);
622+
623+
if (!hasImageFieldOfName(ImageFieldName.choiceImage)) {
624+
cell.setArtwork(null);
625+
}
626+
if (!hasTextFieldOfName(TextFieldName.secondaryText)) {
627+
cell.setSecondaryText(null);
628+
}
629+
if (!hasTextFieldOfName(TextFieldName.tertiaryText)) {
630+
cell.setTertiaryText(null);
631+
}
632+
if (!hasImageFieldOfName(ImageFieldName.choiceSecondaryImage)) {
633+
cell.setSecondaryArtwork(null);
634+
}
635+
}
636+
return strippedCellsClone;
569637
}
570638

571639
void updateIdsOnChoices(LinkedHashSet<ChoiceCell> choices) {
@@ -672,6 +740,14 @@ public void onNotified(RPCNotification notification) {
672740

673741
// ADDITIONAL HELPERS
674742

743+
private boolean hasImageFieldOfName(ImageFieldName imageFieldName) {
744+
return defaultMainWindowCapability == null || ManagerUtility.WindowCapabilityUtility.hasImageFieldOfName(defaultMainWindowCapability, imageFieldName);
745+
}
746+
747+
private boolean hasTextFieldOfName(TextFieldName textFieldName) {
748+
return defaultMainWindowCapability == null || ManagerUtility.WindowCapabilityUtility.hasTextFieldOfName(defaultMainWindowCapability, textFieldName);
749+
}
750+
675751
boolean setUpChoiceSet(ChoiceSet choiceSet) {
676752

677753
List<ChoiceCell> choices = choiceSet.getChoices();
@@ -695,9 +771,8 @@ boolean setUpChoiceSet(ChoiceSet choiceSet) {
695771
int choiceCellWithVoiceCommandCount = 0;
696772

697773
for (ChoiceCell cell : choices) {
698-
699774
uniqueChoiceCells.add(cell);
700-
775+
// Not using cloned cell here because we set the clone's VoiceCommands to null for visual check only
701776
if (cell.getVoiceCommands() != null) {
702777
uniqueVoiceCommands.addAll(cell.getVoiceCommands());
703778
choiceCellWithVoiceCommandCount += 1;

0 commit comments

Comments
 (0)