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

Add API for the terminal bell sound #806

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Piturnah
Copy link
Contributor

Adds a command that allows for ringing the terminal bell by printing the bell character.

Technically it's not an ansi code, so it felt slightly strange to put it under Command::write_ansi, but I don't think it makes sense to implement this separately from the Command API.

I think it's somewhat justified if you reinterpret write_ansi as "write to the terminal potentially including ansi codes", but then is_ansi_code_supported doesn't make a lot of sense.

But, this is one edge case so I think it's fairly reasonable. What do you think? I think this functionality - even though it's really as simple as printing a char - has a place in the Crossterm library

This should close #730

@Piturnah Piturnah requested a review from TimonPost as a code owner July 29, 2023 14:09
@TimonPost
Copy link
Member

TimonPost commented Aug 5, 2023

I think its not appropriate to fitt it in the command api. We use command strictly for writing ansi sequences to the writer. For things that do not fit into that structure, we expose custom functions e.g terminal_size, cursor_pos, etc....

@Piturnah Piturnah force-pushed the feat/bell branch 2 times, most recently from ca4e6bc to 7b34f56 Compare August 5, 2023 14:15
@Piturnah
Copy link
Contributor Author

Piturnah commented Aug 5, 2023

Is this API more appropriate? Or would you rather a function like terminal::bell()?

@Piturnah Piturnah changed the title Add command for the terminal bell sound Add API for the terminal bell sound Aug 9, 2023
@TimonPost
Copy link
Member

TimonPost commented May 3, 2024

Sorry for late response. A function is better I think, that works similar to the other api's we have that do not fit the box

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.

Allow for ringing the terminal bell
2 participants