feat: add `--boundaries` options to move command #864

EcmaXp · 2024-12-16T10:45:29Z

Summary

Resolves [Feature Request] Add --boundaries options to move command #183

This PR introduces a new --boundaries option to the move command, allowing windows to move beyond their current workspace.

By default, window movements remain within a single workspace. With --boundaries all-monitors-outer-frame, windows can now be moved across multiple monitors for increased flexibility.

Key Changes

move Command Updates:

- aerospace move [-h|--help] [--window-id <window-id>] (left|down|up|right)
+ aerospace move [-h|--help] [--window-id <window-id>] [--focus-follows-window]
+                [--fail-if-noop] [--boundaries <boundary>] [--wrap-around] (left|down|up|right)

--boundaries <boundary>: Choose between staying within the workspace or all-monitors-outer-frame.
--wrap-around: Cycle through monitors when reaching an edge.
--focus-follows-window: Automatically focus the moved window.
--fail-if-noop: Exit with a non-zero status if the window cannot move (e.g., hitting the boundary without --boundaries all-monitors-outer-frame).

Why?

These enhancements provide more control over window movement in multi-monitor setups and improve scripting scenarios by offering fail-fast behavior, wrap-around cycling, and automatic refocusing.

Example

# aerospace.toml
ctrl-alt-left = "move left --boundaries all-monitors-outer-frame --focus-follows-window"
ctrl-alt-down = "move down --boundaries all-monitors-outer-frame --focus-follows-window"
ctrl-alt-up = "move up --boundaries all-monitors-outer-frame --focus-follows-window"
ctrl-alt-right = "move right --boundaries all-monitors-outer-frame --focus-follows-window"

…tors

dvrkoo · 2024-12-16T13:50:03Z

Currently switching between linux with Hyprland and macOS with Aerospace a lot and this is one of features I was missing the most, thanks a lot

nikitabobko

Thanks for the effort, but I reject the PR. Your understanding of the mental model of moving windows is incorrect. The PR is fundamentally wrong.

nikitabobko · 2024-12-17T10:51:00Z

docs/aerospace-move.adoc

@@ -25,6 +26,17 @@ Deprecated name: `move-through`
 include::./util/conditional-options-header.adoc[]

 -h, --help:: Print help
+--fail-if-noop:: Exit with a non-zero status if the window cannot move because it’s at the boundary


I didn't read the code yet, but move command cannot fail. Please read #183 thread

The only possible case when it can fail is --boundaries-action (stop|create-implicit-container|fail)

I’ve reviewed #183 and your feedback.

I’m a bit unsure about how to best design the arguments, but I’ll reconsider the approach.

nikitabobko · 2024-12-17T10:52:40Z

docs/aerospace-move.adoc

+`<boundary>` possible values: `(workspace|all-monitors-outer-frame)`. +
+The default is: `workspace`
+
+--focus-follows-window::


I didn't read the code yet, but move already always follows window, I don't understand why you need this flag

nikitabobko · 2024-12-17T10:54:57Z

docs/aerospace-move.adoc

@@ -25,6 +26,17 @@ Deprecated name: `move-through`
 include::./util/conditional-options-header.adoc[]

 -h, --help:: Print help
+--fail-if-noop:: Exit with a non-zero status if the window cannot move because it’s at the boundary
+--wrap-around:: Make it possible to wrap around the movement


I agree on: --boundaries-action (stop|create-implicit-container|fail|wrap-around-the-workspace|wrap-around-all-monitors), but I don't think that --wrap-around on its own is the right flag

nikitabobko · 2024-12-17T11:20:29Z

Sources/AppBundle/command/impl/MoveCommand.swift

@@ -56,6 +64,10 @@ private func moveOut(_ io: CmdIo, window: Window, direction: CardinalDirection)
            bindTo = parent
            bindToIndex = innerMostChild.ownIndex + direction.insertionOffset
        case .workspace(let parent): // create implicit container
+            if !canMoveOutInDirection(window: window, direction: direction) {


From what I read in the code, you disallow creating implicit containers when the window is moved along the axis of the rootTilingNode. This is wrong approach. It should be conditional behavior of --boundaries-action

By default it should always be possible to move out window out of container. Please, read #183 thread

nikitabobko · 2024-12-17T11:30:33Z

Sources/Common/cmdArgs/impl/MoveCmdArgs.swift

    public var workspaceName: WorkspaceName?
+    public var failIfNoop: Bool = false
+    public var rawBoundaries: Boundaries? = nil
+    public var moveNodeToMonitor = MoveNodeToMonitorCmdArgs(rawArgs: [])


I didn't think a lot about it, but it may not be the right place to reuse move-node-to-monitor

I don’t fully agree. MoveNodeToMonitorCommand already calls MoveNodeToWorkspaceCommand in a similar way. There might be other approaches, but I’m not sure yet.

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

Lines 19 to 20 in 2891df5

let moveNodeToWorkspace = args.moveNodeToWorkspace.copy(\.target, .initialized(.direct(wName)))

return MoveNodeToWorkspaceCommand(args: moveNodeToWorkspace).run(env, io)

EcmaXp · 2024-12-17T22:03:19Z

Thanks for the effort, but I reject the PR. Your understanding of the mental model of moving windows is incorrect. The PR is fundamentally wrong.

Thanks for taking the time to review my PR. I appreciate your feedback and insights.

I’ll revisit the approach based on your comments and open a new PR that aligns more closely with the intended mental model.

EcmaXp marked this pull request as ready for review December 16, 2024 10:59

EcmaXp added 8 commits December 16, 2024 20:02

feat: return error if window is not moved in move cmd

c41f065

feat: add --fall-if-noop option to move cmd

14d43ba

feat: move window with across monitors in move cmd

96d6606

fix: fix incorrect comparison and enforce container boundary checks

2431db2

feat: improve window relocation behavior when moving to adjacent moni…

4d5e037

…tors

fix: remove Equatable extension from Rect

43f326d

docs: update move command docs and grammar to reflect new flags

dcdd89d

chore: remove semicolon

b274760

EcmaXp force-pushed the feature/move-with-boundaries branch from 5592a0f to b274760 Compare December 16, 2024 11:02

nikitabobko reviewed Dec 17, 2024

View reviewed changes

nikitabobko added the pr-rejected Pull Request is rejected label Dec 17, 2024

nikitabobko closed this Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `--boundaries` options to move command #864

feat: add `--boundaries` options to move command #864

EcmaXp commented Dec 16, 2024 •

edited

Loading

dvrkoo commented Dec 16, 2024

nikitabobko left a comment

nikitabobko Dec 17, 2024

nikitabobko Dec 17, 2024

EcmaXp Dec 17, 2024

nikitabobko Dec 17, 2024

nikitabobko Dec 17, 2024

nikitabobko Dec 17, 2024

nikitabobko Dec 17, 2024

EcmaXp Dec 17, 2024

EcmaXp commented Dec 17, 2024

	let moveNodeToWorkspace = args.moveNodeToWorkspace.copy(\.target, .initialized(.direct(wName)))
	return MoveNodeToWorkspaceCommand(args: moveNodeToWorkspace).run(env, io)

feat: add --boundaries options to move command #864

feat: add --boundaries options to move command #864

Conversation

EcmaXp commented Dec 16, 2024 • edited Loading

Summary

Key Changes

Why?

Example

dvrkoo commented Dec 16, 2024

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

EcmaXp commented Dec 17, 2024

feat: add `--boundaries` options to move command #864

feat: add `--boundaries` options to move command #864

EcmaXp commented Dec 16, 2024 •

edited

Loading