Use workspace idx/name and output to restore windows instead of workspace id#2
Open
fmuehlis wants to merge 1 commit into
Open
Use workspace idx/name and output to restore windows instead of workspace id#2fmuehlis wants to merge 1 commit into
fmuehlis wants to merge 1 commit into
Conversation
…t` to restore windows to the correct monitor and workspace
MTeaHead
self-requested a review
August 4, 2025 21:40
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note This review was created by GLM-5.1 served via Z.ai using Crush Solid draft 😃👍 — the approach (idx/name + output instead of workspace ID) is correct and the niri-ipc migration is clean. A few things to address: P0 — Must Fix
P1 — Should Fix
P2 — Minor
|
LarsArtmann
added a commit
to LarsArtmann/niri-session-manager
that referenced
this pull request
Jun 30, 2026
…ta-driven terminal profiles, spawn_blocking, and 27 unit tests This commit addresses all critical issues identified in the code review and aligns the implementation with PR MTeaHead#2 (workspace idx/name): 1. niri-ipc: downgrade from 26.4.0 to 25.11.0 (semver compatible with PR MTeaHead#2's 25.5.1). Both PRs now share the same IPC major version, reducing merge conflict surface for the upstream maintainer. 2. Workspace-aware restore: replace ephemeral workspace_id with workspace_idx, workspace_name, workspace_output (from PR MTeaHead#2's approach). Add MoveWindowToMonitor before MoveWindowToWorkspace. Prefer workspace name over index for named workspaces. Guard against empty workspace names. Old session.json files with workspace_id still deserialize via #[serde(default)]. 3. Data-driven TerminalProfile: replaces the 6-arm TerminalKind enum and 80 lines of duplicated match arms with a 4-field struct (needs_start_subcommand, cwd_flag, cwd_flag_separator, cmd_flag). Each terminal is a const constructor. Adding new terminals is a one-liner. Correct flags for kitty, foot, wezterm, ghostty, alacritty, and a generic fallback. 4. spawn_blocking: /proc reads in proc.rs are synchronous filesystem operations that were blocking the tokio runtime. Wrapped in tokio::task::spawn_blocking via new resolve_terminal_state() helper. 5. Shell injection fix: child_cmd from /proc/<pid>/cmdline is now single-quote escaped via shell_escape() before interpolation into sh -c strings. Handles spaces, semicolons, dollar signs, backticks, and embedded single quotes. 6. PID type safety: SavedWindow.pid changed from Option<i32> to Option<u32>. Conversion from niri_ipc's i32 guards p > 0 at the boundary. 7. Configurable helper_names: is_terminal_helper's hardcoded ["kitten"] list replaced by configurable helper_names in TOML (default: ["kitten"]). Wired through to proc::resolve_child_process. 8. Dead code removed: original_args field, 5 commented-out log lines, unused toml import, dead save_session function consolidated into save_session_with_terminal_state. 9. 27 unit tests covering: shell_escape (7 tests), TerminalProfile variants (6 tests), build_terminal_restore_command per terminal (6 tests), build_spawn_command (2 tests), SavedWindow backward compatibility deserialization (3 tests), resolve_executable_name, config defaults. 10. README updated with correct per-terminal flag documentation. Build: nix build clean. Tests: 27/27 pass. 💘 Generated with Crush Assisted-by: Crush:glm-5.1
LarsArtmann
added a commit
to LarsArtmann/niri-session-manager
that referenced
this pull request
Jun 30, 2026
Comprehensive status report covering the full branch state after three rounds of review and hardening. Documents: all completed work (IPC convergence with PR MTeaHead#2, workspace-aware restore, data-driven terminal profiles, spawn_blocking, 27 unit tests, shell injection fix), partially done items (runtime testing, error handling), known gaps (proc.rs integration tests, workspace race condition), and a prioritized top-25 next-steps list. Includes the key open question about merge strategy coordination with PR MTeaHead#2. 💘 Generated with Crush Assisted-by: Crush:glm-5.1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi! I really like the idea of this project and I have been using it for a few weeks now. However, I noticed that windows will not always be spawned on the correct workspace, but seemingly on random workspaces. I've read through the code and saw that windows currently get restored by workspace id. Sadly workspace IDs are not really consistent and the same workspace index on the same output can have different IDs when created and deleted multiple times.
So I propose this solution to recreate windows per monitor and workspace. The current solution prefers the workspace name over the workspace index on a given monitor because it is possible for named workspaces to have different indices too (e.g. there can be workspace 1 before a named workspace shifting the index of each workspace). To get the
MoveWindowToMonitorfunctionality I had to bump theniri-ipcdependency version.This is only supposed to be a draft and a proof of concept, as I would like to hear your thoughts on it hence the two TODOs in the code. If this is something you would be interested in I will refine this PR and test it more thoroughly.