handle alerts in audio cue service, add List Alerts command and settings

[meganrogge]

fixes #195282
fixes #201910

[hediet]

Thanks for integrating the AccessibleNotificationService into the AudioCueService!

The PR is quite hard to review though, because it is a refactoring of the AccessibleNotificationService.
Can you try to revert your initial PR where you introduced AccessibleNotificationService and then rebase this PR onto that, so this PR just describes the feature implementation and not the refactoring?
That would make reviewing so much easier and is what I initially suggested. Thank you!

[hediet]

Please revert the introduction of IAccessibleNotificationService in a separate PR and rebase this PR again, so that it only contains the changes for the feature.

Now it is very hard to review the changes needed for the feature.

[meganrogge]

@hediet that would require reverting all 4 of these PRS, which would complicate things quite a bit. I'm happy to walk through the changes with you if you want on a call.

https://github.com/microsoft/vscode/pulls?q=is%3Apr+author%3Ameganrogge+is%3Aclosed+accessible+notification+service

I initially created the AccessibleNotificationService to handle audio cues and alerts in one place and to deal with the complexity introduced by the format/save cues which have setting values of userGesture, always, or never.

Essentially, now I call the AudioCueService instead of the AccessibleNotificationService. I cannot access the WorkingCopyService from within AudioCueService, thus the addition of the SaveAudioCueContribution.

[hediet]

See comments

[hediet]

I initially created the AccessibleNotificationService to handle audio cues and alerts in one place and to deal with the complexity introduced by the format/save cues which have setting values of userGesture, always, or never.

I understand. I do think however that the complexity the AccessibleNotificationService added (having a separate system that is very similar to AudioCues, with parallel settings and events) is higher than the complexity it removed (dealing with userGesture etc). Also, userGesture could be relevant for audio cues as well.

Btw. thank you for all your work on audio cues! :) I really appreciate that you push this feature.
If you like we can schedule a meeting these days to go through the code and discuss how the AudioCue service could evolve to account for the new and future requirements.

[meganrogge]

I understand. I do think however that the complexity the AccessibleNotificationService added (having a separate system that is very similar to AudioCues, with parallel settings and events) is higher than the complexity it removed (dealing with userGesture etc). Also, userGesture could be relevant for audio cues as well.

I agree, which is why I made the change that you suggested :)

If you like we can schedule a meeting these days to go through the code and discuss how the AudioCue service could evolve to account for the new and future requirements.

Sounds good. Do you think we should do this now or as new and future requirements arise?

[hediet]

Sounds good. Do you think we should do this now or as new and future requirements arise?

I'm fine with either, as you have motivation and time!

[meganrogge]

I have other stuff going on this iteration, so maybe in February we should meet.

[meganrogge]

still wip

[hediet]

Now, you could shorten the type to

{ cue: AudioCue; userGesture?: boolean }

or even use a different type (and adjust callers to getValue):

[AudioCue, userGesture?: boolean]

... but that is just icing on the cake.

[meganrogge]

I tried this to no avail 🤔

[hediet]

Thank you, that looks much better now!

By just looking at the code, moving out JSON.stringify from Cache to the use-sides looks like a complication of the code, but it actually reduces complexity, as you need less assumptions to understand Cache.

[meganrogge]

Thanks for all of the reviews 👍🏼