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

Improve feedback when hook script fails; Add hook script signal handling #314

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

Conversation

mattalxndr
Copy link
Contributor

@mattalxndr mattalxndr commented Dec 15, 2022

I made this change because this feedback would have greatly helped my initial work configuring autorandr.

What is the recommended way to affect/cancel a change using the hook script? Like returning a non-zero exit code from ./preswitch.d/script would cause the switch to not happen.

Thanks

autorandr.py Outdated Show resolved Hide resolved
@phillipberndt
Copy link
Owner

Thanks!

Feel free to invent a return code that does something special :) I so far just went for killing the parent process to stop autorandr whenever I needed that functionality.

@mattalxndr
Copy link
Contributor Author

mattalxndr commented Dec 16, 2022

I added some signal passing from child to parent to cause a halt.

Using the SIGUSR1 signal instead of the untrappable SIGTERM allows a clean shutdown in the parent. It allows some feedback in the logs, and allows exiting with a zero status, which seems more appropriate than being killed, since most of the use cases for this would not include halting the parent process because of an error, necessarily.

Perhaps there are other clean up tasks to be done. Maybe some state that needs to be reverted for a cancelled preswitch call? I am not familiar enough with the codebase to know.

@mattalxndr
Copy link
Contributor Author

mattalxndr commented Dec 16, 2022

Another enhancement for another day: preconfigure hook that allows the user to alter the configuration at runtime.

Use case: I have one main display, and two smaller display, one on either side. The displays on the side are always being physically rotated between portrait and landscape, depending on if I have a terminal up, or I'm watching a movie or something. It would be nice to be able to save my own state somewhere, consult it during a preconfigure hook script call, and change the rotate option on one or both.

I know I could just save a new profile for each combination of these monitors' position, but I already have a profiles to cover one or both of them being off, which is another way I pysically fool with them during the day. So the number of profiles would start to grow exponentially.

EDIT: Moved this to #315

@mattalxndr mattalxndr changed the title Improve feedback when hook script fails Improve feedback when hook script fails; Add hook script signal handling Dec 16, 2022
@phillipberndt
Copy link
Owner

Thanks! I don't quite get the advantage a signal has over a return code? Wouldn't e.g. using an exit code of 42 to indicate that you want autorandr to stop work just as well and be easier to use?

@mattalxndr
Copy link
Contributor Author

mattalxndr commented Apr 18, 2023

Thanks! I don't quite get the advantage a signal has over a return code? Wouldn't e.g. using an exit code of 42 to indicate that you want autorandr to stop work just as well and be easier to use?

My thinking was, an non-zero exit code, even one without a special meaning, means failure. But this is not a failure at all, it's a message.

Linux has a way of sending messages in between processes: Signals. And SIGUSR1 and SIGUSR2 were specifically reserved for application-specific use.

If you understand that and still disagree, I'd be happy to submit a change to this PR that uses exit codes. Anyway, I'll then fix the merge conflict issue.

Let me know.

@phillipberndt
Copy link
Owner

I think I'd still prefer exit codes, because the code on either end is way simpler that way: Autorandr doesn't have to handle signals, the user hook doesn't have to look to the parent PID.

Just don't use 1 or -1 or anything else someone who didn't read the man page would default to to indicate failure, and then I think giving some codes special meaning is just fine.

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