Skip to content

Conversation

@renonick87
Copy link
Contributor

Fixes #433

Risk

This PR makes minor API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have verified that this PR passes lint validation
  • 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

Added unit tests to verify that VoiceCommands are added/deleted correctly.

Changelog

Bug Fixes
  • The VoiceCommandManager will no longer delete a VoiceCommand if the VoiceCommand is going to be immediately added again as part of the same update operation. Below is sample code that can be used to test the functionality. Option 1-3 will be added as part of the first operation. The second operation will delete Option 3 and add Option 4 while leaving Option 1/2 untouched.
const voiceCommands = [
    new SDL.manager.screen.utils.VoiceCommand(['Option 1'], () => {}),
    new SDL.manager.screen.utils.VoiceCommand(['Option 2'], () => {}),
    new SDL.manager.screen.utils.VoiceCommand(['Option 3'], () => {}),
];

await screenManager.setVoiceCommands(voiceCommands);
const newVoiceCommands = [
    voiceCommands[0],
    voiceCommands[1],
    new SDL.manager.screen.utils.VoiceCommand(['Option 4'], () => {}),
]
await screenManager.setVoiceCommands(newVoiceCommands);

CLA

Copy link
Contributor

@crokita crokita left a comment

Choose a reason for hiding this comment

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

See minor feedback

@renonick87
Copy link
Contributor Author

renonick87 commented May 27, 2021

Updates made since last review:

  • Equality check on voice commands is not done at the start.
  • Added a clone method to the VoiceCommand class and unit tests to cover it.
  • The manager's _voiceCommands is set to null if the commands are invalid.
  • Updated the docs description for getVoiceCommands since it says that it returns the currently set voice commands but does not actually return _currentVoiceCommands.
  • Reduced the logic in _voiceCommandsNotInSecondArray to be much cleaner.
  • Fixed the case where updating a Voice Command's listener and calling setVoiceCommands would not update the listener.
  • Updated the VoiceCommand equals method to not check the commandId because otherwise it would never return true for commands that had been presented and had their IDs updated.

Copy link
Contributor

@crokita crokita left a comment

Choose a reason for hiding this comment

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

See feedback

* @param {VoiceCommand[]} secondArray - an array of VoiceCommands
* @returns {VoiceCommand[]} - An array of VoiceCommands that are in the first array but not the second array
*/
_voiceCommandsNotInSecondArray (firstArray, secondArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could fail if either oldVoiceCommands or pendingVoiceCommands is null. Recommend null checking them

Copy link
Contributor

@crokita crokita left a comment

Choose a reason for hiding this comment

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

See feedback

@renonick87 renonick87 merged commit 92a0f3b into develop Jun 2, 2021
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.

Avoid deleting and setting identical voice commands

3 participants