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

Feature: workspace next|prev --all #264

Closed
ilyaluk opened this issue Jun 10, 2024 · 5 comments
Closed

Feature: workspace next|prev --all #264

ilyaluk opened this issue Jun 10, 2024 · 5 comments

Comments

@ilyaluk
Copy link
Contributor

ilyaluk commented Jun 10, 2024

Hi! First of all, kudos for creating such awesome tool! I tried to use yabai in the past, but couldn't bear native worspace switching speed (even with reduced animations), compared to i3. AeroSpace's virtual workspaces work extremely fast in comparison.

Use-case

I'd like to bind alt-tab/alt-shift-tab to cycle through all workspaces, regardless of monitor. This can be solved like that:

alt-tab = 'exec-and-forget aerospace list-workspaces --all | aerospace workspace --wrap-around next'
alt-shift-tab = 'exec-and-forget aerospace list-workspaces --all | aerospace workspace --wrap-around prev'

However, this adds significant delay: ~30-50ms for listing workspaces, and 50+ ms for spawning bash and piping/calling aerospace workspace.

Having static list of workspaces, aerospace list-workspaces --all can be replaced with seq 1 9 or even static echo $'1\n2\n...', however this does not eliminate overhead of spawning subprocesses and calling aerospace CLI, which becomes noticeable, because I navigate workspaces in that way quite a lot.

Proposal

Argument --all could be added to workspace (next|prev) syntax to implement this kind of behaviour (similar to list-(windows|workspaces))

I'd be happy to submit a PR if you're okay with such feature.

@nikitabobko
Copy link
Owner

(1) As long as your provide time measurements, that prove that exec-and-forget aerospace list-workspaces --all | aerospace workspace next is noticeably slower (at least 100ms) than workspace --all next, I'm ok with such feature.

You can measure time in the following way:

diff --git a/Sources/AppBundle/config/HotkeyBinding.swift b/Sources/AppBundle/config/HotkeyBinding.swift
index 18aa3274..e0b73b73 100644
--- a/Sources/AppBundle/config/HotkeyBinding.swift
+++ b/Sources/AppBundle/config/HotkeyBinding.swift
@@ -26,6 +26,7 @@ func activateMode(_ targetMode: String?) {
     let targetBindings = targetMode.flatMap { config.modes[$0] }?.bindings ?? [:]
     for binding in targetBindings.values where !hotkeys.keys.contains(binding.binding) {
         hotkeys[binding.binding] = HotKey(key: binding.key, modifiers: binding.modifiers, keyDownHandler: {
+            print("--- \(binding.binding) pressed. The current active app is \(getNativeFocusedWindow(startup: false)?.app.name). Time: \(Date().timeIntervalSince1970)")
             if let activeMode {
                 refreshSession(forceFocus: true) {
                     _ = config.modes[activeMode]?.bindings[binding.binding]?.commands.run(.focused)
diff --git a/Sources/AppBundle/refresh.swift b/Sources/AppBundle/refresh.swift
index f8f3d489..8c1b9d7e 100644
--- a/Sources/AppBundle/refresh.swift
+++ b/Sources/AppBundle/refresh.swift
@@ -5,6 +5,8 @@ import Common
 /// The function is called as a feedback response on every user input.
 /// The function is idempotent.
 func refreshSession<T>(startup: Bool = false, forceFocus: Bool = false, body: () -> T) -> T {
+    // Once the focused app changes, you can track it in the log
+    print("--- Process event. Focused app: \(getNativeFocusedWindow(startup: false)?.app.name). Time: \(Date().timeIntervalSince1970)")
     check(Thread.current.isMainThread)
     gc()
 

(2) --all is an alias for --monitor all. I'd not rush adding the alias to workspace command. I'd prefer to add a more detailed argument --monitor (all|mouse|focused)

(3) Please don't forget to update the ./docs/aerospace-workspace.adoc

(4) Please don't forget to report an error (stderr + exit-code) if both stdin is not empty and --monitor is supplied

(5) Out of curiosity, why do you prefer to cycle workspace on all monitors? When workspace (next|prev) was initially introduced, it cycled all monitors, but then we realized that the preferred option is to only cycle workspaces on the focused monitor, that's why I changed the default #94 I want to understand if the decision was right


Sidenotes (feel free to ignore them)

Sidenote 1. I understand that AeroSpace could be faster #104, but it still makes me sad that the currently provided solution aerospace list-workspaces --all | aerospace workspace --wrap-around next is slow enough for people to notice it.

The whole point of stdin consumption in workspace command was to avoid arguments parsing logic repetition/reuse. Aparently, stdin consumption is useless, since spawning bash and piping/calling takes noticeable time

Sidenote 2. Maybe it's worth to implement common shell separators &&, ||, ;, and pipe operator inside TOML config. It's not that hard but unlocks a whole variety of use cases. E.g. alt-q = 'close || close --quit-if-last-window' (best effort close), alt-1 = 'workspace 1 || workspace-back-and-forth' (--auto-back-and-forth replacement)

@nikitabobko
Copy link
Owner

As long as your provide time measurements, that prove that exec-and-forget aerospace list-workspaces --all | aerospace workspace next is noticeably slower (at least 100ms) than workspace --all next, I'm ok with such feature.

You can compare alt-tab = 'exec-and-forget aerospace list-workspaces --all | aerospace workspace next' vs alt-tab = 'workspace next' before implementing --monitor all. --monitor all won't slow anything down

@nikitabobko
Copy link
Owner

nikitabobko commented Jun 14, 2024

After thinking more about it, I lean more towards supporting shell-like combinators (sidenote 2)

It's needed anyway to reduce flickering in this case https://nikitabobko.github.io/AeroSpace/goodness#use-trackpad-gestures-to-switch-workspaces

Copying flags from list-workspaces is an abstraction leak, and a violation of single responsibility principle.

I will create the tracking issue for the shell-like combinators later and post here as a comment.

@ilyaluk
Copy link
Contributor Author

ilyaluk commented Jun 15, 2024

@nikitabobko
Hi! Thanks for such detailed reply!

As long as your provide time measurements, that prove that exec-and-forget aerospace list-workspaces --all | aerospace workspace next is noticeably slower (at least 100ms) than workspace --all next, I'm ok with such feature

Thanks for providing example diff! So, I tried various options (with only one monitor, but I'm pretty sure that native filtering would not add such impact):

alt-q = 'exec-and-forget /…/.debug/aerospace list-workspaces --all | /…/.debug/aerospace workspace --wrap-around next'
alt-w = 'exec-and-forget seq 1 9 | /…/.debug/aerospace workspace --wrap-around next'
alt-tab = 'workspace --wrap-around next'

And then manually checked the logs for time delta between keydown and first focused app chage event. That yielded following results on my M1 MBA:

alt-q 355.85, 414.78, 276.46, 361.82, 291.08, 341.92, 370.41, 332.20, 267.04
alt-w 277.67, 238.14, 216.59, 297.80, 379.01, 424.73, 493.56, 269.42
alt-tab 116.60, 100.33, 152.95, 170.88, 121.60, 116.59, 166.63, 95.38

alt-q: mean=334.62ms stddev=45.71ms
alt-w: mean=324.62ms stddev=91.27ms
alt-tab: mean=130.12ms stddev=27.50ms

Which is a bit weird, considering that alt-q should be a bit slower. Maybe the test was not clean enough (spaces had different number of windows), but the slow-down is substantial.

Out of curiosity, why do you prefer to cycle workspace on all monitors?

Force of habit, I guess. I've had an i3 setup a long time ago, with alt-tab for workspace next, not workspace next_on_output. Also, it's quite convenient when I have workspaces 1/2/5-9 on main display, and 3/4 on secondary. Mainly I use 1/2, but regularly switch to 3. alt-tab/alt-shift-tab is a bit quicker and requires less mental effort than alt-2/alt-3 in that case.

I understand that AeroSpace could be faster #104, but it still makes me sad that the currently provided solution aerospace list-workspaces --all | aerospace workspace --wrap-around next is slow enough for people to notice it

I guess, it's not that noticeable, but I'm pretty picky :)
Overall AeroSpace is a breath of fresh air compared to native workspace switching.

I'll be happy to contribute to any performance-related improvements, do you maybe have a good first issue in mind?

Copying flags from list-workspaces is an abstraction leak, and a violation of single responsibility principle.

Yeah, that makes sense.

@nikitabobko
Copy link
Owner

I will create the tracking issue for the shell-like combinators later and post here as a comment.

#278

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

No branches or pull requests

2 participants