-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add support for modifying VoiceOver settings #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice abstraction for pressKeysToChangeSetting
- however the name of the method glosses over the detail of it trying twice assuming it's a toggle, and it might've already been in that state. Maybe pressKeysToToggleSetting(sequence, desiredResponse, numTries = 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
if (settings === 'quickNavOn' || settings === 'arrowQuickKeyNavOn') { | ||
await this.pressKeysToChangeSetting( | ||
ATKey.sequence(ATKey.chord(ATKey.key('left'), ATKey.key('right'))), | ||
'quick nav on' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that support.json's instructions text implies a completely different phrase for arrowQuickKeyNavOn
:
"If VoiceOver said 'arrow quick key nav on' ...."
I did confirm it does say "Quick Nav on/off" on MacOS 14.5; this definitely needs additional clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we discuss the support.json
file during today's CG meeting, then we should definitely mention this aspect, too.
Good idea, @gnarf! "toggle" seems like an easy way to communicate the way the method words. I'm not sold on parameterizing the number of tries, though--I have a feeling we ain't gonna need it, and unused defaults are something of a problem in this code base, if you ask me. (If/when we do need to support settings with three or more states, the word "cycle" might be better than "toggle.") |
I've structured this as two commits to ease the review process.
These commands are defined in ARIA-AT. That includes the apparent duplication between
quickNavOn
/quickNavOff
andarrowQuickKeyNavOn
/arrowQuickKeyNavOff
. @howard-e and I intend to verify the intention behind that separately.