Implement focus dfs-next/dfs-prev. #1243

rickyz · 2025-03-28T09:28:37Z

This allows switching to the next/prev window from the current one in
the depth-first order of the windows in the current workspace tree.

This is convenient, as it allows users to shift focus through a
workspace's windows in a more predictable way than the directional
movement commands, where the move target can depend on invisible
most-recently-used state.

_fixes #248

rickyz · 2025-03-29T16:20:55Z

I added an implementation of swap as well (which depends on part of the implementation of focus (dfs-next|dfs-prev), so it was most convenient to include in this PR as well. Happy to split that out if preferred though!

timobenn · 2025-04-01T19:42:24Z

Somewhat unfortunate/amusing timing, I had been poking at this same feature (and learning Swift), though I don't consider mine ready for a PR yet: main...timobenn:AeroSpace:focus-dfs-next-prev-248

A couple of things to note:

neither your PR nor my branch (yet) support floating windows; they're not included in the list of leaf windows. I was going to see today if simply appending the list of floating windows to the list of leaf windows (unless --ignore-floating is set) would feel natural in use.
my branch supports both workspace and all-monitors-outer-frame boundary options -- feel free to steal that

nikitabobko · 2025-04-01T20:10:25Z

neither your PR nor my branch (yet) support floating windows

@timobenn wdym? This PR supports focusing floating windows via dfs-next|dfs-prev

timobenn · 2025-04-01T20:16:30Z

@nikitabobko when I'd tested mine last week, I came to the conclusion that target.workspace.rootTilingContainer.allLeafWindowsRecursive does not include floating windows, so any index operations on just that list would skip floating windows (thus I added a note). It's possible I misinterpreted my testing, but it looks like this PR would run into the same issue I was, where focus dfs-next/dfs-prev would just skip floating windows. At least, I don't see how it would include them.

(edit: not on a Mac right now, otherwise I'd pull & test)

nikitabobko

I didn't read all of the code yet, here the first portion of comments

Overall - looks good, thanks

I don't care if the commits are in one big PR or two separate PRs. I review commit by commit anyway.

If your second commit depends on the first one, I understand why you put them into a single PR. Otherwise, you might yourself prefer to split it into two different PRs, because the code review will take longer with several commits in one PR

nikitabobko · 2025-03-30T18:54:18Z

docs/aerospace-swap.adoc

@@ -0,0 +1,45 @@
+= aerospace-swap(1)


Please update ./docs/commands.adoc

In future, it will be probably auto-generated #1027

nikitabobko · 2025-04-01T18:53:46Z

docs/aerospace-swap.adoc

+== Synopsis
+[verse]
+// tag::synopsis[]
+aerospace swap [-h|--help] [--move-focus-to-target] [--wrap-around]


--move-focus-to-target is wrong. Probably you meant --swap-focus

Oops, fixed

nikitabobko · 2025-04-01T18:55:04Z

Sources/Common/cmdArgs/impl/SwapCmdArgs.swift

+        allowInConfig: true,
+        help: swap_help_generated,
+        options: [
+            "--swap-focus": trueBoolFlag(\.swapFocus),


Love the --swap-focus flag

nikitabobko · 2025-04-01T19:00:37Z

Sources/AppBundle/command/impl/SwapCommand.swift

+    let args: SwapCmdArgs
+
+    func run(_ env: CmdEnv, _ io: CmdIo) -> Bool {
+        guard let target = args.resolveTargetOrReportError(env, io), let currentWindow = target.windowOrNil else {


Please split these two lets into separate guards. Otherwise, if resolveTargetOrReportError returns nil, the noWindowIsFocused error is incorrect

Done (though now that I see that target could be overridden by resolveTargetOrReportError, is it even correct to emit "noWindowIsFocused" in the second guard?

nikitabobko · 2025-04-01T19:06:21Z

Sources/AppBundle/command/impl/SwapCommand.swift

+            return io.err(noWindowIsFocused)
+        }
+
+        var targetWindow: Window?


Suggested change

var targetWindow: Window?

let targetWindow: Window?

nikitabobko · 2025-04-01T20:21:10Z

Sources/Common/cmdArgs/impl/FocusCmdArgs.swift

+        case "right": .success(.direction(.right))
+        case "dfs-next": .success(.dfsRelative(.next))
+        case "dfs-prev": .success(.dfsRelative(.prev))
+        default: .failure("Can't parse '\(arg)\'. Possible values: left|down|up|right|dfs-next|dfs-prev")


The same for parsing the argument itself. I don't like the hardcoded case "left", case "down", case "dfs-next". I think it's possible to make use of CaseIterable at least manually (ideally - automatically, recursively)

Renamed FocusTargetArg to CardinalOrDfsDirection and implemented the CaseIterable and RawRepresentable protocols for it so that parseEnum and unionLiteral can be used.

nikitabobko · 2025-04-01T20:25:47Z

docs/aerospace-focus.adoc

+*1. (left|down|up|right) arguments*
+
+{manpurpose}
+
+*2. (dfs-next|dfs-prev) arguments*
+
+Set focus to the window before or after the current window in the depth-first order (top-to-bottom and left-to-right) of windows in the current workspace tree.
+In this mode, `--boundaries` must be `workspace` (the default) and `--boundaries-action` can be set to one of `(stop|fail|wrap-around-the-workspace)`.


I know that docs for some of other commands written in this style, but please prefer the style with ARGUMENTS section like in aerospace-focus-monitor

Other commands that use this style of documentation (e.g. workspace) will be migrated to ARGUMENTS-section-style in the future

Thanks for the example, switched to ARGUMENTS-section-style.

nikitabobko · 2025-04-01T20:27:33Z

docs/aerospace-focus.adoc

@@ -12,6 +12,9 @@ include::util/man-attributes.adoc[]
 aerospace focus [-h|--help] [--ignore-floating]
                [--boundaries <boundary>] [--boundaries-action <action>]
                (left|down|up|right)
+aerospace focus [-h|--help] [--ignore-floating]


--ignore-floating is also supported here

nikitabobko · 2025-04-01T20:29:18Z

grammar/commands-bnf-grammar.txt

+<focus_dir_flag> ::= --boundaries <boundary>|--boundaries-action <dir_boundaries_action>|--ignore-floating;
+<focus_dfs_relative_flag> ::= --boundaries-action <dfs_relative_boundaries_action>|--ignore-floating;
+<dir_boundaries_action> ::= stop|fail|wrap-around-the-workspace|wrap-around-all-monitors;
+<dfs_relative_boundaries_action> ::= stop|fail|wrap-around-the-workspace;


I'd prefer the shell completion to suggest yet unsupported wrap-around-all-monitors rather than complicating the grammar

Done - I had left it out because I wasn't sure that wrap-around-all-monitors made sense for dfs-next/dfs-prev. Within a workspace, the ordering of DFS indices are well defined, but it wasn't clear to me that there was a clear correct next monitor to wrap around to.

nikitabobko · 2025-04-01T20:30:36Z

grammar/commands-bnf-grammar.txt

@@ -20,9 +20,10 @@ aerospace -h;

    | flatten-workspace-tree [--workspace <workspace>]

-    | focus [<focus_flag>]... (left|down|up|right) [<focus_flag>]...
+    | focus [<focus_dir_flag>]... (left|down|up|right) [<focus_dir_flag>]...


IMO "dir" = "direction" is not obvious shortening

Renamed to focus_direction_flag

nikitabobko · 2025-04-01T20:36:35Z

@timobenn since focus command is special in that regard, it has a small "hack" inside of it

AeroSpace/Sources/AppBundle/command/impl/FocusCommand.swift

Lines 9 to 15 in fb3a863

    
           // todo bug: floating windows break mru 
        
           let floatingWindows = args.floatingAsTiling ? makeFloatingWindowsSeenAsTiling(workspace: target.workspace) : [] 
        
           defer { 
        
               if args.floatingAsTiling { 
        
                   restoreFloatingWindows(floatingWindows: floatingWindows, workspace: target.workspace) 
        
               } 
        
           }

timobenn · 2025-04-01T20:59:13Z

@nikitabobko I must have been running into a weird issue with window state when first launching the debug build on top of a running instance; when I test today I'm able to get both my branch and this PR to acknowledge floating windows correctly, as long as I make sure to fully reset the layout after launch first, before floating any windows.

timobenn · 2025-04-01T21:08:12Z

If you all have interest in having focus dfs-next|dfs-prev fully function with all-monitors-outer-frame & wrap-around-all-monitors, perhaps I can put up a smaller PR after this is merged.

rickyz · 2025-04-02T11:49:26Z

Thanks for the review!

@timobenn Oops, sorry for the unintended duplication of work - I took your nicer naming for the focus argument, but didn't pull in the monitor wrapping code - feel free to follow up with that if folks are able to agree on what that behavior would be!

fwiw, I didn't attempt to implement wrap-around-all-monitors because I wasn't sure that there was a well-defined next monitor to wrap to with dfs-next/dfs-prev. With the cardinal directions, it makes sense to wrap to the opposite direction edge, but couldn't think of something analogous for monitors (especially since the prev focused window could be adjacent to multiple monitors).

timobenn · 2025-04-02T23:09:46Z

@rickyz

@timobenn Oops, sorry for the unintended duplication of work - I took your nicer naming for the focus argument, but didn't pull in the monitor wrapping code - feel free to follow up with that if folks are able to agree on what that behavior would be!

No worries! Risk I took by working so slowly without broadcasting I was doing it.

For wrapping around all monitors I was using this logic:

when it's time to pick a different monitor, use the same logic as is used by focus-monitor [--wrap-around] (next|prev), which goes to the "next" or "prev" monitor based on the same order they are shown in the menu bar
if dfs-next, go to first DFS window it the new monitor, else the last DFS window

For the limited amount of manual testing I'd done so far, that logic was feeling natural.

rickyz · 2025-04-04T03:56:03Z

Ah, I see - I wasn't aware that monitors already had some fixed ordering to them.

Re the earlier discussion about floating windows - it looks like the current trick for dealing with floating windows inserts them into the window tree at a location depending on its location on the screen. As a result, they are assigned an unintuitive DFS position when using dfs-next and dfs-prev - I think I would actually prefer we explicitly sorted floating windows after all of the windows in the tree instead - can update the implementation to do that if there are no objections (though I guess the downside is that the command might cycle through multiple floating windows in an arbitrary-feeling order instead - there may also be implications for the behavior of things like --dfs-index if we are deciding to define a meaningful DFS index value for floating windows).

timobenn · 2025-04-04T16:16:54Z

I do agree that floating windows being mixed among the leaves the way they currently are feels a bit weird in the context of dfs-next/prev. I'm not sure how much better grouping them together would feel (though I expect it would be at least a little), or how much complexity would be added by handling floating windows differently based on the focus target.

This allows switching to the next/prev window from the current one in the depth-first order of the windows in the current workspace tree. This is convenient, as it allows users to shift focus through a workspace's windows in a more predictable way than the directional movement commands, where the move target can depend on invisible most-recently-used state. _fixes nikitabobko#248

`aerospace swap` swaps the currently focused window with a window in a cardinal direction or with the next/prev window in the depth first order of windows in the workspace. Target window selection works identically to the focus command (so the cardinal directions respect MRU). _fixes nikitabobko#8

rickyz force-pushed the focus_dfs_relative branch 3 times, most recently from 820d9df to 9d68666 Compare March 29, 2025 15:59

nikitabobko requested changes Apr 1, 2025

View reviewed changes

rickyz force-pushed the focus_dfs_relative branch 2 times, most recently from 203c892 to b9b9b86 Compare April 2, 2025 11:35

rickyz force-pushed the focus_dfs_relative branch from b9b9b86 to 3a0b207 Compare April 2, 2025 11:58

rickyz force-pushed the focus_dfs_relative branch from 3a0b207 to f4a4a73 Compare April 14, 2025 02:33

rickyz added 2 commits April 18, 2025 05:50

rickyz force-pushed the focus_dfs_relative branch from f4a4a73 to a1b88a5 Compare April 18, 2025 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement focus dfs-next/dfs-prev. #1243

Implement focus dfs-next/dfs-prev. #1243

rickyz commented Mar 28, 2025

rickyz commented Mar 29, 2025

timobenn commented Apr 1, 2025 •

edited

Loading

nikitabobko commented Apr 1, 2025

timobenn commented Apr 1, 2025 •

edited

Loading

nikitabobko left a comment

nikitabobko Mar 30, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

nikitabobko Apr 1, 2025 •

edited

Loading

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko Apr 1, 2025

rickyz Apr 2, 2025

nikitabobko commented Apr 1, 2025

timobenn commented Apr 1, 2025

timobenn commented Apr 1, 2025 •

edited

Loading

rickyz commented Apr 2, 2025

timobenn commented Apr 2, 2025

rickyz commented Apr 4, 2025 •

edited

Loading

timobenn commented Apr 4, 2025

Implement focus dfs-next/dfs-prev. #1243

Are you sure you want to change the base?

Implement focus dfs-next/dfs-prev. #1243

Conversation

rickyz commented Mar 28, 2025

rickyz commented Mar 29, 2025

timobenn commented Apr 1, 2025 • edited Loading

nikitabobko commented Apr 1, 2025

timobenn commented Apr 1, 2025 • edited Loading

nikitabobko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitabobko Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitabobko commented Apr 1, 2025

timobenn commented Apr 1, 2025

timobenn commented Apr 1, 2025 • edited Loading

rickyz commented Apr 2, 2025

timobenn commented Apr 2, 2025

rickyz commented Apr 4, 2025 • edited Loading

timobenn commented Apr 4, 2025

timobenn commented Apr 1, 2025 •

edited

Loading

timobenn commented Apr 1, 2025 •

edited

Loading

nikitabobko Apr 1, 2025 •

edited

Loading

timobenn commented Apr 1, 2025 •

edited

Loading

rickyz commented Apr 4, 2025 •

edited

Loading