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

Fixes undefined soundService #3314

Merged
merged 1 commit into from
May 4, 2021

Conversation

jeanp413
Copy link
Contributor

@jeanp413 jeanp413 commented May 4, 2021

@Tyriar Tyriar added this to the 4.12.0 milestone May 4, 2021
@Tyriar
Copy link
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
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
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