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

Let libedit handle terminal size changes. #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dag-erling
Copy link

Previously, we would catch SIGWINCH and call el_resize() from the signal handler. This is unsafe and led to strange behavior such as terminating on second resize. The simplest solution is to let libedit handle SIGWINCH itself.

This reverts 56bb182 and 89d6c34 and removes all traces of SIGWINCH from bc itself, and instead sets the EL_SIGNAL flag on the editline context, which causes libedit to detect and handle terminal size changes internally.

Previously, we would catch SIGWINCH and call el_resize() from the signal handler.  This is unsafe and led to strange behavior such as terminating on second resize.  The simplest solution is to let libedit handle SIGWINCH itself.

This reverts 56bb182 and 89d6c34 and removes all traces of SIGWINCH from bc itself, and instead sets the EL_SIGNAL flag on the editline context, which causes libedit to detect and handle terminal size changes internally.
dag-erling referenced this pull request Nov 12, 2024
This was broken by the macOS fix because apparently, editline acts
differently on those platforms.

I have tested that Linux, macOS, and FreeBSD all work with this one,
both SIGWINCH and Ctrl+D. Well, Ctrl+D doesn't work on macOS, but that
is actually a limitation I *can't* get around.

Fixes FreeBSD being broken by 56bb182.

Signed-off-by: Gavin D. Howard <[email protected]>
@gavinhoward
Copy link
Owner

Thank you for your contribution.

However, I cannot accept it. The man page for libedit says that it installs its own signal handler for a set of signals, one of which is SIGINT, which bc needs to treat specially. So I cannot use EL_SIGNAL. If libedit only handled SIGWINCH, this would work.

@dag-erling
Copy link
Author

Did you even test my patch? It works just fine, because (except for SIGWINCH and SIGCONT) libedit restores the previous signal handler and re-posts the signal after cleaning up.

@gavinhoward
Copy link
Owner

I tested it on macOS. It did not work with SIGINT. I do not have access to a Linux machine, but I will later in the day.

But I do not like depending on that behavior. Signals are finicky, and that could expose platform differences.

@dag-erling
Copy link
Author

I developed the patch on macOS, it works perfectly fine there and on FreeBSD. If you have an issue with it, perhaps you can describe the symptoms so I can help you figure it out instead of dismissing it out of hand?

@gavinhoward
Copy link
Owner

Please give me some leniency. Testing it is not dismissing it out of hand. I am wary of making changes that may break things. And I have little time.

However, in this case, I decided to test my code as well and found that it had the same problem: that multiple interrupts would not be handled. I found the fix, and it works for your code.

It make still take me time to test on Linux and FreeBSD, but this patch may still be viable.

@gavinhoward gavinhoward reopened this Nov 19, 2024
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