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

add sketchybar docs #81

Closed

Conversation

MatthiasGrandl
Copy link
Contributor

@MatthiasGrandl MatthiasGrandl commented Jan 1, 2024

Disclaimer: I am not super familiar/comfortable with Asciidoc.

This is a sample integration for Sketchybar. It should be equivalent to the default Sketchybar Mission Control module.

Depends on #80 to work.

@MatthiasGrandl
Copy link
Contributor Author

I’ll update this PR tomorrow based on the changes to #80 .

# ~/.config/sketchybar/sketchybarrc

# Add AeroSpace workspace event
sketchybar --add event aerospace_change bobko.aerospace.WorkspaceFocus
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be added directly when you create a space. See my example below

sketchybar --add event aerospace_change bobko.aerospace.WorkspaceFocus

# Add Workspaces
for sid in $(aerospace list-workspaces); do
Copy link
Contributor

Choose a reason for hiding this comment

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

You could extend this to show how to deal with multiple screens. Here is my config:

#!/bin/bash

SPACE_ICONS=("1" "2" "3" "4" "5" "6" "7" "8" "9" "10" "11" "12" "13" "14" "15")

sid=0
monitors_count=$(aerospace list-monitors | wc -l)
workspaces=$(aerospace list-workspaces)

for sid in $workspaces; do
  if [ "$sid" -lt 5 ] || [ "$monitors_count" -eq 1 ]; then
    display=1
  else
    display=2
  fi

  padding_right=0
  if [ "$sid" -eq 8 ] || [ "$sid" -eq 4 ]; then
    padding_right=8
  fi

  sketchybar --add item space.$sid left \
    --add event aerospace_change bobko.aerospace.focusedWorkspaceChanged \
    --subscribe space.$sid aerospace_change \
    --set space.$sid \
    associated_display=$display \
    icon=${SPACE_ICONS[$((sid-1))]} \
    icon.padding_left=16 \
    icon.padding_right=$padding_right \
    icon.highlight_color=$DRACULA_GREEN \
    click_script="aerospace workspace $sid" \
    script="$PLUGIN_DIR/aerospace.sh $sid"

  sketchybar --trigger aerospace_change
done

aerospace.sh:

#!/bin/bash

SID=$1
visible_workspaces=$(aerospace list-workspaces --visible)
visible=$(echo "$visible_workspaces" | grep -q "$SID" && echo "on" || echo "off")

sketchybar --set $NAME icon.highlight=$visible label.highlight=$visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intentionally keeping the config as simple as possible and adapted the default sketchybar config. I think such elaborate configs are better suited to the sketchybar showcase github discussion as to not bloat this document.

@MatthiasGrandl
Copy link
Contributor Author

Updated, should work now with #80 changes.

@nikitabobko
Copy link
Owner

nikitabobko commented Jan 5, 2024

I tried to set up this config to see how it goes. Is it ok that sketchybar renders on the desktop? Windows can overlay the "bar" (well, I'd not call it a "bar" in its current state)

image

Is it supposed to work that weird, or is it the config?

@nikitabobko
Copy link
Owner

And it gets me that the workspace switching feedback isn't immediate. I mean, AeroSpace command is fast, why is feedback switch slow then?

That's super suspicious.

I also tried to explicitly invoke sketchybar --refresh after the workspace switch to see if the feedback would be immediate. alt-0 = ['workspace 0', 'exec-and-forget sketchybar --refresh'] but it didn't refresh sketchybar...

@wojciech-kulik
Copy link
Contributor

@nikitabobko a small delay is I guess because of using DistributedNotificationCenter. It is an asynchronous operation and we have two points of delay: 1. system, 2. sketchybar.

@nikitabobko
Copy link
Owner

nikitabobko commented Jan 5, 2024

But @MatthiasGrandl says that sketchybar was fast when we were passing let userInfo = ["workspace": focusedWorkspaceName] to DistributedNotificationCenter.default().postNotificationName #80 (comment) => DistributedNotificationCenter is fast

Now AeroSpace still sends the DistributedNotificationCenter => sketchybar should be as fast to receive the event, and after receiving the event it should call aerospace list-workspaces --focused (which is also fast)

That all doesn't add up. It all makes me think that the problem is somewhere in sketchybar

@MatthiasGrandl
Copy link
Contributor Author

I tried to set up this config to see how it goes. Is it ok that sketchybar renders on the desktop? Windows can overlay the "bar" (well, I'd not call it a "bar" in its current state)

image Is it supposed to work that weird, or is it the config?

The workspace config is meant to be used with the default config, shipped in the sketchybar repo. Then you will have paddings around the numbers. Still not pretty. I am using a more complicated setup myself.

@MatthiasGrandl
Copy link
Contributor Author

MatthiasGrandl commented Jan 5, 2024

Now AeroSpace still sends the DistributedNotificationCenter => sketchybar should be as fast to receive the event, and after receiving the event it should call aerospace list-workspaces --focused (which is also fast)

I also experimented with this a bit more. I tried adding a second sketchybar event that fires when the notification is received. That event queries the active workspace and adds it to its payload.

this way the aerospace command only needs to be executed once instead of once per workspace. It seems to help somewhat, but still not as responsive as the initial userInfo implementation 🤷‍♂️

@FelixKratz
Copy link

FelixKratz commented Jan 5, 2024

The problem is not caused by sketchybar. Try running

sh -c 'time aerospace list-workspaces --focused'

it returns

real	0m0.061s
user	0m0.003s
sys	0m0.002s

which is already slow if you compare it to a sketchybar query. This might be due to the slow socket IPC used in AeroSpace vs the orders of magnitude faster mach IPC used in sketchybar. (Edit: yabai querys are actually as fast as sketchybar querys while using sockets, so it must be something else)

Additionally, there is another problem with the IPC AeroSpace uses, namely it is blocked if the main thread is congested. This will happen when many AX api calls are made, because this is a blocking API and will thus drastically increase response latency. To test this you can use the provided snippet to try and call the time command once the distributed notification is sent like this:

sketchybar --add event aerospace_change bobko.aerospace.focusedWorkspaceChanged
sketchybar --add item test left --set test script='time aerospace list-workspaces --focused' --subscribe test aerospace_change

resulting in:

real	0m0.173s
user	0m0.004s
sys	0m0.003s

Which is the raw time it takes to call the aerospace command and has nothing to do with sketchybar, as you can verify by manually triggering the aerospace_change event without an actual aerospace change occuring and congesting the main thread:

sketchybar --trigger aerospace_change

resulting in the same latency as

sh -c 'time aerospace list-workspaces --focused'

@wojciech-kulik
Copy link
Contributor

wojciech-kulik commented Jan 5, 2024

@FelixKratz I can confirm. I replaced aerospace command with some randomly generated value and it works instantly. So aerospace list-workspaces --visible is a bottle neck. In my case the lag is around 1 second.

Update:
actually, it looks like the command itself is quite fast. But the problem is that the configuration triggers this command for every space. Probably if we could list workspaces once it would be much faster. I'm using 8 workspaces so it takes around 1 second. Together it's 9 calls.

  sketchybar --add item space.$sid left \
    --subscribe space.$sid aerospace_change \
    --set space.$sid \
    background.color=0x44ffffff \
    background.corner_radius=5 \
    background.height=20 \
    background.drawing=off \
    label="$sid" \
    click_script="aerospace workspace $sid" \
    script="$PLUGIN_DIR/aerospace.sh $sid"

This code runs $PLUGIN_DIR/aerospace.sh for every space on every workspace change.

@FelixKratz
Copy link

You could easily set it up to only execute the aerospace command once, but still ~150ms of command latency during a workspace change is definitely noticeable.

@MatthiasGrandl
Copy link
Contributor Author

You could easily set it up to only execute the aerospace command once, but still ~150ms of command latency during a workspace change is definitely noticeable.

jup that's what I did. it definitely helps, but still not nice.

@wojciech-kulik
Copy link
Contributor

I updated my config to run only once this script:

#!/bin/bash

workspaces=$(aerospace list-workspaces)
visible_workspaces=$(aerospace list-workspaces --visible)

for sid in $workspaces; do
  NAME="space.$sid"
  visible=$(echo "$visible_workspaces" | grep -q "$sid" && echo "on" || echo "off")
  sketchybar --set $NAME icon.highlight=$visible label.highlight=$visible
done

Now it works better but still the lag from aerospace list-workspaces is noticeable. I have to call it twice to get both all and visible workspaces.

@nikitabobko
Copy link
Owner

Thanks for the comments!

I change my mind. I now agree that query commands need to be optimized -> #104

I updated my config to run only once this script:

Can you please write how you did that? I'm not fluent in sketchybar. --subscribe flag requires <name> of the item, but we don't want individual items to subscribe to events, we want to subscribe only once

@MatthiasGrandl
Copy link
Contributor Author

@nikitabobko I did it like this:

# Add AeroSpace workspace event
sketchybar --add event aerospace_change bobko.aerospace.focusedWorkspaceChanged \
  --add item ev left \
  --subscribe ev aerospace_change \
  --set ev script='sketchybar --trigger aerospace FOCUSED="$(aerospace list-workspaces --focused)"' \
  --add event aerospace aerospace

then you have access to the FOCUSED variable when you subscribe to aerospace. Idk if that's the best way to do it, @FelixKratz could chime on for best practices.

@FelixKratz
Copy link

FelixKratz commented Jan 7, 2024

The most elegant way to provide this information would be to add the space change event info to the payload of the distributed notification. Since the AX api is used, it will never be possible to sandbox this app (AX and Sandbox are incompatible by nature), so passing a non-null payload is no constraint.

But if thats not possible, I would also do it as @MatthiasGrandl showed, creating a (invisible) dummy item that annotates the event from aerospace with the required info and forwards it to all other items:

sketchybar --add event aerospace_raw bobko.aerospace.focusedWorkspaceChanged \
  --add event aerospace_change \
  --add item ev left \
  --set ev updates=on drawing=off \
           script='sketchybar --trigger aerospace_change FOCUSED="$(aerospace list-workspaces --focused)"' \
  --subscribe ev aerospace_raw

The space items would now subscribe to the annotated aerospace_change event and receive the FOCUSED env variable in their script.

@MatthiasGrandl
Copy link
Contributor Author

The most elegant way to provide this information would be to add the space change event info to the payload of the distributed notification. Since the AX api is used, it will never be possible to sandbox this app (AX and Sandbox are incompatible by nature), so passing a non-null payload is no constraint.

Jup that was my approach in the PR #80 , but we changed it due to the sandboxing concerns.

@nikitabobko
Copy link
Owner

Since the AX api is used, it will never be possible to sandbox this app (AX and Sandbox are incompatible by nature), so passing a non-null payload is no constraint.

Most probably it's true. But I still don't want to use userInfo in postNotificationName.

It's some obscure non-composable Apple API, but I want to keep AeroSpace close to UNIX foundations. I don't see any standard ways to send NSDistributedNotifications from CLI, and I can't find any popular third-party tool that would do that.

I haven't used NSDistributedNotifications before and I don't know what implications it might have. The way AeroSpace communicates with other applications is a "public API", and it must be chosen and designed carefully.

Frankly speaking, I didn't want to include the patch that uses DistributedNotificationCenter, in the first place. I'd prefer the implementation that would allow to specify arbitrary shell commands on workspace change #87

My opinion is close to "religious", but it's what it is right now. I might change it later

@nikitabobko
Copy link
Owner

I adapted the PR and merged it

Thanks!

@FelixKratz
Copy link

FelixKratz commented Jan 8, 2024

Frankly speaking, I didn't want to include the patch that uses DistributedNotificationCenter, in the first place. I'd prefer the implementation that would allow to specify arbitrary shell commands on workspace change #87

This has another benefit, where you will be able to provide environment variables to scripts. Once this is included, the superior implementation would be to call the sketchybar --trigger aerospace_change event in this workspace change shell script and simply pass the relevant environment variables you set in AeroSpace down to sketchybar. Then there would be no need at all for using DistributedNotifications.

nikitabobko added a commit that referenced this pull request Jan 13, 2024
@nikitabobko
Copy link
Owner

I've dropped NSDistributedNotification in favor of exec-on-workspace-change that can run arbitrary process 37e4a50

The process has access to AEROSPACE_FOCUSED_WORKSPACE and AEROSPACE_PREV_WORKSPACE env variables

@MatthiasGrandl
Copy link
Contributor Author

I've dropped NSDistributedNotification in favor of exec-on-workspace-change that can run arbitrary process 37e4a50

The process has access to AEROSPACE_FOCUSED_WORKSPACE and AEROSPACE_PREV_WORKSPACE env variables

Sounds good to me!

jakenvac pushed a commit to jakenvac/AeroSpace that referenced this pull request Aug 16, 2024
Co-authored-by: Nikita Bobko <[email protected]>

closes nikitabobko#81
jakenvac pushed a commit to jakenvac/AeroSpace that referenced this pull request Aug 16, 2024
@nikitabobko nikitabobko added the pr-accepted Pull Request is accepted label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-accepted Pull Request is accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants