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
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/js/src/manager/screen/choiceset/ChoiceCell.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class ChoiceCell {
this._text = null;
this._secondaryText = null;
this._tertiaryText = null;
this._uniqueText = null;
this._voiceCommands = null;
this._artwork = null;
this._secondaryArtwork = null;
Expand All @@ -51,6 +52,7 @@ class ChoiceCell {
}

this._text = text;
this._uniqueText = text;
}

/**
Expand Down Expand Up @@ -112,6 +114,29 @@ class ChoiceCell {
return this;
}

/**
* Get the state unique text. USED INTERNALLY
* @private
* @returns {String} - The uniqueText to be used in place of primaryText when core does not support identical names for ChoiceSets
*/
_getUniqueText () {
return this._uniqueText;
}

/**
* Primary text of the cell to be displayed on the module. Used to distinguish cells with the
* same `text` but other fields are different. This is autogenerated by the screen manager.
* Attempting to use cells that are exactly the same (all text and artwork fields are the same)
* will not cause this to be used. This will not be used when connected to modules supporting RPC 7.1+.
* @private
* @param {String} uniqueText - The uniqueText to be used in place of primaryText when core does not support identical names for ChoiceSets
* @returns {ChoiceCell} - A reference to this instance to support method chaining
*/
_setUniqueText (uniqueText) {
this._uniqueText = uniqueText;
return this;
}

/**
* Get the state voiceCommands
* @returns {String[]|null} - The list of voice command strings
Expand Down
42 changes: 35 additions & 7 deletions lib/js/src/manager/screen/choiceset/_ChoiceSetManagerBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,21 @@ class _ChoiceSetManagerBase extends _SubManagerBase {
* @param {ChoiceCell[]} choices - A list of ChoiceCell objects that will be part of a choice set later
* @returns {Promise} - A promise that resolves to a Boolean of whether the operation is a success
*/
async preloadChoices (choices) {
async preloadChoices (choices = null) {
if (this._getState() === _SubManagerBase.ERROR) {
console.warn('ChoiceSetManager: Choice Manager In Error State');
return false;
}

// 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.
if (choices !== null && this._lifecycleManager.getSdlMsgVersion() !== null
&& (this._lifecycleManager.getSdlMsgVersion().getMajorVersion() < 7
|| (this._lifecycleManager.getSdlMsgVersion().getMajorVersion() === 7 && this._lifecycleManager.getSdlMsgVersion().getMinorVersion() === 0))) {
// version if 7.0.0 or lower
this._addUniqueNamesToCells(choices);
}
const choicesToUpload = choices.map(choice => choice); // shallow copy

this._removeChoicesFromChoices(this._preloadedChoices, choicesToUpload);
this._removeChoicesFromChoices(this._pendingPreloadChoices, choicesToUpload);

Expand Down Expand Up @@ -167,6 +176,25 @@ class _ChoiceSetManagerBase extends _SubManagerBase {
});
}

/**
* Checks if 2 or more cells have the same text/title. In case this condition is true, this function will handle the presented issue by adding "(count)".
* 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)".
* @param {ChoiceCell[]} choices - A list of ChoiceCell objects to be uploaded
*/
_addUniqueNamesToCells (choices) {
const dictCounter = {}; // create a string to number hash for counting similar primary texts

choices.forEach(choice => {
const choiceName = choice.getText();
if (dictCounter[choiceName] === undefined) {
dictCounter[choiceName] = 1; // unique text
} else { // found a duplicate
dictCounter[choiceName] += 1;
choice._setUniqueText(`${choiceName} (${dictCounter[choiceName]})`);
}
});
}

/**
* Deletes choices that were sent previously
* @param {ChoiceCell[]} choices - A list of ChoiceCell objects
Expand Down Expand Up @@ -276,15 +304,16 @@ class _ChoiceSetManagerBase extends _SubManagerBase {
}
}

const uniqueChoiceTexts = {};
const uniqueChoiceCells = [];
const uniqueVoiceCommands = {};
let choiceCellWithVoiceCommandCount = 0;
let allVoiceCommandsCount = 0;

for (let index = 0; index < choices.length; index++) {
const choiceText = choices[index].getText();
const choiceVoiceCommands = choices[index].getVoiceCommands();
uniqueChoiceTexts[choiceText] = true;
if (uniqueChoiceCells.findIndex(choice => choice.equals(choices[index])) === -1) {
uniqueChoiceCells.push(choices[index]);
}

if (choiceVoiceCommands !== null) {
choiceCellWithVoiceCommandCount++;
Expand All @@ -297,9 +326,8 @@ class _ChoiceSetManagerBase extends _SubManagerBase {
}
}

// Cell text MUST be unique
if (Object.keys(uniqueChoiceTexts).length < choices.length) {
console.error('ChoiceSetManager: Attempted to create a choice set with duplicate cell text. Cell text must be unique. The choice set will not be set.');
if (uniqueChoiceCells.length !== choices.length) {
console.error('Attempted to create a choice set with a duplicate cell. Cell must have a unique value other than its primary text. The choice set will not be set.');
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ class _PreloadChoicesOperation extends _Task {
vrCommands = this._isVrOptional ? null : [`${cell._getChoiceId()}`]; // stringified choice id
}

const menuName = this._shouldSendChoiceText() ? cell.getText() : null;
const menuName = this._shouldSendChoiceText() ? cell._getUniqueText() : null;
if (menuName === null) {
console.error('PreloadChoicesOperation: Could not convert Choice Cell to CreateInteractionChoiceSet. It will not be shown');
return null;
Expand Down Expand Up @@ -247,4 +247,4 @@ class _PreloadChoicesOperation extends _Task {
}
}

export { _PreloadChoicesOperation };
export { _PreloadChoicesOperation };
12 changes: 11 additions & 1 deletion tests/managers/screen/choiceset/ChoiceCellTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ module.exports = function (appClient) {
Validator.assertEquals(choiceCell.getSecondaryArtwork(), artwork);
Validator.assertEquals(choiceCell._getChoiceId(), MAX_ID);

Validator.assertEquals(choiceCell._getUniqueText(), choiceCell.getText());

choiceCell._setUniqueText('hi');
Validator.assertEquals(choiceCell._getUniqueText(), 'hi');

choiceCell.setText('hello');
Validator.assertEquals(choiceCell.getText(), 'hello');
});
Expand All @@ -62,8 +67,13 @@ module.exports = function (appClient) {
.setSecondaryText(Test.GENERAL_STRING)
.setTertiaryText(Test.GENERAL_STRING);

// UniqueText should not be taken into consideration when checking equality
choiceCell._setUniqueText('1');
choiceCell2._setUniqueText('2');
choiceCell3._setUniqueText('3');

Validator.assertTrue(choiceCell.equals(choiceCell2));
Validator.assertTrue(!choiceCell.equals(choiceCell3));
});
});
};
};
65 changes: 53 additions & 12 deletions tests/managers/screen/choiceset/ChoiceSetManagerTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ module.exports = function (appClient) {
});

it('testSetupChoiceSet', function () {
const stub = sinon.stub(sdlManager._lifecycleManager, 'getSdlMsgVersion')
.returns(new SDL.rpc.structs.SdlMsgVersion()
.setMajorVersion(7)
.setMinorVersion(0)
.setPatchVersion(0));

const choiceSetSelectionListener = new SDL.manager.screen.choiceset.ChoiceSetSelectionListener()
.setOnChoiceSelected(() => {})
.setOnError(() => {});
Expand All @@ -57,34 +63,45 @@ module.exports = function (appClient) {
const choiceSet1 = new SDL.manager.screen.choiceset.ChoiceSet('test', [], choiceSetSelectionListener);
Validator.assertTrue(!csm._setUpChoiceSet(choiceSet1));

// cells cant have duplicate text
// Identical cells will not be allowed
const cell1 = new SDL.manager.screen.choiceset.ChoiceCell('test');
const cell2 = new SDL.manager.screen.choiceset.ChoiceCell('test');
const choiceSet2 = new SDL.manager.screen.choiceset.ChoiceSet('test', [cell1, cell2], choiceSetSelectionListener);
Validator.assertTrue(!csm._setUpChoiceSet(choiceSet2));

// cells cannot mix and match VR / non-VR
// cells that have duplicate text will be allowed if there is another property to make them unique
// because a unique name will be assigned and used
const cell3 = new SDL.manager.screen.choiceset.ChoiceCell('test')
.setVoiceCommands(['Test']);
const cell4 = new SDL.manager.screen.choiceset.ChoiceCell('test2');
.setSecondaryText('text 1');
const cell4 = new SDL.manager.screen.choiceset.ChoiceCell('test')
.setSecondaryText('text 2');
const choiceSet3 = new SDL.manager.screen.choiceset.ChoiceSet('test', [cell3, cell4], choiceSetSelectionListener);
Validator.assertTrue(!csm._setUpChoiceSet(choiceSet3));
Validator.assertTrue(csm._setUpChoiceSet(choiceSet3));

// VR Commands must be unique
// cells cannot mix and match VR / non-VR
const cell5 = new SDL.manager.screen.choiceset.ChoiceCell('test')
.setVoiceCommands(['Test']);
const cell6 = new SDL.manager.screen.choiceset.ChoiceCell('test2')
.setVoiceCommands(['Test']);
const cell6 = new SDL.manager.screen.choiceset.ChoiceCell('test2');
const choiceSet4 = new SDL.manager.screen.choiceset.ChoiceSet('test', [cell5, cell6], choiceSetSelectionListener);
Validator.assertTrue(!csm._setUpChoiceSet(choiceSet4));

// Passing Case
// VR Commands must be unique
const cell7 = new SDL.manager.screen.choiceset.ChoiceCell('test')
.setVoiceCommands(['Test']);
const cell8 = new SDL.manager.screen.choiceset.ChoiceCell('test2')
.setVoiceCommands(['Test2']);
.setVoiceCommands(['Test']);
const choiceSet5 = new SDL.manager.screen.choiceset.ChoiceSet('test', [cell7, cell8], choiceSetSelectionListener);
Validator.assertTrue(csm._setUpChoiceSet(choiceSet5));
Validator.assertTrue(!csm._setUpChoiceSet(choiceSet5));

// Passing Case
const cell9 = new SDL.manager.screen.choiceset.ChoiceCell('test')
.setVoiceCommands(['Test']);
const cell10 = new SDL.manager.screen.choiceset.ChoiceCell('test2')
.setVoiceCommands(['Test2']);
const choiceSet6 = new SDL.manager.screen.choiceset.ChoiceSet('test', [cell9, cell10], choiceSetSelectionListener);
Validator.assertTrue(csm._setUpChoiceSet(choiceSet6));

stub.restore();
});

it('testUpdateIdsOnChoices', function () {
Expand Down Expand Up @@ -173,5 +190,29 @@ module.exports = function (appClient) {
stub.restore();
stub2.restore();
});

it('testAddUniqueNamesToCells', function () {
crokita marked this conversation as resolved.
Show resolved Hide resolved
const cell1 = new SDL.manager.screen.choiceset.ChoiceCell('McDonalds')
.setSecondaryText('1 mile away');
const cell2 = new SDL.manager.screen.choiceset.ChoiceCell('McDonalds')
.setSecondaryText('2 miles away');
const cell3 = new SDL.manager.screen.choiceset.ChoiceCell('Starbucks')
.setSecondaryText('3 miles away');
const cell4 = new SDL.manager.screen.choiceset.ChoiceCell('McDonalds')
.setSecondaryText('4 miles away');
const cell5 = new SDL.manager.screen.choiceset.ChoiceCell('Starbucks')
.setSecondaryText('5 miles away');
const cell6 = new SDL.manager.screen.choiceset.ChoiceCell('Meijer')
.setSecondaryText('6 miles away');

csm._addUniqueNamesToCells([cell1, cell2, cell3, cell4, cell5, cell6]);

Validator.assertEquals(cell1._getUniqueText(), 'McDonalds');
Validator.assertEquals(cell2._getUniqueText(), 'McDonalds (2)');
Validator.assertEquals(cell3._getUniqueText(), 'Starbucks');
Validator.assertEquals(cell4._getUniqueText(), 'McDonalds (3)');
Validator.assertEquals(cell5._getUniqueText(), 'Starbucks (2)');
Validator.assertEquals(cell6._getUniqueText(), 'Meijer');
});
});
};
};