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

Update preset-announcement.js to skip restore of idle players #891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

crissmil
Copy link
Contributor

When using presets all players get backup/restore. This affects a playing player by restoring it to the time of backup state.elapsedTime, even if this player is not included in the preset file and is supposed to be skipped. Even if pauseOthers is set to false, by this behavior the player still get interrupted one sec because it is shifted back to recorded elapsedTime.

This PR is changing "saveAll" to "savePresetPlayers" and will backup/restore only players specified in preset file. Idle players are no more affected by running presets.

Tested and found more stable when joining/leaving groups after tts or chime clips.

When using presets all players get backup/restore. This affects a playing player by restoring it to the time of backup state.elapsedTime, even if this player is not included in the preset file and is supposed to be skipped. Even if pauseOthers is set to false, by this behavior the player still get interrupted one sec because it is shifted back to recorded elapsedTime.

This PR is changing "saveAll" to "savePresetPlayers" and will backup/restore only players specified in preset file. Idle players are no more affected by running presets.

Tested and found more stable when joining/leaving groups after tts or chime clips.
Copy link
Owner

@jishi jishi left a comment

Choose a reason for hiding this comment

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

Sorry about the late review, I hadn't really seen this one and it seems like it fixes something broken.

Please use 2 space indentation to match the rest of the codebase, and not tabs, otherwise the diffs looks weird.

delete backupPresets[key];
}
});

Copy link
Owner

Choose a reason for hiding this comment

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

instead of mutating the array to remove "empty", you can add a .filter(Boolean) after the map() when creating backupPresets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I don't know how

preset.elapsedTime = state.elapsedTime;
}
for (var index = 0; index < pset.players.length; ++index) {
if (pset.players[index].roomName == coordinator.roomName) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not really following the logic here, why would you create a preset for each player in a zone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is to completely ignore other players from my house. When my daughter was listening spotify in her room, if I had a preset announcement in any other room(s) , at the moment of tts ending, her player track got rewind at the moment of preset backup. Her player was not supposed to be restored at all.
I couldn't find a way to restore only specific player from an allSave preset, so I decided to create separate presets then restore all available.

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.

2 participants