Skip to content

Conversation

rolesen
Copy link
Contributor

@rolesen rolesen commented Jan 24, 2021

…rrupt signal (usually 'ctrl-c' - also enabled as default) to be able to clean up the terminal on user interrupt. Consider if kill signal should also be handled. This is implemented via a monitoring thread that then calls std::exit, as it is very limited what is allowed to do be done in a signal handler. I am proposing this as part of the library so users of the library won't have to bother with these issues.

…rrupt signal (usually 'ctrl-c' - also enabled as default) to be able to clean up the terminal on user interrupt. Consider if kill signal should also be handled. This is implemented via a monitoring thread that then calls std::exit, as it is very limited what is allowed to do be done in a signal handler. I am proposing this as part of the library so users of the library won't have to bother with these issues.
@rolesen
Copy link
Contributor Author

rolesen commented Jan 31, 2021

Please take a look and let me know where this stands ? I am happy to whatever or answer any questions you may have. If you do not think these features should belongs in the library also, NP, just let us know by rejecting the PR :) (it was easier to try and get this into the library)

~InitAlternateBuffer()
{
// Exit the alternate buffer
std::cout << "\x1b?1049l";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this supposed to be "\x1b[?1049l" ?

@nikhedonia
Copy link
Contributor

Thanks for your time and patience.
I don't think the signal handler and usage of threads (why is the thread needed at all?) is appropiate as it may make this library unsuitable in some usecases. For instance the user may have installed a signal handler that implements custom behaviour and VirtualTerminals signal handler might break the expected behaviour.

I think the VirtualConsoleMode is a great addendum and allows the implementation of new (T)UX patters!
How about we just merge that bit?

@rolesen
Copy link
Contributor Author

rolesen commented Feb 1, 2021

Thank you for the quick responce. I do fully understand why this suggestion is borderline what the library should be handling. It now all gets a bit complicated now. Yes, we could just include the alternate buffer stuff BUT it relies upon a clean exit to get those global destructors called to exit the alternate buffer. So, it is true that most advanced terminal programs handles some signals themselves, but it is probably lost on the average user that just want to make something simpel.

You have to use a thread if you want to keep it within c++ standard (which is simpler crossplatform wise) - ie. you cannot call exit / std::exit from the signal itself. The problem is this is all very complicated to figure out for the average user of the library and so leaves the terminal in a mess (in my example unusable).

Alternatively one could get rid of just the thread and use at_quick_exit + at_exit and call std::quick_exit in the signal handler.
One annoying drawback is that then any normal cleanup code, eg. at_exit and destructors the user then has isn't called and then he have to create her own signal handler herself anyway.

I did use bools in the constructor for configuration (if signal handler not wanted we are just assuming the user has one that exits cleanly) but we could make it more explicit by useing typing/policy based design and/or not having default arguments to make it more clear and show in code that we install a signal handler. Alternatively reverse the defaults (so nothing is setup by default).

The bottom line is that if you are just writing a small program or you are a learning user - then you do not expect that the terminal is in a mess from using the library and/or it becomes noisy in your own code to include signal handlers, etc. - but i dunno :)

I really don't know what is best here cause of action, but i tried to explain how i think about it - awaiting your advice. (and again it is fine to just reject - i do not mind at all and completely understands if you don't want this 'noise' into the library)

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.

2 participants