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

Use yabai signals for events #13

Merged
merged 2 commits into from
Aug 4, 2020
Merged

Conversation

alin23
Copy link
Contributor

@alin23 alin23 commented Aug 3, 2020

Hi Adam!

I see you are already working on another branch to use hs.window.filter everywhere, so this PR will mostly serve as a reference if you have already made up your mind about that.

This PR does the following:

  • Use yabai signals along with hs.ipc for checking for changes at the right time
    • this fixes the sleep problem as the changes are always present before yabai sends the signal
    • it also reacts to changes in window/gap/padding size
  • Use the already optimised /bin/dash instead of the brew installed /usr/local/bin/dash. The default /bin/dash starts almost twice as fast as you can see in the below screenshot:
    image
  • Install stackline by directly cloning it into the ~/hammerspoon directory
    • the current instructions have a mistake: require "stackline.stackline.core" expects the whole repo, not separate symlinks
  • Fixes the use of path_helper which outputs executable sh code, not a PATH value
  • Makes showIcons configurable through hs.settings
    • To toggle icons: echo ":toggle_icons:1" | hs -m stackline-config
  • Add some padding to the icons layout
  • Remove some debug messages as everything seems to work perfectly fine and makes the code easier to parse and edit

This is how icons look right now (pills look the same so no need for a screenshot):

CleanShot 2020-08-03 at 16 41 02@2x

CleanShot 2020-08-03 at 16 38 30@2x

This is what one would need to add in their .yabairc for the signals:

STACKLINE_EVENTS="\
    application_activated \
    application_front_switched \
    application_hidden \
    application_launched \
    application_terminated \
    application_visible \
    window_created \
    window_deminimized \
    window_focused \
    window_minimized \
    window_resized"

yabai -m signal --add \
    event="window_destroyed" \
    label="stackline_window_destroyed" \
    action="echo ':window_destroyed' | /usr/local/bin/hs -m stackline-events"

for event in $STACKLINE_EVENTS
do
    yabai -m signal --add \
        event="$event" \
        label="stackline_$event" \
        app!="Hammerspoon" \
        action="echo ':$event' | /usr/local/bin/hs -m stackline-events"
done

fixes #12 #9

@alin23 alin23 changed the title Use yabai signals for events Use yabai signals for events (fixes #12 #9) Aug 3, 2020
@alin23 alin23 changed the title Use yabai signals for events (fixes #12 #9) Use yabai signals for events Aug 3, 2020
@AdamWagner
Copy link
Owner

AdamWagner commented Aug 4, 2020

@alin23 !!

These changes are AWESOME! Thank you!

I just read through all the changes, and they look very very good. I'm going to merge this so anyone who's trying this out has a less bad experience :-)

Some more specific thoughts:

Use yabai signals along with hs.ipc for checking for changes at the right time
this fixes the sleep problem as the changes are always present before yabai sends the signal
it also reacts to changes in window/gap/padding size

Ha! My v0 prototype used signals + hs.ipc. The performance wasn't great, and I also had issues with excessive re-stacking causing flicker. I think this was due to tons of yabai signals being fired and not debounced. The move to Hammerspoon's window filter was an attempt to fix these issues, which caused other issues. The way you've weaved yabai signals back into the mix to patch up those issues is pretty neat.

It doesn't seem many people know about the Hammerspoon ipc tool, so nice find!

Use the already optimised /bin/dash instead of the brew installed /usr/local/bin/dash. The default /bin/dash starts almost twice as fast as you can see in the below screenshot:

Oops! Just a minute while I change all of my hashbangs… haha.
My system is far from stock, and I was worried that I had forgotten a dependency here or there. Thanks for catching/fixing this, and for helping out johnallen3d to get it running.

Install stackline by directly cloning it into the ~/hammerspoon directory
the current instructions have a mistake: require "stackline.stackline.core" expects the whole repo, not separate symlinks

Yea. Not only is it unnecessary to symlink (from ~/Downloads?!), I just plain messed up the instructions in the readme. Big improvement.

Fixes the use of path_helper which outputs executable sh code, not a PATH value

This is the only one that confuses me. 🤨️ I'll followup with a gif of running path_helper and getting a path setting output as a string. Not sure what's going on here. Would be nice to avoid eval if possible.

Makes showIcons configurable through hs.settings
To toggle icons: echo ":toggle_icons:1" | hs -m stackline-config

This is just great! I wanted to create an issue for reading from a settings file yesterday, but forgot. You implemented it before I could do it tonight!

The icons look really really excellent.

I'll test this real quick and then merge. Thanks again!


PS:

But @AdamWagner is already in the middle of a big refactor to ditch the shell script so this might get more stable in the next few days.

Tis true. I have a lot of local changes that I haven't pushed yet, but it's looking promising. MUCH better performance when switching the focused window within a stack. Also, tracking the state of hammerspoon window objects instead of ingesting json and building our own window objects cleans up a lot of stuff. Also, I discovered hs.timer.delayed which, ahem: "Is a specialized timer objects to coalesce processing of unpredictable asynchronous events into a single callback". That's exactly what I had been looking for to fix the excessive re-stacking issue I had when using yabai signals, and still had to some degree when using hammerspoon's window filters.

The major problem I ran into is that the premise of #8 isn't true: There's a key bit of info that only (as far as I can tell) exists in yabai: stack index.

Hammerspoon window filters order windows by last focused by default, which causes the indicators to appear as if they aren't changing when they're actually changing and reordering (I wasted a lot of time trying to solve the wrong problem there).

That said, I actually think most of your changes here will survive (settings, icon beautification, and even some of the yabai signals).

@AdamWagner
Copy link
Owner

Here's a gif of path_helper spitting out the path as a string that could be assigned to PATH. I wonder why we're experiencing different things here?

path_helper_outputs_path

@AdamWagner AdamWagner merged commit 6346cb0 into AdamWagner:master Aug 4, 2020
@AdamWagner
Copy link
Owner

Okay! Everything should be merged in. I added your yabai signals, toggle_icons command, and improved icons screenshot to the readme (I hope it's okay that I used the image).

Can you confirm that the flicker seen in the gif below is also present on your system? Just want to make sure it's not caused by something weird in my setup.

stackline-flicker

@alin23
Copy link
Contributor Author

alin23 commented Aug 4, 2020

@AdamWagner the path_helper man specifically says that it outputs shell commands for evaluating. I can also see that in your gif, you can see the ; export PATH code in the output which clearly does not make sense to be assigned to the PATH variable.
image

Eval is not that wild, a lot of other init scripts use it (ssh-agent, pyenv, gpg-agent etc.) and path_helper is a system binary living in a read-only path which should be as secure as you can have it.
image

About the flicker, it is caused by the window_destroyed signal which does not have an app!= filter to filter out Hammerspoon window events.
I've fixed that here: #14

But of course, this means that closing a window leaves the stackline with a window that does not exist anymore.
Until I can think of a proper solution, I'm working around that by adjusting the yabai gap size which triggers a window_resize event and recreates the stack

@alin23
Copy link
Contributor Author

alin23 commented Aug 4, 2020

Thanks for merging this! I did not expect that, seeing how much work you've put into using Hammespoon's window events.
It's still not perfectly snappy, but I think that's the best we can get given that we're not running C code inside the yabai process. Shells and Lua scripting will always have a small visible delay.

@alin23 alin23 deleted the yabai-signals branch August 4, 2020 08:27
@AdamWagner
Copy link
Owner

the path_helper man specifically says that it outputs shell commands for evaluating

Shame on me for not RTFM ;-) . I think what tripped me up is that PATH=$(/usr/libexec/path_helper) still seems to work for me. I use it often enough that I even have it saved as an Alfred snippet. Without that snippet, scripts that don't inherit the environment fail. With it, they succeed. Nonetheless, it's clearly the wrong way to use it, so thanks for calling this out. TIL.

AdamWagner added a commit that referenced this pull request Aug 10, 2020
AdamWagner added a commit that referenced this pull request Aug 22, 2020
* format: auto-formatted per luaformat config. Functions folded with 'set fdm=marker' in vim

* WIP: Working toward addressing #8 in stackline/query.lua, which replaces bin/yabai-get-stacks.

The goal of this file is to eliminate the need to 'shell out' to yabai to query
window data needed to render stackline, which would address
#8. The main problem with relying
on yabai is that a 0.03s sleep is required in the yabai script to ensure that
the changes that triggered hammerspoon's window event subscriber are, in fact,
represented in the query response from yabai. There are probably secondary
downsides, such as overall performance, and specifically *yabai* performance
(I've noticed that changing focus is slower when lots of yabai queries are
happening simultaneously).

┌────────┐
│ Status │
└────────┘
We're not yet using any of the code in this file to actually render the
indiators or query ata — all of that is still achieved via the "old" methods.
However, this file IS being required by ./core.lua and runs one every window focus
event, and the resulting "stack" data is printed to the hammerspoon console.
The stack data structure differs from that used in ./stack.lua enough that it
won't work as a drop-in replacement. I think that's fine (and it wouldn't be
worth attempting to make this a non-breaking change, esp. since zero people rely
on it as of 2020-08-02.

┌──────┐
│ Next │
└──────┘
- [ ] Integrate appropriate functionality in this file into the Stack module
- [ ] Update key Stack module functions to have basic compatiblity with the new data structure
- [ ] Simplify / refine Stack functions to leverage the benefits of having access to the hs.window module for each tracked window
- [ ] Integrate appropriate functionality in this file into the Core module
- [ ] … see if there's anything left and decide where it should live

┌───────────┐
│ WIP NOTES │
└───────────┘
Much of the functionality in this file should either be integrated into
stack.lua or core.lua — I don't think a new file is needed.
Rather than calling out to the script ../bin/yabai-get-stacks, we're using
hammerspoon's mature (if complicated) hs.window.filter and hs.window modules to
achieve the same goal natively within hammerspon.
There might be other benefits in addition to fixing the problems that inspired
tracked by stackline, which will probably make it easier to implement
enhancements that we haven't even considered yet. This approach should also be
easier to maintain, *and* we get to drop the jq dependency!

* WIP: basic working state with lots of cruft to clean up and edge cases to fix

* Bring over some changes from #13

* Bring over indicator styling improvements from #13

* Move self.colorOpts into Window:drawIndicator() so that the latter can be called in place of Window:process() to redraw a window.

* A small bit of much-needed cleanup

* Basic functionality working well. Parameterized display options. Indicators display on left/right edge of window based on which side of the screen the window resides. Known bug: changes broke multiple stacks on the same space.

* Fixed issue where only 1 of N stacks responded to focus events. Moved all stack-is-occluded functionality from Query to StackMgr and Stack modules.

* Workaround hammerspoon bug (Hammerspoon/hammerspoon#2400) to ensure indicators update when switching between windows of the same app.

* Fold Window:getScreenSide()

* Modifying existing indicators on focus change is MUCH faster than destroying them & rendering from scratch.

* Cleanup (one of the) utils.lua files

* Add a status update to readme describing recent changes

* Included a bunch of cleanup/reorg changes missed in last commit

* Fix path to dash shell (again)

* Add hs._asm.undocumented.spaces dependency

* Remove unused StackConfig:handleSignal() method (it needs to be available outside of class)

* Cleanup comments & delete unused bluclass.lua

* Don't use hs._asm.undocumented.spaces. Uncomment window.filter.notInCurrentSpace which depends on it.'

* Fill gap left behind by hs._asm.undocumented.spaces with hs.spaces.watcher

* Increase threshold in window:getSide() to fix issue w/ large yabai padding values

* Cleanup comments in query.lua

* Removed hs._asm.undocumented.spaces from list of dependencies

* Delete unused utils, consolidate utils into single file.

* Track stack focus state, indicate last-focused window in unfocused stack, and centralize indicator config settings & retrieval.

* Fix misleading comment

* Remove garbage characters in comment

* Fix error stackline/window.lua:95: attempt to perform arithmetic on a nil value (field 'stackIdx') by checking for stackIdx > 0

* Remove completed TODO comments, try to keep TODO comments on single line to work well with leasot / rg

* Enable keybindings in init.lua by returning stackConfig & stackManager instances from main stackline.lua
Remove keybinding from config.lua & move to readme.

* Fix #21 - Limit stack left/right edge to screen boundary so it doesn't go off screen

* Update version to 0.1.50
@AdamWagner AdamWagner linked an issue Aug 26, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants