add scratchpads by auscyber · Pull Request #233 · acsandmann/rift

auscyber · 2026-01-19T12:49:30Z

No description provided.

acsandmann · 2026-01-20T14:28:32Z

can you please remove all of the unnecessary files or open a fresh pr?

auscyber · 2026-02-19T05:03:47Z

the nix files are so it can be built with nix,

Copilot

Pull request overview

This pull request adds scratchpad functionality to Rift, a tiling window manager. Scratchpads are floating windows that can be quickly shown/hidden on demand, similar to dropdown terminals in other window managers. The implementation includes named scratchpads (allowing different scratchpads for different purposes), app rule-based scratchpad assignment, and keyboard commands for managing scratchpads.

Changes:

Added new ScratchpadManager module with comprehensive test coverage for managing scratchpad windows
Extended app workspace rules to support scratchpad configuration (boolean or named)
Added CLI commands (add_scratchpad, toggle_scratchpad) and keyboard shortcuts for scratchpad operations
Modified EventResponse to support hiding windows, enabling scratchpad windows to be hidden when inactive

Reviewed changes

Copilot reviewed 14 out of 17 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/layout_engine/scratchpad.rs	New module implementing scratchpad window management with full test coverage
src/layout_engine/engine.rs	Integration of scratchpad functionality into layout engine, added hide_windows to EventResponse
src/layout_engine/floating.rs	Added is_active method for checking floating window visibility state
src/layout_engine.rs	Module declarations for new scratchpad functionality
src/model/virtual_workspace.rs	Support for scratchpad in app rules, removed Copy trait due to String field
src/common/config.rs	Added ScratchpadConfig enum for app rule configuration
src/actor/reactor.rs	Implementation of window hiding logic for scratchpads
src/actor/app.rs	Added Activate request for focusing apps
src/bin/rift-cli.rs	CLI commands for scratchpad operations
rift.default.toml	Example scratchpad keybindings in default config
src/actor/reactor/tests.rs	Updated tests for new hide_windows field in EventResponse
src/actor/reactor/testing.rs	Test utility updates
flake.nix	Nix flake for development environment setup
flake.lock	Nix flake lock file
.vscode/settings.json	VS Code Nix environment configuration
.gitignore	Added Nix development directories
.envrc	direnv configuration for Nix development environment

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-19T05:09:30Z

src/actor/reactor.rs

+#[derive(Clone, Debug)]
+struct Screen {
+    frame: CGRect,
+    space: Option<SpaceId>,
+    display_uuid: String,
+    name: Option<String>,
+    screen_id: ScreenId,
+}
+
+impl Screen {
+    fn display_uuid_opt(&self) -> Option<&str> {
+        if self.display_uuid.is_empty() {
+            None
+        } else {
+            Some(self.display_uuid.as_str())
+        }
+    }
+
+    fn display_uuid_owned(&self) -> Option<String> {
+        self.display_uuid_opt().map(|uuid| uuid.to_string())
+    }
+}
+


This Screen struct and its implementation appear to be unused in the codebase. If this was added in preparation for future work, it should be removed until actually needed, or if it is used, the usage is not visible in the diff.

Suggested change

#[derive(Clone, Debug)]

struct Screen {

frame: CGRect,

space: Option<SpaceId>,

display_uuid: String,

name: Option<String>,

screen_id: ScreenId,

}

impl Screen {

fn display_uuid_opt(&self) -> Option<&str> {

if self.display_uuid.is_empty() {

None

} else {

Some(self.display_uuid.as_str())

}

}

fn display_uuid_owned(&self) -> Option<String> {

self.display_uuid_opt().map(|uuid| uuid.to_string())

}

}

Copilot · 2026-02-19T05:09:30Z

src/layout_engine/engine.rs

+                self.toggle_stack_for_workspace(workspace_id, layout, default_orientation);
+                let unstacked_windows =
+                    self.workspace_tree_mut(workspace_id)
+                        .unstack_parent_of_selection(layout, default_orientation);
+
+                if !unstacked_windows.is_empty() {
+                    return EventResponse {
+                        raise_windows: unstacked_windows,
+                        focus_window: None,
+                        ..Default::default()
+                    };
+                }
+
+                let stacked_windows =
+                    self.workspace_tree_mut(workspace_id)
+                        .apply_stacking_to_parent_of_selection(layout, default_orientation);
+                if !stacked_windows.is_empty() {
+                    return EventResponse {
+                        raise_windows: stacked_windows,
+                        focus_window: None,
+                        ..Default::default()
+                    };
+                }
+
+                let visible_windows = self.workspace_tree(workspace_id).visible_windows_in_layout(layout);
+                if !visible_windows.is_empty() {
+                    EventResponse {
+                        raise_windows: visible_windows,
+                        focus_window: None,
+                        ..Default::default()
+                    }
+                } else {
+                    EventResponse::default()
+                }


The return value of toggle_stack_for_workspace is being ignored, and then the same operations (unstack_parent_of_selection, apply_stacking_to_parent_of_selection, and visible_windows_in_layout) are being performed again. This duplicates the work already done inside toggle_stack_for_workspace. The line should be changed to return self.toggle_stack_for_workspace(workspace_id, layout, default_orientation); and lines 1562-1594 should be removed.

Suggested change

self.toggle_stack_for_workspace(workspace_id, layout, default_orientation);

let unstacked_windows =

self.workspace_tree_mut(workspace_id)

.unstack_parent_of_selection(layout, default_orientation);

if !unstacked_windows.is_empty() {

return EventResponse {

raise_windows: unstacked_windows,

focus_window: None,

..Default::default()

};

}

let stacked_windows =

self.workspace_tree_mut(workspace_id)

.apply_stacking_to_parent_of_selection(layout, default_orientation);

if !stacked_windows.is_empty() {

return EventResponse {

raise_windows: stacked_windows,

focus_window: None,

..Default::default()

};

}

let visible_windows = self.workspace_tree(workspace_id).visible_windows_in_layout(layout);

if !visible_windows.is_empty() {

EventResponse {

raise_windows: visible_windows,

focus_window: None,

..Default::default()

}

} else {

EventResponse::default()

}

return self.toggle_stack_for_workspace(workspace_id, layout, default_orientation);

Copilot · 2026-02-19T05:09:30Z

src/actor/reactor.rs

+                            let hidden_rect = self
+                                .layout_manager
+                                .layout_engine
+                                .virtual_workspace_manager
+                                .calculate_hidden_position(
+                                    screen.frame,
+                                    0,
+                                    w.frame_monotonic.size,
+                                    HideCorner::BottomRight,
+                                    bundle_id,
+                                );


Direct field access to virtual_workspace_manager breaks encapsulation. The code should use the existing accessor method virtual_workspace_manager() instead (e.g., self.layout_manager.layout_engine.virtual_workspace_manager().calculate_hidden_position(...)). This would allow keeping the field private in LayoutEngine, maintaining better encapsulation.

Copilot · 2026-02-19T05:09:31Z

src/layout_engine/engine.rs

    #[serde(skip)]
    focused_window: Option<WindowId>,
-    virtual_workspace_manager: VirtualWorkspaceManager,
+    pub virtual_workspace_manager: VirtualWorkspaceManager,


The virtual_workspace_manager field should remain private. If the direct field access at src/actor/reactor.rs:2124 is changed to use the accessor method virtual_workspace_manager(), this field can stay private, maintaining better encapsulation.

Suggested change

pub virtual_workspace_manager: VirtualWorkspaceManager,

virtual_workspace_manager: VirtualWorkspaceManager,

Copilot · 2026-02-19T05:09:31Z

src/model/virtual_workspace.rs

@@ -1122,9 +1141,28 @@ impl VirtualWorkspaceManager {
                } else {
                    self.window_rule_floating.remove(&(space, window_id));
                }
+
+                let scratchpad_name = match &rule.scratchpad {
+                    crate::common::config::ScratchpadConfig::Boolean(true) => Some("_default_".to_string()),
+                    crate::common::config::ScratchpadConfig::Boolean(false) => None,
+                    crate::common::config::ScratchpadConfig::Named(n) => Some(n.clone()),
+                };
+
+                if let Some(ref name) = scratchpad_name {
+                tracing::info!(
+                    "Assigning window {:?} to scratchpad {:?} based on app rule",
+                    window_id,
+                    scratchpad_name
+                );
+                    self.window_rule_scratchpad.insert((space, window_id), name.clone());
+                } else {
+                    self.window_rule_scratchpad.remove(&(space, window_id));
+                }


The scratchpad name conversion logic (lines 1118-1122 and 1145-1149) is duplicated. Consider extracting this into a helper method to reduce code duplication and make maintenance easier.

Copilot · 2026-02-19T05:09:31Z

src/layout_engine/engine.rs

+                            Self::toggle_orientation_for_system(s, layout, default_orientation);
+                            if s.parent_of_selection_is_stacked(layout) {
+                                let toggled_windows = s
+                                    .apply_stacking_to_parent_of_selection(layout, default_orientation);
+                                if !toggled_windows.is_empty() {
+                                    return EventResponse {
+                                        raise_windows: toggled_windows,
+                                        focus_window: None,
+                                        ..Default::default()
+                                    };
+                                }
+                            } else {
+                                s.toggle_tile_orientation(layout);
+                            }


For the Traditional layout system case, toggle_orientation_for_system is called on line 1609, but then the same logic from that method is duplicated in lines 1610-1622. This causes the operation to be performed twice. The duplicate logic should be removed, and the result of toggle_orientation_for_system should be returned directly, similar to how it's handled for Bsp, MasterStack, and Scrolling layout systems (lines 1629, 1636, 1643).

Suggested change

Self::toggle_orientation_for_system(s, layout, default_orientation);

if s.parent_of_selection_is_stacked(layout) {

let toggled_windows = s

.apply_stacking_to_parent_of_selection(layout, default_orientation);

if !toggled_windows.is_empty() {

return EventResponse {

raise_windows: toggled_windows,

focus_window: None,

..Default::default()

};

}

} else {

s.toggle_tile_orientation(layout);

}

return Self::toggle_orientation_for_system(s, layout, default_orientation);

Copilot · 2026-02-19T05:09:31Z

src/layout_engine/engine.rs

+            }
+            LayoutCommand::ToggleScratchpad => self.handle_toggle_scratchpad(space, None),
+            LayoutCommand::ToggleScratchpadNamed(name) => {
+                println!("ToggleScratchpadNamed: {}", name);


Debug print statement left in production code. This should be removed or converted to a proper logging statement using the tracing crate (as done elsewhere in the codebase).

Suggested change

println!("ToggleScratchpadNamed: {}", name);

info!("ToggleScratchpadNamed: {}", name);

Copilot · 2026-02-19T05:09:32Z

src/model/virtual_workspace.rs

+                tracing::info!(
+                    "Assigning window {:?} to scratchpad {:?} based on app rule",
+                    window_id,
+                    scratchpad_name
+                );


Inconsistent indentation: the tracing::info! block (lines 1152-1156) is indented inside the if let Some(ref name) block starting at line 1151, but line 1157 which is also part of the same if-block has different indentation. The entire block from lines 1152-1157 should have consistent indentation.

Suggested change

tracing::info!(

"Assigning window {:?} to scratchpad {:?} based on app rule",

window_id,

scratchpad_name

);

tracing::info!(

"Assigning window {:?} to scratchpad {:?} based on app rule",

window_id,

scratchpad_name

);

auscyber force-pushed the main branch from de90b44 to 52013a4 Compare January 19, 2026 13:02

fix i think

a2b494e

auscyber force-pushed the main branch from 52013a4 to a2b494e Compare February 19, 2026 04:51

add tests

e8daaee

auscyber marked this pull request as ready for review February 19, 2026 05:03

Copilot AI review requested due to automatic review settings February 19, 2026 05:03

Copilot started reviewing on behalf of auscyber February 19, 2026 05:03 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add scratchpads#233

add scratchpads#233
auscyber wants to merge 2 commits intoacsandmann:mainfrom
auscyber:main

auscyber commented Jan 19, 2026

Uh oh!

acsandmann commented Jan 20, 2026

Uh oh!

auscyber commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub virtual_workspace_manager: VirtualWorkspaceManager,
	virtual_workspace_manager: VirtualWorkspaceManager,

	println!("ToggleScratchpadNamed: {}", name);
	info!("ToggleScratchpadNamed: {}", name);

Uh oh!

Conversation

auscyber commented Jan 19, 2026

Uh oh!

acsandmann commented Jan 20, 2026

Uh oh!

auscyber commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants