add alert so braille users can detect events #201328
Conversation
alert for line changesalert so braille users can detect events
| readonly _serviceBrand: undefined; | ||
| notify(event: AccessibleNotificationEvent, userGesture?: boolean): void; | ||
| notify(event: AccessibleNotificationEvent, userGesture?: boolean, forceSound?: boolean, allowManyInParallel?: boolean): void; | ||
| notifyLineChanges(event: AccessibleNotificationEvent[]): void; |
There was a problem hiding this comment.
Why is this LineChanges?
I don't understand what notifyLineChanges([AccessibleNotificationEvent.Save]) would mean. Maybe that needs to be a different enum?
| class FoldedAreaLineFeature implements LineFeature { | ||
| public readonly audioCue = AudioCue.foldedArea; | ||
|
|
||
| public readonly event = AccessibleNotificationEvent.Folded; |
There was a problem hiding this comment.
This is fishy that every LineFeature now has an event and audioCue and that the audioCue can be derived from the event.
| @@ -41,7 +42,8 @@ export class AudioCueLineFeatureContribution | |||
| @IEditorService private readonly editorService: IEditorService, | |||
| @IInstantiationService private readonly instantiationService: IInstantiationService, | |||
| @IAudioCueService private readonly audioCueService: IAudioCueService, | |||
There was a problem hiding this comment.
@IAudioCueService private readonly audioCueService: IAudioCueService,
It is very fishy that this contribution now uses both the audioCueService and the accessibleNotificationService.
And indeed this causes bugs, as the line-feature enablement state is taken from the audio cue service. So if some audio cues are disabled, the corresponding accessibleNotificationEvent will not be sent either.
|
Except the mentioned bug (that line-based features don't send accessible notification events when the corresponding audio cue is disabled), I can see this implementation working. However, from a software-design perspective, I think this feature should be implemented in a different way. Rather, I'd rethink audio cues (which are already an abstract thing, decoupled from the actual sounds). I see two options:
Also, as a thought I just had, with the line features, maybe it even makes sense to distinguish event-based signals/audio-cues (such as the save-signal) from state-based signals/audio-cues (such as the current line has errors/warnings/breakpoints etc). I'd start with 1. and would think about 2. as a separate refactoring. |
fixes #195282
Braille users don't benefit from audio cues, so we should offer
alertas a customizable / additional alternative. I have heard from users that some might use both cues and the alerts.