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

check each line of messages for the correct mode on mode switch #48

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

gnarf
Copy link
Contributor

@gnarf gnarf commented Mar 28, 2024

Have seen errors while trying to switch modes on firefox. Occasionally it will end up reading out extra information after switching modes, for example "Focus Mode\n Pizza Crust\n Grouping"

Updated the mode switch to check each line of the output individually for the correct mode text.

@gnarf gnarf requested a review from jugglinmike March 28, 2024 17:11
Comment on lines 122 to 124
// try to keep this short because it runs up to twice for every test
// have seen occasional timeout/misses at 500ms
const MODE_SWITCH_SPEECH_TIMEOUT = 750;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for a quick fix, but in general, a variable should be controlled from the context upon which it depends. What that means here is this value should be specified command-line argument. @ccanash Can you create a Task/Issue to track that change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 122 to 123
// try to keep this short because it runs up to twice for every test
// have seen occasional timeout/misses at 500ms
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little vague (and colloquial besides). Have you observed delays longer than 500ms in the GitHub Actions environment? During local testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Local test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a vm

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I've clarified those comments.

@jugglinmike jugglinmike merged commit dc0d405 into w3c:main Apr 1, 2024
4 checks passed
@jugglinmike jugglinmike deleted the fix-extra-text-mode-switch branch April 1, 2024 16:55
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