Mobile Choice Set Manager #1080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #1080 +/- ##
============================================
- Coverage 46.48% 43.9% -2.59%
- Complexity 3852 3996 +144
============================================
Files 440 450 +10
Lines 21052 23633 +2581
Branches 2327 2976 +649
============================================
+ Hits 9787 10375 +588
- Misses 10718 12652 +1934
- Partials 547 606 +59
Continue to review full report at Codecov.
|
# Conflicts: # base/src/main/java/com/smartdevicelink/managers/screen/BaseScreenManager.java
… feature/choice_set_manager
bilal-alsharifi
commented
Jun 14, 2019
| @@ -61,7 +59,6 @@ public void testInstantiation(){ | |||
| assertNull(screenManager.getSoftButtonObjectByName("test")); | |||
| assertNull(screenManager.getSoftButtonObjectById(1)); | |||
| assertEquals(screenManager.getDynamicMenuUpdatesMode(), DynamicMenuUpdatesMode.ON_WITH_COMPAT_MODE); | |||
There was a problem hiding this comment.
Shouldn't be able to add this back?
joeygrover
reviewed
Jun 14, 2019
| // make sure there is at least one vr param | ||
| List<String> existingVrCommands = getVrCommands(); | ||
| // this is added to allow the choice set manager to disable this functionality | ||
| if (!getIgnoreAddingVRItems()) { |
There was a problem hiding this comment.
This can be a direct access to the variable as it's private and in the class. Unless you moved this to a static instance for all choice instances and then synchronized the read/write operations.
joeygrover
reviewed
Jun 14, 2019
| @@ -97,6 +97,7 @@ | |||
| public static final String KEY_VR_COMMANDS = "vrCommands"; | |||
| public static final String KEY_CHOICE_ID = "choiceID"; | |||
| public static final String KEY_IMAGE = "image"; | |||
| private boolean ignoreAddingVRItems; | |||
There was a problem hiding this comment.
I would add a description to this variable so that it's clear why we have it.
joeygrover
reviewed
Jun 14, 2019
| int nextChoiceId; | ||
| final int choiceCellIdMin = 1; | ||
|
|
||
| SystemContext currentSystemContext; |
joeygrover
reviewed
Jun 14, 2019
| executor.submit(deleteChoicesOperation); | ||
| } | ||
|
|
||
| public void presentChoiceSet(@NonNull final ChoiceSet choiceSet, @Nullable final InteractionMode mode, @Nullable final KeyboardListener keyboardListener){ |
joeygrover
reviewed
Jun 14, 2019
| } | ||
| } | ||
|
|
||
| public void deleteChoices(@NonNull List<ChoiceCell> choices){ |
joeygrover
reviewed
Jun 14, 2019
| executor.submit(checkChoiceVR); | ||
| } | ||
|
|
||
| public void preloadChoices(@NonNull List<ChoiceCell> choices, @Nullable final CompletionListener listener){ |
joeygrover
reviewed
Jun 14, 2019
| pendingPresentOperation = executor.submit(presentOp); | ||
| } | ||
|
|
||
| public void presentKeyboard(@NonNull String initialText, @Nullable KeyboardProperties customKeyboardConfig, @NonNull KeyboardListener listener){ |
joeygrover
reviewed
Jun 14, 2019
| // if this is not an instance of this class, not the same | ||
| if (!(o instanceof ChoiceCell)) return false; | ||
|
|
||
| return hashCode() == o.hashCode(); |
There was a problem hiding this comment.
'o' could be null here and that will throw an NPE
joeygrover
reviewed
Jun 14, 2019
| pi.setOnRPCResponseListener(new OnRPCResponseListener() { | ||
| @Override | ||
| public void onResponse(int correlationId, RPCResponse response) { | ||
| finishOperation(); |
There was a problem hiding this comment.
This didn't check the onError method, meaning if there is an error this will never call finishOperation
joeygrover
reviewed
Jun 14, 2019
| DeleteInteractionChoiceSet delete = createDeleteInteractionChoiceSet(); | ||
| delete.setOnRPCResponseListener(new OnRPCResponseListener() { | ||
| @Override | ||
| public void onResponse(int correlationId, RPCResponse response) { |
BrettyWhite
approved these changes
Jun 14, 2019
BrettyWhite
approved these changes
Jun 14, 2019
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #764
This PR is ready for review.
Risk
This PR makes minor API changes.
Testing Plan
To be added later
Summary
This PR implements the
ChoiceSetManagerfor Android, JavaSE, and JavaEE projects as described in the proposalTasks Remaining:
CLA