fixes per more review comments

6 files changed, 86 insertions, 19 deletions

diff --git a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
index 66fbf7d4e..19e035f2b 100644
--- a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
+++ b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
@@ -59,6 +59,7 @@ import com.smartdevicelink.proxy.rpc.listeners.OnRPCNotificationListener;
 import com.smartdevicelink.util.DebugTool;
 
 import java.lang.ref.WeakReference;
+import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.concurrent.Future;
@@ -75,17 +76,18 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
     // additional state
     private static final int CHECKING_VOICE = 0xA0;
     private KeyboardProperties keyboardConfiguration;
+    final WeakReference<FileManager> fileManager;
 
     OnRPCNotificationListener hmiListener;
     OnSystemCapabilityListener displayListener;
-
-    final WeakReference<FileManager> fileManager;
     HMILevel currentHMILevel;
     DisplayCapabilities displayCapabilities;
-
+    SystemContext currentSystemContext;
     HashSet<ChoiceCell> preloadedChoices, pendingPreloadChoices;
     ChoiceSet pendingPresentationSet;
-    Boolean isVROptional;
+    private List<ChoiceCell> waitingChoices;
+    private CompletionListener waitingListener;
+
     // We will pass operations into this to be completed
     PausableThreadPoolExecutor executor;
     LinkedBlockingQueue<Runnable> operationQueue;
@@ -93,8 +95,7 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
 
     int nextChoiceId;
     final int choiceCellIdMin = 1;
-
-	SystemContext currentSystemContext;
+    boolean isVROptional;
 
     BaseChoiceSetManager(@NonNull ISdl internalInterface, @NonNull FileManager fileManager) {
         super(internalInterface);
@@ -136,6 +137,8 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
 
         pendingPresentationSet = null;
         pendingPresentOperation = null;
+        waitingChoices = null;
+        waitingListener = null;
         isVROptional = true;
         nextChoiceId = choiceCellIdMin;
 
@@ -171,8 +174,20 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
         executor.submit(checkChoiceVR);
     }
 
+    /**
+     * Preload choices to improve performance while presenting a choice set at a later time
+     * @param choices - a list of ChoiceCell objects that will be part of a choice set later
+     * @param listener - a completion listener to inform when the operation is complete
+     */
     public void preloadChoices(@NonNull List<ChoiceCell> choices, @Nullable final CompletionListener listener){
 
+        if (!isReady()){
+            waitingChoices = new ArrayList<>(choices);
+            waitingListener = listener;
+            DebugTool.logInfo("Preload pending choice set manager being ready");
+            return;
+        }
+
         final HashSet<ChoiceCell> choicesToUpload = new HashSet<>(choices);
         choicesToUpload.removeAll(preloadedChoices);
         choicesToUpload.removeAll(pendingPreloadChoices);
@@ -199,11 +214,15 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
                         if (listener != null){
                             listener.onComplete(true);
                         }
+                        waitingChoices = null;
+                        waitingListener = null;
                     }else {
                         DebugTool.logError("There was an error pre loading choice cells");
                         if (listener != null){
                             listener.onComplete(false);
                         }
+                        waitingChoices = null;
+                        waitingListener = null;
                     }
                 }
             });
@@ -214,6 +233,10 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
         }
     }
 
+    /**
+     * Deletes choices that were sent previously
+     * @param choices - A list of ChoiceCell objects
+     */
     public void deleteChoices(@NonNull List<ChoiceCell> choices){
 
         if (!isReady()){ return; }
@@ -260,6 +283,12 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
         executor.submit(deleteChoicesOperation);
     }
 
+    /**
+     * Presents a choice set
+     * @param choiceSet - The choice set to be presented. This can include Choice Cells that were preloaded or not
+     * @param mode - The intended interaction mode
+     * @param keyboardListener - A keyboard listener to capture user input
+     */
     public void presentChoiceSet(@NonNull final ChoiceSet choiceSet, @Nullable final InteractionMode mode, @Nullable final KeyboardListener keyboardListener){
 
         if (!isReady()){ return; }
@@ -328,6 +357,12 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
         pendingPresentOperation = executor.submit(presentOp);
     }
 
+    /**
+     * Presents a keyboard on the Head unit to capture user input
+     * @param initialText - The initial text that is used as a placeholder text. It might not work on some head units.
+     * @param customKeyboardConfig - the custom keyboard configuration to be used when the keyboard is displayed
+     * @param listener - A keyboard listener to capture user input
+     */
     public void presentKeyboard(@NonNull String initialText, @Nullable KeyboardProperties customKeyboardConfig, @NonNull KeyboardListener listener){
 
         if (!isReady()){ return; }
@@ -352,6 +387,10 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
         pendingPresentOperation = executor.submit(keyboardOp);
     }
 
+    /**
+     * Set a custom keyboard configuration for this session. If set to null, it will reset to default keyboard configuration.
+     * @param keyboardConfiguration - the custom keyboard configuration to be used when the keyboard is displayed
+     */
     public void setKeyboardConfiguration(@Nullable KeyboardProperties keyboardConfiguration){
         if (keyboardConfiguration == null){
             this.keyboardConfiguration = defaultKeyboardConfiguration();
@@ -368,6 +407,9 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
 
     // GETTERS
 
+    /**
+     * @return A set of choice cells that have been preloaded to the head unit
+     */
     public HashSet<ChoiceCell> getPreloadedChoices(){
         return this.preloadedChoices;
     }
@@ -452,6 +494,10 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
 
                 if (oldHMILevel.equals(HMILevel.HMI_NONE) && !currentHMILevel.equals(HMILevel.HMI_NONE)){
                     executor.resume();
+                    if (waitingChoices != null && waitingChoices.size() > 0){
+                        DebugTool.logInfo("Pending Preload Choices now being sent");
+                        preloadChoices(waitingChoices, waitingListener);
+                    }
                 }
 
                 currentSystemContext = hmiStatus.getSystemContext();
@@ -462,6 +508,10 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
 
                 if (currentSystemContext.equals(SystemContext.SYSCTXT_MAIN) && !currentHMILevel.equals(HMILevel.HMI_NONE)){
                     executor.resume();
+                    if (waitingChoices != null && waitingChoices.size() > 0){
+                        DebugTool.logInfo("Pending Preload Choices now being sent");
+                        preloadChoices(waitingChoices, waitingListener);
+                    }
                 }
 
             }
@@ -535,7 +585,7 @@ abstract class BaseChoiceSetManager extends BaseSubManager {
     @SuppressWarnings("BooleanMethodIsAlwaysInverted")
     private boolean isReady(){
         if (getState() != READY){
-            DebugTool.logError("Choice Manager In Not-Ready State: "+ getState());
+            DebugTool.logError("Choice Manager In Not-Ready State");
             return false;
         }
         return true;
diff --git a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/CheckChoiceVROptionalOperation.java b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/CheckChoiceVROptionalOperation.java
index c49522c25..b9551a6d9 100644
--- a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/CheckChoiceVROptionalOperation.java
+++ b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/CheckChoiceVROptionalOperation.java
@@ -150,6 +150,14 @@ class CheckChoiceVROptionalOperation implements Runnable {
 					checkChoiceVROptionalInterface.onCheckChoiceVROperationComplete(isVROptional);
 				}
 			}
+
+			@Override
+			public void onError(int correlationId, Result resultCode, String info){
+				DebugTool.logError("There was an error presenting the keyboard. Finishing operation - choice set manager - . Error: " + info);
+				if (checkChoiceVROptionalInterface != null){
+					checkChoiceVROptionalInterface.onError(info);
+				}
+			}
 		});
 		if (internalInterface.get() != null){
 			internalInterface.get().sendRPC(delete);
diff --git a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/ChoiceCell.java b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/ChoiceCell.java
index 5a74b94f5..c96825c56 100644
--- a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/ChoiceCell.java
+++ b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/ChoiceCell.java
@@ -237,23 +237,24 @@ public class ChoiceCell {
      */
     @Override
     public boolean equals(Object o) {
+        if (o == null) { return false; }
         // if this is the same memory address, its the same
         if (this == o) return true;
         // if this is not an instance of this class, not the same
         if (!(o instanceof ChoiceCell)) return false;
-
+        // return comparison
         return hashCode() == o.hashCode();
     }
 
     /**
-     * Overriding toString was throwing a warning in AS, so I changed the name for now
      * @return A string description of the cell, useful for debugging.
      */
-    public String getDescription() {
-        return "ChoiceCell: ID: " + this.choiceId + " Text: " + text+ " - "+ secondaryText+" - "+ " - "+ tertiaryText+ " " +
+    @Override @NonNull
+    public String toString() {
+        return "ChoiceCell: ID: " + this.choiceId + " Text: " + text+ " - Secondary Text: "+ secondaryText+" - Tertiary Text: "+ tertiaryText+ " " +
                 "| Artwork Names: "+ ((getArtwork() == null || getArtwork().getName() == null) ? "Primary Art null" : getArtwork().getName())
-                + " - "+((getSecondaryArtwork() == null || getSecondaryArtwork().getName() == null) ? "Secondary Art null" : getSecondaryArtwork().getName()) +
-                " Voice Commands Size: "+ ((getVoiceCommands() == null) ? 0 : getVoiceCommands().size());
+                + " Secondary Art - "+((getSecondaryArtwork() == null || getSecondaryArtwork().getName() == null) ? "Secondary Art null" : getSecondaryArtwork().getName()) +
+                " | Voice Commands Size: "+ ((getVoiceCommands() == null) ? 0 : getVoiceCommands().size());
     }
 
 }
diff --git a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/PresentKeyboardOperation.java b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/PresentKeyboardOperation.java
index f5eef7bd8..a2fcc8f0d 100644
--- a/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/PresentKeyboardOperation.java
+++ b/base/src/main/java/com/smartdevicelink/managers/screen/choiceset/PresentKeyboardOperation.java
@@ -47,6 +47,7 @@ import com.smartdevicelink.proxy.rpc.SetGlobalProperties;
 import com.smartdevicelink.proxy.rpc.enums.InteractionMode;
 import com.smartdevicelink.proxy.rpc.enums.KeyboardEvent;
 import com.smartdevicelink.proxy.rpc.enums.LayoutMode;
+import com.smartdevicelink.proxy.rpc.enums.Result;
 import com.smartdevicelink.proxy.rpc.listeners.OnRPCNotificationListener;
 import com.smartdevicelink.proxy.rpc.listeners.OnRPCResponseListener;
 import com.smartdevicelink.util.DebugTool;
@@ -106,6 +107,12 @@ class PresentKeyboardOperation implements Runnable {
 				public void onResponse(int correlationId, RPCResponse response) {
 					finishOperation();
 				}
+
+				@Override
+				public void onError(int correlationId, Result resultCode, String info){
+					DebugTool.logError("There was an error presenting the keyboard. Finishing operation - choice set manager - . Error: " + info);
+					finishOperation();
+				}
 			});
 
 			internalInterface.get().sendRPC(pi);
diff --git a/base/src/main/java/com/smartdevicelink/managers/screen/menu/MenuCell.java b/base/src/main/java/com/smartdevicelink/managers/screen/menu/MenuCell.java
index a772862f8..3ba0ee0b6 100644
--- a/base/src/main/java/com/smartdevicelink/managers/screen/menu/MenuCell.java
+++ b/base/src/main/java/com/smartdevicelink/managers/screen/menu/MenuCell.java
@@ -265,11 +265,11 @@ public class MenuCell implements Cloneable{
 	 */
 	@Override
 	public boolean equals(Object o) {
+		if (o == null) { return false; }
 		// if this is the same memory address, its the same
 		if (this == o) return true;
 		// if this is not an instance of this class, not the same
 		if (!(o instanceof MenuCell)) return false;
-
 		// if we get to this point, create the hashes and compare them
 		return hashCode() == o.hashCode();
 	}
diff --git a/base/src/main/java/com/smartdevicelink/proxy/rpc/Choice.java b/base/src/main/java/com/smartdevicelink/proxy/rpc/Choice.java
index e21f0a77a..8f78e1a20 100644
--- a/base/src/main/java/com/smartdevicelink/proxy/rpc/Choice.java
+++ b/base/src/main/java/com/smartdevicelink/proxy/rpc/Choice.java
@@ -97,6 +97,11 @@ public class Choice extends RPCStruct {
 	public static final String KEY_VR_COMMANDS = "vrCommands";

 	public static final String KEY_CHOICE_ID = "choiceID";

 	public static final String KEY_IMAGE = "image";

+

+    /**

+     * Used to bypass the format() method that adds VR items based on RPC version. This is used by the

+     * choiceSetManager, which has a more in-depth approach as to whether or not it should add VR items

+     */

 	private boolean ignoreAddingVRItems;

 

 	/**

@@ -155,7 +160,7 @@ public class Choice extends RPCStruct {
         if (rpcVersion == null || rpcVersion.getMajor() < 5){

 

             // this is added to allow the choice set manager to disable this functionality

-            if (!getIgnoreAddingVRItems()) {

+            if (!ignoreAddingVRItems) {

                 // make sure there is at least one vr param

                 List<String> existingVrCommands = getVrCommands();

 

@@ -269,8 +274,4 @@ public class Choice extends RPCStruct {
     public void setIgnoreAddingVRItems(boolean ignoreAddingVRItems){

         this.ignoreAddingVRItems = ignoreAddingVRItems;

     }

-

-    private Boolean getIgnoreAddingVRItems(){

-        return this.ignoreAddingVRItems;

-    }

 }