From c617ae26a25a6e04807bf13876a850157fe69218 Mon Sep 17 00:00:00 2001 From: bbedward Date: Sun, 7 Dec 2025 22:39:43 -0500 Subject: [PATCH] niri: fix some keybind tab issues - Fix args for screenshot - move-column stuff is focus=true by default - Parsing fixes part of #914 --- core/internal/keybinds/providers/niri.go | 24 ++- .../keybinds/providers/niri_parser.go | 5 + .../keybinds/providers/niri_parser_test.go | 20 ++- quickshell/Common/KeybindActions.js | 87 ++++++---- quickshell/Modules/Settings/KeybindsTab.qml | 2 +- quickshell/Widgets/KeybindItem.qml | 159 ++++++++++-------- 6 files changed, 190 insertions(+), 107 deletions(-) diff --git a/core/internal/keybinds/providers/niri.go b/core/internal/keybinds/providers/niri.go index fbe8b0ed..340f203d 100644 --- a/core/internal/keybinds/providers/niri.go +++ b/core/internal/keybinds/providers/niri.go @@ -371,6 +371,18 @@ func (n *NiriProvider) buildActionNode(action string) *document.Node { node.SetName(parts[0]) for _, arg := range parts[1:] { + if strings.Contains(arg, "=") { + kv := strings.SplitN(arg, "=", 2) + switch kv[1] { + case "true": + node.AddProperty(kv[0], true, "") + case "false": + node.AddProperty(kv[0], false, "") + default: + node.AddProperty(kv[0], kv[1], "") + } + continue + } node.AddArgument(arg, "") } return node @@ -379,7 +391,7 @@ func (n *NiriProvider) buildActionNode(action string) *document.Node { func (n *NiriProvider) parseActionParts(action string) []string { var parts []string var current strings.Builder - var inQuote, escaped bool + var inQuote, escaped, wasQuoted bool for _, r := range action { switch { @@ -389,17 +401,19 @@ func (n *NiriProvider) parseActionParts(action string) []string { case r == '\\': escaped = true case r == '"': + wasQuoted = true inQuote = !inQuote case r == ' ' && !inQuote: - if current.Len() > 0 { + if current.Len() > 0 || wasQuoted { parts = append(parts, current.String()) current.Reset() + wasQuoted = false } default: current.WriteRune(r) } } - if current.Len() > 0 { + if current.Len() > 0 || wasQuoted { parts = append(parts, current.String()) } return parts @@ -508,6 +522,10 @@ func (n *NiriProvider) writeBindNode(sb *strings.Builder, bind *overrideBind, in sb.WriteString(" ") n.writeArg(sb, arg.ValueString(), forceQuote) } + if child.Properties.Exist() { + sb.WriteString(" ") + sb.WriteString(strings.TrimLeft(child.Properties.String(), " ")) + } } sb.WriteString("; }\n") } diff --git a/core/internal/keybinds/providers/niri_parser.go b/core/internal/keybinds/providers/niri_parser.go index 988f5c17..52d8ad10 100644 --- a/core/internal/keybinds/providers/niri_parser.go +++ b/core/internal/keybinds/providers/niri_parser.go @@ -265,6 +265,11 @@ func (p *NiriParser) parseKeybindNode(node *document.Node, _ string) *NiriKeyBin for _, arg := range actionNode.Arguments { args = append(args, arg.ValueString()) } + if actionNode.Properties != nil { + if val, ok := actionNode.Properties.Get("focus"); ok { + args = append(args, "focus="+val.String()) + } + } } var description string diff --git a/core/internal/keybinds/providers/niri_parser_test.go b/core/internal/keybinds/providers/niri_parser_test.go index dfd715f4..937017b0 100644 --- a/core/internal/keybinds/providers/niri_parser_test.go +++ b/core/internal/keybinds/providers/niri_parser_test.go @@ -602,8 +602,24 @@ func TestNiriParseActionWithProperties(t *testing.T) { for _, kb := range result.Section.Keybinds { switch kb.Action { case "move-column-to-workspace": - if len(kb.Args) != 1 { - t.Errorf("move-column-to-workspace should have 1 arg, got %d", len(kb.Args)) + if len(kb.Args) != 2 { + t.Errorf("move-column-to-workspace should have 2 args (index + focus), got %d", len(kb.Args)) + } + hasIndex := false + hasFocus := false + for _, arg := range kb.Args { + if arg == "1" || arg == "2" { + hasIndex = true + } + if arg == "focus=false" { + hasFocus = true + } + } + if !hasIndex { + t.Errorf("move-column-to-workspace missing index arg") + } + if !hasFocus { + t.Errorf("move-column-to-workspace missing focus=false arg") } case "next-window": if kb.Key != "Tab" { diff --git a/quickshell/Common/KeybindActions.js b/quickshell/Common/KeybindActions.js index 7b97acc9..d2731061 100644 --- a/quickshell/Common/KeybindActions.js +++ b/quickshell/Common/KeybindActions.js @@ -197,14 +197,26 @@ const ACTION_ARGS = { { name: "focus", type: "bool", label: "Follow focus", default: false } ] }, + "move-column-to-workspace-down": { + args: [{ name: "focus", type: "bool", label: "Follow focus", default: false }] + }, + "move-column-to-workspace-up": { + args: [{ name: "focus", type: "bool", label: "Follow focus", default: false }] + }, "screenshot": { - args: [{ name: "opts", type: "screenshot", label: "Options" }] + args: [{ name: "show-pointer", type: "bool", label: "Show pointer" }] }, "screenshot-screen": { - args: [{ name: "opts", type: "screenshot", label: "Options" }] + args: [ + { name: "show-pointer", type: "bool", label: "Show pointer" }, + { name: "write-to-disk", type: "bool", label: "Save to disk" } + ] }, "screenshot-window": { - args: [{ name: "opts", type: "screenshot", label: "Options" }] + args: [ + { name: "show-pointer", type: "bool", label: "Show pointer" }, + { name: "write-to-disk", type: "bool", label: "Save to disk" } + ] } }; @@ -288,11 +300,12 @@ function getActionLabel(action) { if (!action) return ""; - const dmsAct = findDmsAction(action); + var dmsAct = findDmsAction(action); if (dmsAct) return dmsAct.label; - const compAct = findCompositorAction(action); + var base = action.split(" ")[0]; + var compAct = findCompositorAction(base); if (compAct) return compAct.label; @@ -337,7 +350,8 @@ function isValidAction(action) { function isKnownCompositorAction(action) { if (!action) return false; - return findCompositorAction(action) !== null; + var base = action.split(" ")[0]; + return findCompositorAction(base) !== null; } function buildSpawnAction(command, args) { @@ -404,10 +418,10 @@ function parseCompositorActionArgs(action) { if (!ACTION_ARGS[base]) return { base: action, args: {} }; - var argConfig = ACTION_ARGS[base]; var argParts = parts.slice(1); - if (base === "move-column-to-workspace") { + switch (base) { + case "move-column-to-workspace": for (var i = 0; i < argParts.length; i++) { if (argParts[i] === "focus=true" || argParts[i] === "focus=false") { args.focus = argParts[i] === "focus=true"; @@ -415,14 +429,24 @@ function parseCompositorActionArgs(action) { args.index = argParts[i]; } } - } else if (base.startsWith("screenshot")) { - args.opts = {}; - for (var j = 0; j < argParts.length; j += 2) { - if (j + 1 < argParts.length) - args.opts[argParts[j]] = argParts[j + 1]; + break; + case "move-column-to-workspace-down": + case "move-column-to-workspace-up": + for (var k = 0; k < argParts.length; k++) { + if (argParts[k] === "focus=true" || argParts[k] === "focus=false") + args.focus = argParts[k] === "focus=true"; + } + break; + default: + if (base.startsWith("screenshot")) { + for (var j = 0; j < argParts.length; j++) { + var kv = argParts[j].split("="); + if (kv.length === 2) + args[kv[0]] = kv[1] === "true"; + } + } else if (argParts.length > 0) { + args.value = argParts.join(" "); } - } else if (argParts.length > 0) { - args.value = argParts.join(" "); } return { base: base, args: args }; @@ -437,24 +461,29 @@ function buildCompositorAction(base, args) { if (!args || Object.keys(args).length === 0) return base; - if (base === "move-column-to-workspace") { + switch (base) { + case "move-column-to-workspace": if (args.index) parts.push(args.index); - if (args.focus === true) - parts.push("focus=true"); - else if (args.focus === false) + if (args.focus === false) parts.push("focus=false"); - } else if (base.startsWith("screenshot") && args.opts) { - for (var key in args.opts) { - if (args.opts[key] !== undefined && args.opts[key] !== "") { - parts.push(key); - parts.push(args.opts[key]); - } + break; + case "move-column-to-workspace-down": + case "move-column-to-workspace-up": + if (args.focus === false) + parts.push("focus=false"); + break; + default: + if (base.startsWith("screenshot")) { + if (args["show-pointer"] === true) + parts.push("show-pointer=true"); + if (args["write-to-disk"] === true) + parts.push("write-to-disk=true"); + } else if (args.value) { + parts.push(args.value); + } else if (args.index) { + parts.push(args.index); } - } else if (args.value) { - parts.push(args.value); - } else if (args.index) { - parts.push(args.index); } return parts.join(" "); diff --git a/quickshell/Modules/Settings/KeybindsTab.qml b/quickshell/Modules/Settings/KeybindsTab.qml index 7d261618..01f9163c 100644 --- a/quickshell/Modules/Settings/KeybindsTab.qml +++ b/quickshell/Modules/Settings/KeybindsTab.qml @@ -597,7 +597,7 @@ Item { onSaveBind: (originalKey, newData) => { KeybindsService.saveBind(originalKey, newData); keybindsTab._editingKey = newData.key; - keybindsTab.expandedKey = modelData.action; + keybindsTab.expandedKey = newData.action; } onRemoveBind: key => { const remainingKey = bindItem.keys.find(k => k.key !== key)?.key ?? ""; diff --git a/quickshell/Widgets/KeybindItem.qml b/quickshell/Widgets/KeybindItem.qml index 64b7e09c..b759b75f 100644 --- a/quickshell/Widgets/KeybindItem.qml +++ b/quickshell/Widgets/KeybindItem.qml @@ -650,9 +650,10 @@ Item { } onWheel: wheel => { - if (!root.recording) + if (!root.recording) { + wheel.accepted = false; return; - + } wheel.accepted = true; const mods = []; @@ -959,12 +960,12 @@ Item { Layout.preferredWidth: 120 compactMode: true currentValue: { - const action = root.editAction; + const base = root.editAction.split(" ")[0]; const cats = KeybindsService.getCompositorCategories(); for (const cat of cats) { const actions = KeybindsService.getCompositorActions(cat); for (const act of actions) { - if (act.id === action) + if (act.id === base) return cat; } } @@ -1024,12 +1025,13 @@ Item { } RowLayout { + id: optionsRow Layout.fillWidth: true spacing: Theme.spacingM visible: root._actionType === "compositor" && !root.useCustomCompositor && Actions.getActionArgConfig(root.editAction) - property var argConfig: Actions.getActionArgConfig(root.editAction) - property var parsedArgs: Actions.parseCompositorActionArgs(root.editAction) + readonly property var argConfig: Actions.getActionArgConfig(root.editAction) + readonly property var parsedArgs: Actions.parseCompositorActionArgs(root.editAction) StyledText { text: I18n.tr("Options") @@ -1048,56 +1050,75 @@ Item { Layout.fillWidth: true Layout.preferredHeight: 40 visible: { - const cfg = parent.parent.argConfig; - if (!cfg || !cfg.config || !cfg.config.args) + const cfg = optionsRow.argConfig; + if (!cfg?.config?.args) return false; const firstArg = cfg.config.args[0]; return firstArg && (firstArg.type === "text" || firstArg.type === "number"); } - placeholderText: { - const cfg = parent.parent.argConfig; - if (!cfg || !cfg.config || !cfg.config.args) - return ""; - return cfg.config.args[0]?.placeholder || ""; + placeholderText: optionsRow.argConfig?.config?.args?.[0]?.placeholder || "" + + Connections { + target: optionsRow + function onParsedArgsChanged() { + const newText = optionsRow.parsedArgs?.args?.value || optionsRow.parsedArgs?.args?.index || ""; + if (argValueField.text !== newText) + argValueField.text = newText; + } } - text: parent.parent.parsedArgs?.args?.value || parent.parent.parsedArgs?.args?.index || "" - onTextChanged: { - const cfg = parent.parent.argConfig; + + Component.onCompleted: { + text = optionsRow.parsedArgs?.args?.value || optionsRow.parsedArgs?.args?.index || ""; + } + + onEditingFinished: { + const cfg = optionsRow.argConfig; if (!cfg) return; - const base = parent.parent.parsedArgs?.base || root.editAction.split(" ")[0]; - const args = cfg.config.args[0]?.type === "number" ? { - index: text - } : { - value: text - }; + const parsed = optionsRow.parsedArgs; + const args = {}; + if (cfg.config.args[0]?.type === "number") + args.index = text; + else + args.value = text; + if (parsed?.args?.focus === false) + args.focus = false; root.updateEdit({ - action: Actions.buildCompositorAction(base, args) + action: Actions.buildCompositorAction(parsed?.base || cfg.base, args) }); } } RowLayout { visible: { - const cfg = parent.parent.argConfig; - return cfg && cfg.base === "move-column-to-workspace"; + const cfg = optionsRow.argConfig; + if (!cfg) + return false; + switch (cfg.base) { + case "move-column-to-workspace": + case "move-column-to-workspace-down": + case "move-column-to-workspace-up": + return true; + } + return false; } spacing: Theme.spacingXS DankToggle { id: focusToggle - checked: parent.parent.parent.parsedArgs?.args?.focus === true - onCheckedChanged: { - const cfg = parent.parent.parent.argConfig; + checked: optionsRow.parsedArgs?.args?.focus !== false + onToggled: newChecked => { + const cfg = optionsRow.argConfig; if (!cfg) return; - const parsed = parent.parent.parent.parsedArgs; - const args = { - index: parsed?.args?.index || "", - focus: checked - }; + const parsed = optionsRow.parsedArgs; + const args = {}; + if (cfg.base === "move-column-to-workspace") + args.index = parsed?.args?.index || ""; + if (!newChecked) + args.focus = false; root.updateEdit({ - action: Actions.buildCompositorAction("move-column-to-workspace", args) + action: Actions.buildCompositorAction(cfg.base, args) }); } } @@ -1110,53 +1131,22 @@ Item { } RowLayout { - visible: { - const cfg = parent.parent.argConfig; - return cfg && cfg.base && cfg.base.startsWith("screenshot"); - } + visible: optionsRow.argConfig?.base?.startsWith("screenshot") ?? false spacing: Theme.spacingM - RowLayout { - spacing: Theme.spacingXS - - DankToggle { - id: writeToDiskToggle - checked: parent.parent.parent.parent.parsedArgs?.args?.opts?.["write-to-disk"] === "true" - onCheckedChanged: { - const parsed = parent.parent.parent.parent.parsedArgs; - const base = parsed?.base || "screenshot"; - const opts = parsed?.args?.opts || {}; - opts["write-to-disk"] = checked ? "true" : ""; - root.updateEdit({ - action: Actions.buildCompositorAction(base, { - opts: opts - }) - }); - } - } - - StyledText { - text: I18n.tr("Save") - font.pixelSize: Theme.fontSizeSmall - color: Theme.surfaceVariantText - } - } - RowLayout { spacing: Theme.spacingXS DankToggle { id: showPointerToggle - checked: parent.parent.parent.parent.parsedArgs?.args?.opts?.["show-pointer"] === "true" - onCheckedChanged: { - const parsed = parent.parent.parent.parent.parsedArgs; + checked: optionsRow.parsedArgs?.args?.["show-pointer"] === true + onToggled: newChecked => { + const parsed = optionsRow.parsedArgs; const base = parsed?.base || "screenshot"; - const opts = parsed?.args?.opts || {}; - opts["show-pointer"] = checked ? "true" : ""; + const args = Object.assign({}, parsed?.args || {}); + args["show-pointer"] = newChecked; root.updateEdit({ - action: Actions.buildCompositorAction(base, { - opts: opts - }) + action: Actions.buildCompositorAction(base, args) }); } } @@ -1167,6 +1157,31 @@ Item { color: Theme.surfaceVariantText } } + + RowLayout { + visible: optionsRow.argConfig?.base !== "screenshot" + spacing: Theme.spacingXS + + DankToggle { + id: writeToDiskToggle + checked: optionsRow.parsedArgs?.args?.["write-to-disk"] === true + onToggled: newChecked => { + const parsed = optionsRow.parsedArgs; + const base = parsed?.base || "screenshot-screen"; + const args = Object.assign({}, parsed?.args || {}); + args["write-to-disk"] = newChecked; + root.updateEdit({ + action: Actions.buildCompositorAction(base, args) + }); + } + } + + StyledText { + text: I18n.tr("Save") + font.pixelSize: Theme.fontSizeSmall + color: Theme.surfaceVariantText + } + } } } }