Skip to content
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.

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

Feature/0180 choice uniqueness #382

Merged
merged 5 commits into from
Mar 4, 2021
Merged

Conversation

crokita
Copy link
Contributor

@crokita crokita commented Jan 27, 2021

Fixes #345

NOTE: This PR relies on the choice set manager, so this PR should be merged first: #371

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).

Unit Tests

Updated tests to account for text uniqueness changes, as well as adding a new test for the new choicesetmanager method.

Core Tests

Tested against current version of Manticore (core 7.0.0)

Summary

Allows for text duplication in the choice set manager, and has a fallback method for deduplication in older versions of SDL Core.

Copy link
Contributor

@renonick87 renonick87 left a comment

Choose a reason for hiding this comment

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

Will want to pull in the develop branch to fix the issues showing up from the linter.
The VideoStreaming PR is also present in this branch. It's fine if that gets merged first, but if not then it'll need to be reverted and squashed on this PR.
This may be unrelated to this PR but I've tried presenting a ChoiceSet with this code snippet

const choice1 = new ChoiceCell('cell')._setChoiceId(1);
const choice2 = new ChoiceCell('cell')._setChoiceId(2);
const choice3 = new ChoiceCell('cell')._setChoiceId(3);
const listener = new SDL.manager.screen.choiceset.ChoiceSetSelectionListener()
    .setOnError((error) => {
        console.log(`error: ${error}`);
    })
    .setOnChoiceSelected((choiceCell, triggerSource, rowIndex) => {
        console.log(`choiceCell: ${choiceCell}, triggerSource: ${triggerSource}, rowIndex: ${rowIndex}`);
    });
const choiceset = new ChoiceSet('choices', [choice1, choice2, choice3], listener);
screenManager.presentChoiceSet(choiceset, SDL.rpc.enums.InteractionMode.MANUAL_ONLY);

I get an error that says INVALID_ID. This happens regardless of whether I manually set the choiceID. I checked the PerformInteraction request and it looks like the ChoiceCells have all had their IDs set to 1 which could be what is causing the request to fail.

@crokita crokita force-pushed the feature/0180-choice-uniqueness branch from 1bd846b to 12e783c Compare February 9, 2021 16:44
@crokita
Copy link
Contributor Author

crokita commented Feb 9, 2021

I have recreated the branch to exclude those other PR commits, accounting for the squashed changes of the newly merged choice set manager as well.

The reason for the choice set failing in the snippet above is because of the choice cells with the same names. The choice set manager will consider them as duplicate cell names and will be unable to use them to send a perform interaction to core (this will happen when testing on Manticore).

For this snippet to work you will need the build of core with the broaden choice uniqueness features so that duplicate primary texts are allowed

@crokita
Copy link
Contributor Author

crokita commented Feb 9, 2021

I see what you mean by the issue you found. Looks like the issue should be caught before the point where the RPC is attempted to be sent. See Joel's review in this PR for the ChoiceSet class: smartdevicelink/sdl_ios#1886

@crokita
Copy link
Contributor Author

crokita commented Feb 11, 2021

Updated to have better behavior when sending exact duplicate choice cells, which should cause an error regardless of version used.

@crokita crokita merged commit 30dd1f2 into develop Mar 4, 2021
@crokita crokita deleted the feature/0180-choice-uniqueness branch March 26, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SDL 0180] Broaden Choice Uniqueness
2 participants