Repository navigation
Conversation
🦊 mika's reviewIncremental review: new commits since the last checkpoint. Summary
Change diagramflowchart LR
Settings["Identity settings / CLI"] --> Theme["Persisted QS Bar setting"]
WM["WM workspace and output state"] --> Widget["Workspace widget"]
WM --> Panel["Workspace popup"]
Theme --> Widget
Theme --> Panel
Helper["Workspace isolation helper"] --> Widget
Estimated review effort: 2/5 · risk: medium 📁 Walkthrough details
|
| Severity | Category | Fix effort | File | Issue |
|---|---|---|---|---|
| 🟡 minor | correctness | medium | ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/modules/WorkspaceWidget.qml |
Possible Bug — With isolation enabled, fixed modes depend on every Wm.workspaces update, and workspaceList() returns a new array even when the displayed IDs have not changed. Focus or occupancy updates can therefore reset the repea… |
| 🟢 trivial | maintainability | low | ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/Theme.qml |
Maintainability — The new comment contains en-dash characters, which the repository's mechanical rules prohibit. Use ASCII punctuation instead. |
| 🟢 trivial | maintainability | low | ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/controlcenter/routes/IdentityRoute.qml |
Maintainability — The new comment contains en-dash characters, which the repository's mechanical rules prohibit. Use ASCII punctuation instead. |
⚡ Quick fixes by effort
Low effort (apply from the diff):
ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/Theme.qml:2345 — Maintainability — The new comment contains en-dash characters, which the repository's mechanical rules prohibit. Use ASCII punctuation instead.ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/controlcenter/routes/IdentityRoute.qml:307 — Maintainability — The new comment contains en-dash characters, which the repository's mechanical rules prohibit. Use ASCII punctuation instead.
Medium effort (a function-level change):
ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/modules/WorkspaceWidget.qml:41 — Possible Bug — With isolation enabled, fixed modes depend on everyWm.workspacesupdate, andworkspaceList()returns a new array even when the displayed IDs have not changed. Focus or occupancy updates can therefore reset the repea…
🤖 Prompt for AI agents (click, copy, paste into Claude/Codex)
Fix the following issues found by mika-agent on Ryoku-dev/ryoku#770 (branch feat/qsbar-workspace-isolate-monitors):
- ryoku/shell/quickshell/shell/modules/bar/barstyles/qsbar/modules/WorkspaceWidget.qml:41-45 — Possible Bug (minor severity, medium effort): With isolation enabled, fixed modes depend on every `Wm.workspaces` update, and `workspaceList()` returns a new array even when the displayed IDs have not changed. Focus or occupancy updates can therefore reset the repea…
For each: verify it is real in the current code, apply the smallest correct fix, and run the repo's tests. Skip anything already resolved and say why.
🛠 Deterministic checks run
- golangci-lint: not installed (skip)
🚦 Merge recommendation: needs_review
🦊 Generated by Mika — the Ryoku org's self-hosted AI code reviewer. Machine findings: verify before merging.
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| title: I18n.tr("ISOLATE") | ||
| kana: "\u5206" | ||
|
|
||
| SettingRow { | ||
| anchors.left: parent.left | ||
| anchors.right: parent.right | ||
| label: I18n.tr("Per-monitor") | ||
| desc: I18n.tr("Each bar shows only the workspaces on its own screen.") |
There was a problem hiding this comment.
🟡 minor · Data Integrity · fix: medium effort
The new translation keys are not added to the source and English catalogs in this diff. The new settings card therefore lacks catalog entries for its title, label, and description, breaking translated-UI integrity.
There was a problem hiding this comment.
Fixed. Added missing English catalog entries for "Per-monitor" and "Each bar shows only the workspaces on its own screen." in ryoku/i18n/catalog/en.json.
| if (!ws[i].special && ws[i].active && ws[i].output === wsWidget.screenName) | ||
| return ws[i] | ||
| } | ||
| return Wm.focusedWorkspace |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: low effort
If no active workspace is found for a screen, this falls back to the globally focused workspace, which can belong to another screen. In fixed mode an out-of-range global workspace is then appended to that screen's list and highlighted, breaking isolation; return no local active workspace instead.
Suggested change
| return Wm.focusedWorkspace | |
| return null |
There was a problem hiding this comment.
Fixed. Returned null instead of falling back to Wm.focusedWorkspace when isolateMonitors is active but no monitor-active workspace is found.
| continue | ||
| res.push(vs[i]) | ||
| } | ||
| res.sort(function(x, y) { return Number(x.name) - Number(y.name) }) |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: medium effort
The popup now sorts its visible workspaces even when isolation is disabled, whereas previously it displayed Wm.workspaces in provider order. If the provider returns a non-numeric order, opening the popup changes the displayed order despite isolation being off; preserve the original order in that mode.
There was a problem hiding this comment.
Fixed. Only sort filteredWorkspaces when isolateMonitors is enabled, preserving original provider order when disabled.
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| readonly property var wsIds: { | ||
| var a = [] | ||
| var vs = Wm.workspaces | ||
| for (var i = 0; i < vs.length; i++) if (!vs[i].special) a.push(vs[i].name) | ||
| a.sort(function(x, y) { return Number(x) - Number(y) }) | ||
| var vs = card.filteredWorkspaces | ||
| for (var i = 0; i < vs.length; i++) a.push(vs[i].name) | ||
| if (!root.isolateMonitors) { | ||
| a.sort(function(x, y) { return Number(x) - Number(y) }) | ||
| } |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: low effort
When isolation is off, the repeater preserves Wm.workspaces order but wsIds is sorted separately. If the compositor returns workspaces out of numeric order, keyboard navigation moves through them in a different order from the displayed rows. Use the same ordered model for both display and navigation.
…cePanel Always sort filteredWorkspaces numerically (not only when isolateMonitors is on) so the repeater and wsIds are always in the same order. wsIds is now derived directly from filteredWorkspaces without an independent sort, which fixes keyboard navigation when the compositor returns workspaces out of numeric order and isolation is off. Addresses mika review finding on PR Ryoku-dev#770.
|
Fixed in b753286. The root cause: Fix applied:
This guarantees the two arrays are always aligned, so keyboard Up/Down always selects the visually highlighted row. |
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| // TestBarWorkspacesIsolateMonitors verifies that the isolateMonitors toggle | ||
| // is accepted by the bar-set verb and round-trips through the settings store. | ||
| func TestBarWorkspacesIsolateMonitors(t *testing.T) { |
There was a problem hiding this comment.
🟡 minor · Tests · fix: medium effort
The new test only checks persistence through the CLI, leaving the workspace filtering and per-monitor active marker untested. Add coverage for multiple outputs, including uncreated fixed-mode workspaces and focus changes.
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| readonly property var workspaceList: WorkspaceIsolation.workspaceList( | ||
| Wm.workspaces, root.workspaceMode, wsWidget.dynamicModel, wsWidget.screenName, | ||
| root.isolateMonitors, root.isolateMonitors ? wsWidget.monitorActiveWs : Wm.focusedWorkspace) |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: medium effort
In fixed workspace modes, this binding now depends directly on Wm.focusedWorkspace and returns a new array on every focus change, even when the displayed workspace IDs are unchanged. Replacing the Repeater model recreates its delegates, interrupting their focus animations; the previous extraWs property avoided model changes for in-range switches. Keep the model stable when its IDs have not changed.
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| readonly property var workspaceList: WorkspaceIsolation.workspaceList( | ||
| Wm.workspaces, root.workspaceMode, wsWidget.dynamicModel, wsWidget.screenName, | ||
| root.isolateMonitors, wsWidget.listActiveWorkspace, wsWidget.extraWs) |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: medium effort
In fixed workspace modes, workspaceList now depends on Wm.workspaces even when isolation is disabled. A workspace update therefore produces a new array and resets the Repeater model, recreating delegates and interrupting their focus animations; the previous fixed model stayed stable for in-range focus changes. Avoid depending on the live workspace list in fixed mode when isolation is off.
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
|
|
||
|
|
||
| // ── PREVIEW ── | ||
| // ── workspaces: ISOLATE ── |
There was a problem hiding this comment.
🟢 trivial · Maintainability · fix: low effort
The new comment contains en-dash characters, which the repository's mechanical rules prohibit. Use ASCII punctuation instead.
Suggested change
| // ── workspaces: ISOLATE ── | |
| // Workspaces: ISOLATE |
Reply here to discuss, say it's fixed, or explain why it's intended — no mention needed.
What changed
Implements per-monitor workspace isolation (
isolateMonitors) forqsbarworkspaces, addressing #767.isolateMonitorsboolean setting (defaultfalse) underqsbar.isolateMonitorsinshell.json.widgets.jsonunderworkspaces, enabling CLI control viaryoku-shell bar set workspaces isolateMonitors on/off.isolateMonitorsenabled:WorkspacePanelpopup also filters by screen when enabled.isolateMonitors: falseretain 100% identical behavior.Fixes #767
Area
ryoku
How it was tested
ryoku/shell/ipc/bar_test.go(TestBarWorkspacesIsolateMonitors) verifying CLI toggling and round-tripping through the settings store (go test ./...inshell/ipc).Theme.qml,BarSlot.qml,WorkspaceWidget.qml,WorkspacePanel.qml,IdentityRoute.qml) withqmllint(0 errors).Checklist
[area] scope: summarycommit subject.CHANGELOG.mdupdated in the area I touched.--no-verify.luac -p), shell scripts passbash -n, and QML passesqmllintwhere applicable.