Repository navigation
Fix Cmd-W for Task Manager and auxiliary windows #3734
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
Merged
+255
−0
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
edf7d46
Add auxiliary window close shortcut lint
lawrencecchen 856f65a
Register auxiliary windows for Cmd-W close
lawrencecchen 066f398
Merge remote-tracking branch 'origin/main' into task-fix-task-manager…
lawrencecchen bd617d8
Harden auxiliary window lint parser
lawrencecchen 0594219
Cover lint comment and multiline cases
lawrencecchen b12b6f8
Strip comments before owner list parsing
lawrencecchen c150a48
Ignore block comments in window lint
lawrencecchen d844e04
Preserve window lint line numbers
lawrencecchen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| #!/usr/bin/env python3 | ||
| """Require standalone cmux windows to own the standard close shortcut.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import pathlib | ||
| import re | ||
| import sys | ||
|
|
||
|
|
||
| DEFAULT_ROOTS = ("Sources",) | ||
| OWNER_LIST_PATH = pathlib.Path("Sources/cmuxApp.swift") | ||
| OWNER_LIST_NAME = "cmuxAuxiliaryWindowIdentifiers" | ||
|
|
||
| # Hidden/internal bootstrap windows should not take Cmd+W away from the active | ||
| # main window. Add to this set only when a window is intentionally not user | ||
| # closable. | ||
| IGNORED_IDENTIFIERS = { | ||
| "cmux.bootstrap", | ||
| } | ||
|
|
||
| IDENTIFIER_ASSIGNMENT_RE = re.compile( | ||
| r"""\b[A-Za-z_][A-Za-z0-9_]*\.identifier\s*=\s*NSUserInterfaceItemIdentifier\("(?P<identifier>cmux\.[^"]+)"\)""" | ||
| ) | ||
| STRING_LITERAL_RE = re.compile(r'"(?P<identifier>cmux\.[^"]+)"') | ||
| BLOCK_COMMENT_RE = re.compile(r"/\*.*?\*/", re.DOTALL) | ||
| LINE_COMMENT_RE = re.compile(r"//[^\n]*") | ||
|
|
||
|
|
||
| def strip_line_comments(text: str) -> str: | ||
| text = BLOCK_COMMENT_RE.sub(lambda match: "\n" * match.group(0).count("\n"), text) | ||
| return LINE_COMMENT_RE.sub("", text) | ||
|
lawrencecchen marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def load_close_owner_identifiers(repo_root: pathlib.Path) -> set[str]: | ||
| path = repo_root / OWNER_LIST_PATH | ||
| try: | ||
| text = path.read_text(encoding="utf-8") | ||
| except FileNotFoundError: | ||
| raise ValueError(f"missing {OWNER_LIST_PATH}") from None | ||
|
|
||
| parse_text = strip_line_comments(text) | ||
| marker = f"private let {OWNER_LIST_NAME}" | ||
| marker_index = parse_text.find(marker) | ||
| if marker_index < 0: | ||
| raise ValueError(f"missing {OWNER_LIST_NAME} in {OWNER_LIST_PATH}") | ||
|
|
||
| list_start = parse_text.find("[", marker_index) | ||
| if list_start < 0: | ||
| raise ValueError(f"could not parse {OWNER_LIST_NAME} in {OWNER_LIST_PATH}") | ||
|
|
||
| depth = 0 | ||
| list_end = -1 | ||
| for index in range(list_start, len(parse_text)): | ||
| if parse_text[index] == "[": | ||
| depth += 1 | ||
| elif parse_text[index] == "]": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| list_end = index | ||
| break | ||
|
lawrencecchen marked this conversation as resolved.
|
||
| if list_end < 0: | ||
| raise ValueError(f"could not parse {OWNER_LIST_NAME} in {OWNER_LIST_PATH}") | ||
|
|
||
| list_body = parse_text[list_start:list_end] | ||
| return {match.group("identifier") for match in STRING_LITERAL_RE.finditer(list_body)} | ||
|
|
||
|
|
||
| def collect_window_identifier_assignments( | ||
| repo_root: pathlib.Path, | ||
|
lawrencecchen marked this conversation as resolved.
|
||
| roots: tuple[str, ...], | ||
| ) -> dict[str, list[str]]: | ||
| assignments: dict[str, list[str]] = {} | ||
| for root in roots: | ||
| root_path = repo_root / root | ||
| if not root_path.exists(): | ||
| continue | ||
| for path in sorted(root_path.rglob("*.swift")): | ||
| rel_path = path.relative_to(repo_root).as_posix() | ||
| text = strip_line_comments(path.read_text(encoding="utf-8", errors="replace")) | ||
| for match in IDENTIFIER_ASSIGNMENT_RE.finditer(text): | ||
| identifier = match.group("identifier") | ||
| line_number = text.count("\n", 0, match.start()) + 1 | ||
| assignments.setdefault(identifier, []).append(f"{rel_path}:{line_number}") | ||
|
lawrencecchen marked this conversation as resolved.
|
||
| return assignments | ||
|
|
||
|
|
||
| def main(argv: list[str]) -> int: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument( | ||
| "--repo-root", | ||
| default=pathlib.Path.cwd(), | ||
| type=pathlib.Path, | ||
| help="repository root to scan", | ||
| ) | ||
| parser.add_argument( | ||
| "--roots", | ||
| nargs="+", | ||
| default=list(DEFAULT_ROOTS), | ||
| help="repo-relative Swift roots to scan", | ||
| ) | ||
| args = parser.parse_args(argv) | ||
|
|
||
| repo_root = args.repo_root.resolve(strict=False) | ||
| try: | ||
| close_owners = load_close_owner_identifiers(repo_root) | ||
| except ValueError as exc: | ||
| print(f"Auxiliary window close-shortcut lint could not run: {exc}", file=sys.stderr) | ||
| return 2 | ||
|
|
||
| assignments = collect_window_identifier_assignments(repo_root, tuple(args.roots)) | ||
| missing = { | ||
| identifier: locations | ||
| for identifier, locations in assignments.items() | ||
| if identifier not in close_owners and identifier not in IGNORED_IDENTIFIERS | ||
| } | ||
|
|
||
| if missing: | ||
| print("Auxiliary window close-shortcut lint failed.") | ||
| print("") | ||
| print( | ||
| "These cmux window identifiers are assigned to NSWindow/NSPanel " | ||
| f"but are missing from {OWNER_LIST_NAME}:" | ||
| ) | ||
| for identifier in sorted(missing): | ||
| print(f"- {identifier}") | ||
| for location in missing[identifier]: | ||
| print(f" {location}") | ||
| print("") | ||
| print( | ||
| f"Add each user-closable window to {OWNER_LIST_NAME} in {OWNER_LIST_PATH}, " | ||
| "or add a documented lint ignore for internal windows that must not own Cmd+W." | ||
| ) | ||
| return 1 | ||
|
|
||
| print("Auxiliary window close-shortcut lint passed.") | ||
| print(f"Checked {len(assignments)} cmux window identifier(s).") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main(sys.argv[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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| cd "$ROOT_DIR" | ||
|
|
||
| python3 scripts/lint_auxiliary_window_close_shortcuts.py | ||
|
|
||
| TMP_DIR="$(mktemp -d)" | ||
| trap 'rm -rf "$TMP_DIR"' EXIT | ||
|
|
||
| mkdir -p "$TMP_DIR/Sources" | ||
|
|
||
| cat > "$TMP_DIR/Sources/cmuxApp.swift" <<'SWIFT' | ||
| private let cmuxAuxiliaryWindowIdentifiers: Set<String> = [ | ||
| "cmux.settings", | ||
| ] | ||
| SWIFT | ||
|
|
||
| cat > "$TMP_DIR/Sources/NewWindow.swift" <<'SWIFT' | ||
| import AppKit | ||
|
|
||
| /* | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.blockCommentOnly") | ||
| */ | ||
|
|
||
| func makeWindow() { | ||
| let window = NSWindow() | ||
| window.identifier = | ||
| NSUserInterfaceItemIdentifier("cmux.newWindow") | ||
| } | ||
| SWIFT | ||
|
|
||
| if python3 scripts/lint_auxiliary_window_close_shortcuts.py --repo-root "$TMP_DIR" >"$TMP_DIR/missing.out" 2>&1; then | ||
| echo "Expected missing auxiliary-window close owner to fail" >&2 | ||
| exit 1 | ||
| fi | ||
| grep -q "cmux.newWindow" "$TMP_DIR/missing.out" | ||
| grep -q "Sources/NewWindow.swift:9" "$TMP_DIR/missing.out" | ||
|
|
||
| cat > "$TMP_DIR/Sources/cmuxApp.swift" <<'SWIFT' | ||
| private let cmuxAuxiliaryWindowIdentifiers: Set<String> = [ | ||
| // "cmux.newWindow", | ||
| /* | ||
| "cmux.newWindow", | ||
| */ | ||
| "cmux.settings", | ||
| ] | ||
| SWIFT | ||
|
|
||
| if python3 scripts/lint_auxiliary_window_close_shortcuts.py --repo-root "$TMP_DIR" >"$TMP_DIR/commented-owner.out" 2>&1; then | ||
| echo "Expected commented-out auxiliary-window close owner to be ignored" >&2 | ||
| exit 1 | ||
| fi | ||
| grep -q "cmux.newWindow" "$TMP_DIR/commented-owner.out" | ||
|
|
||
| cat > "$TMP_DIR/Sources/cmuxApp.swift" <<'SWIFT' | ||
| private let cmuxAuxiliaryWindowIdentifiers: Set<String> = [ | ||
| // MARK: - Main Windows [user-closable] | ||
| // This comment intentionally contains a lone ] bracket. | ||
| "cmux.newWindow", | ||
| "cmux.settings", | ||
| ] | ||
| SWIFT | ||
|
|
||
| python3 scripts/lint_auxiliary_window_close_shortcuts.py --repo-root "$TMP_DIR" | ||
|
|
||
| cat > "$TMP_DIR/Sources/NewWindow.swift" <<'SWIFT' | ||
| import AppKit | ||
|
|
||
| func makeWindow() { | ||
| let window = NSWindow() | ||
| /* | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.blockCommentOnly") | ||
| */ | ||
| // window.identifier = NSUserInterfaceItemIdentifier("cmux.commentOnly") | ||
| _ = window | ||
| } | ||
| SWIFT | ||
|
|
||
| python3 scripts/lint_auxiliary_window_close_shortcuts.py --repo-root "$TMP_DIR" | ||
|
|
||
| cat > "$TMP_DIR/Sources/cmuxApp.swift" <<'SWIFT' | ||
| private let cmuxAuxiliaryWindowIdentifiers: Set<String> = [ | ||
| "cmux.settings", | ||
| ] | ||
| SWIFT | ||
|
|
||
| cat > "$TMP_DIR/Sources/NewWindow.swift" <<'SWIFT' | ||
| import AppKit | ||
|
|
||
| func makeWindow() { | ||
| let window = NSWindow() | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.bootstrap") | ||
| } | ||
| SWIFT | ||
|
|
||
| python3 scripts/lint_auxiliary_window_close_shortcuts.py --repo-root "$TMP_DIR" | ||
|
lawrencecchen marked this conversation as resolved.
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.