Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve usability of signals #167

Closed
dominiklohmann opened this issue Jul 29, 2019 · 10 comments
Closed

Improve usability of signals #167

dominiklohmann opened this issue Jul 29, 2019 · 10 comments
Labels
enhancement New feature or request

Comments

@dominiklohmann
Copy link
Collaborator

Feature request

This is a two-part feature request aiming to greatly improve the usability of signals, and their robustness.

Currently, arguments are just appended to the action of the signal. This leads to some weird situations:

  • If you want to ignore the arguments, your action needs to end in ;: (; to separate commands, : is a no-op that swallows arguments)
  • If you want to use the arguments, but not as the last argument, you need to pass in a filename of an executable file so you can use ${@} / ${1} / ...
  • When arguments change during further development of yabai, scripts using these arguments may break (see window_destroyed signal fires before window is removed #154)

As a solution, I propose not passing the arguments at all by default, but rather relying on environment variables. That way, parameters can be used by their name in the action.

# Current behaviour
/usr/bin/env sh -c '<action> <window id>'

# Desired behaviour
/usr/bin/env wid='<window id>' sh -c '<action>'

The second part is to allow filtering signals using optional app=<regex> and title=<regex> just like rules for events where this makes sense. This filtering is a very common use case, and yabai spawns many child processes unnecessarily without it.

@koekeishiya koekeishiya added the enhancement New feature or request label Jul 29, 2019
@koekeishiya
Copy link
Owner

Signals now pass arguments through environment variables YABAI_SIGNAL_ARG1 and YABAI_SIGNAL_ARG2. Reason for names is to avoid conflicts with other applications or user-created env-vars and also such that yabai itself does not have to care which event-type is currently trying to set the value of a signal argument.

Filtering based on app and title needs some more thought before I'm willing to take that step.

@dominiklohmann
Copy link
Collaborator Author

With the way you implemented this fork_exec should always be run from the main thread, right?

@koekeishiya
Copy link
Owner

koekeishiya commented Jul 29, 2019

That should not matter. The call to setenv happens two forks deep right before we replace the process image using execvp.

@dominiklohmann
Copy link
Collaborator Author

Really boils down to whether signals are processed by a single thread.

  • setenv itself is not thread safe
  • Calls to setenv or unsetenv may happen on one thread right before the call to execvp on another.

@koekeishiya
Copy link
Owner

That's not an issue because the call to setenv happens after the fork, not before, so it will only affect its own instance.

@dominiklohmann
Copy link
Collaborator Author

dominiklohmann commented Jul 29, 2019

I totally missed the call to fork. Nevermind then.

Regarding the naming, I really think that fork_exec should just take another two parameters for env variable names (or a va_list of some sort). That way, the name could be YABAI_WINDOW_ID instead of YABAI_SIGNAL_ARG1 for signals where it's the window id or YABAI_RECENT_SPACE_ID instead of YABAI_SIGNAL_ARG2 for the event=space_changed signal. This makes code easier to write.

@koekeishiya
Copy link
Owner

Regarding the naming, I really think that fork_exec should just take another two parameters for env variable names (or a va_list of some sort). That way, the name could be YABAI_WINDOW_ID instead of YABAI_SIGNAL_ARG1 for signals where it's the window id or YABAI_RECENT_SPACE_ID instead of YABAI_SIGNAL_ARG2 for the event=space_changed signal. This makes code easier to write.

I'm not sold on this. I personally think it is easier to remember generic names for arg1 and arg2, and then if you want your script to be clearer, simply start the script with

current_space=$YABAI_SIGNAL_ARG1
recent_space=$YABAI_SIGNAL_ARG2

instead of having to remember that the environment variable for window events are called x, and for space they are called y, z, etc.

@dominiklohmann
Copy link
Collaborator Author

I can write this from scratch without having to look at the docs:

# expresses intent clearly
yabai -m signal --add label=space_changed_debug event=space_changed \
    action='echo "debug: space $YABAI_RECENT_SPACE_ID -> $YABAI_SPACE_ID"'

# Is this the id or the index? Need to look up in the docs to understand
yabai -m signal --add label=space_changed_debug event=space_changed \
    action='echo "debug: space $YABAI_SIGNAL_ARG1 -> $YABAI_SIGNAL_ARG2"'

Another use case is for scripts that are triggered by multiple actions, where the same env variable can mean totally different things depending on which signal called it. What if you wanted to use the window id if available, but just use the focused window if none was provided?

@koekeishiya
Copy link
Owner

I can write this from scratch without having to look at the docs:

Well, I mean the only reason you can do that is because you are contributing with regards to names, which is fine.

Another use case is for scripts that are triggered by multiple actions, where the same env variable can mean totally different things depending on which signal called it.

I agree that this is a use case that makes having unique names better.

@koekeishiya koekeishiya added the addressed on master; not released Fixed upstream, but not yet released label Jul 30, 2019
koekeishiya added a commit that referenced this issue Jul 30, 2019
@dominiklohmann
Copy link
Collaborator Author

Awesome. Loving it already. Thanks so much @koekeishiya!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants