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

use audio cue to signal no inlay hints can be read #143539

Merged
merged 2 commits into from Feb 21, 2022
Merged

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Feb 21, 2022

This PR extends #142532 by playing an audio cue whenever users ask to read the current line with inlay hint but when there are none.

@jrieken jrieken self-assigned this Feb 21, 2022
@jrieken
Copy link
Member Author

jrieken commented Feb 21, 2022

fyi @hediet and @isidorn - let me know what you think about this audio cue usage and if that's something that we want.

The change is a bit complex because audio cues are workbench only. Therefore I had to extract some inlay hint bits into workbench as well. The gist is

  • a new cue for no inlay hints (uses error source)
  • play that cue when asking for inlay hints when there are none

Copy link
Member

@hediet hediet left a comment

Choose a reason for hiding this comment

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

Nice ;) Left some feedback.

@jrieken jrieken merged commit a00e9d1 into main Feb 21, 2022
@jrieken jrieken deleted the joh/inlayAudioCue branch February 21, 2022 13:32
public static readonly noInlayHints = AudioCue.register({
name: localize('audioClues.noInlayHints', 'No Inlay Hints available for the current line'),
sound: Sound.error,
settingsKey: 'audioClues.noInlayHints'
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not audioClues it is audioCues :)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in main

@isidorn
Copy link
Contributor

isidorn commented Feb 21, 2022

Cool work!
I think this should follow the pattern that we do for all other features. If the audioCue setting is "on" that means the sound is played when the cursor is on a line with inlay hints. So a user can trigger an additional command to learn more.
I do not think we need a sound for "no inlay hints". Silence will carry that meaning.

@isidorn
Copy link
Contributor

isidorn commented Feb 21, 2022

I tried it out and some more feedback:

  1. "Read line with inlay hints" -> I think we should rename this command to be be more general and that we can potentially use it for all editor richness in the future. I am not sure what is the right name, but I will think about it
  2. I will ping the screen reader users so they try it out so we get immediate feedback. @jrieken do you know if inlay hints are available in Java, Python, C++ just so I know how to tell users to try it out
  3. I am a bit worried about only esc and tab going out of this mode. Ideally any key or at least left/right/up/down would take you out
  4. I do not think there is a need for "stop reading" to be in the command palette

All in all, cool work and I think we should just re-think some things so we can build more on top of this :)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants