Skip to content

Fixes undefined soundService#3314

Merged
Tyriar merged 1 commit intoxtermjs:masterfrom
jeanp413:fix-undefined-soundService
May 4, 2021
Merged

Fixes undefined soundService#3314
Tyriar merged 1 commit intoxtermjs:masterfrom
jeanp413:fix-undefined-soundService

Conversation

@jeanp413
Copy link
Copy Markdown
Contributor

@jeanp413 jeanp413 commented May 4, 2021

@Tyriar Tyriar added this to the 4.12.0 milestone May 4, 2021
@Tyriar
Copy link
Copy Markdown
Member

Tyriar commented May 4, 2021

Thanks for the fix!

@Tyriar Tyriar merged commit e5c9b0e into xtermjs:master May 4, 2021
@jeanp413 jeanp413 deleted the fix-undefined-soundService branch May 4, 2021 17:42
@jerch
Copy link
Copy Markdown
Member

jerch commented May 5, 2021

@Tyriar I suggest to rethink the way we handle the bell for xterm.js v5. Idea:

  • Strip sound output handling from xterm.js.
  • Propagate onBell as interface to hook into the bell (make sound output an outer env problem). Here integrators can do whatever they want or suits their purpose.
  • Move current direct sound output to a default bell addon, that keeps working in browsers as now, but listens on onBell. If demanded, we could also extend the addon by other bell metaphors, like a visual bell and such.

Issue behind - the current default sound output handling uses an AudioContext internally, which is a rare resource in browser envs (can only be instantiated like 6 times or so). This might lead to weird problems on more complex integrations with multiple terminals. While the SoundService tries to work around that limitation by using a static context instance, it might not work everywhere (e.g. integration already holds all possible instances). The addon separation makes bell handling more flexible, e.g. a complex integration might have its own means to handle onBell, or the addon could be instantiated with an existing audio context instance.

@Tyriar
Copy link
Copy Markdown
Member

Tyriar commented May 5, 2021

@jerch love it, can you make an issue for that?

@jerch jerch mentioned this pull request May 5, 2021
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.

Error Cannot read property 'playBellSound' of undefined after reloading window with hidden terminal

4 participants