Repository navigation
Conversation
A bind's description is display text, and three things use it as an identity: the menu's Lua bind cache, the key that cache recovers for loop-generated binds, and the window pattern a web app bind focuses by. Renaming a bind moves behaviour along with the label. Add opts.id, defaulting to the description so nothing changes for a bind that needs no second name, and key the menu's dispatcher and arg cache on it. The four web app binds that focus an open window name their id outright, since that string is matched against the window's own class or title. Hyprland reports no id, so a reported bind is still matched back to one through its description; that lookup and the key recovery beside it are now the only two places display text is load-bearing.
o.bind wrote the id fallback into the table the caller handed it, and that fallback only fills an id that is not set yet, so a config reusing one options table across several binds gave every later bind the first one's id. The description hid it by being assigned outright on every call.
Lua discards an argument a function does not declare, so a bind written with a fifth one binds, works, and silently loses whatever was in it. An id passed that way is the case that bites: the bind falls back to its description, and the only symptom is a row somewhere else in the menu. o.rebind and o.bind_toggle hand their extra arguments on, so a bind written through either of them warns about its own keys instead of dropping the argument one call earlier. The warning goes to stderr. The keybindings menu loads the config under a stub and parses what hl.bind prints on stdout, so that stream is not free to write to.
The other direction of the same silent loss. An id set inside a dispatcher table is read by nothing, the bind falls back to its description, and the keypress still works, so the only symptom is a row somewhere else in the menu. command_from hands back the table it was given when it recognises none of its keys, so a different return value is what says the table was a dispatcher it understood. The check keys on that rather than on a key list of its own: a table passed straight through could be anything Hyprland's API hands back, and warning there would be a guess.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| end | ||
|
|
||
| print(table.concat({ tostring(modmask), opts.description, key, kind, arg }, "\t")) | ||
| print(table.concat({ tostring(modmask), opts.description, id, key, kind, arg }, "\t")) |
There was a problem hiding this comment.
If a user-defined ID contains a tab or newline, this line writes it directly into the tab-delimited record. The cache then shifts or splits the fields and cannot recover the bind’s key and dispatcher, leaving the shortcut unusable from the menu. Escape or encode IDs before emitting records.
| local opts = {} | ||
|
|
||
| for key, value in pairs(options or {}) do | ||
| opts[key] = value | ||
| end |
There was a problem hiding this comment.
If a personal config shares bind defaults through a table’s __index metatable, pairs does not copy those inherited values. The table passed to hl.bind can silently lose flags such as locked. Preserve the effective options while avoiding changes to the caller’s table.
| # translate. Anything added alongside the four has to say so out loud. | ||
| while IFS= read -r binding; do | ||
| [[ $binding == *"id = "* ]] || | ||
| fail "every web app bind that focuses an open window names its own id" "$binding" | ||
| done < <(grep -rh 'webapp = ' "$ROOT/default/hypr/bindings" | grep -F 'focus = true') |
There was a problem hiding this comment.
These filters only inspect lines containing both webapp = and focus = true. Splitting a dispatcher table across lines makes its bind disappear from the check, allowing a missing ID to pass unnoticed. Check the loaded bind records instead of matching source lines.
The assertion that Omarchy's own bindings warn about nothing reads an empty stderr, and the scan it reads that from loads the config inside a pcall. A module that threw part way down the list would leave stderr empty too, so the assertion guarded itself with a record count instead. That count does not say what it was meant to say. The modules before applications print 201 records between them, so a config that stopped before the last one still clears a threshold of 150 and the assertion passes on a config that never finished loading. greptile's review on omacom#13923 named it. The config's own last line now writes a marker, which only runs if every require above it returned. With a module made to throw, the old guard reported the bindings as silent and the new one fails instead.
Copying the options table keeps the id fallback out of the caller's own table, but a copy made key by key sees only what that table holds itself. pairs walks no further, so a config that keeps its shared flags on one table and points each bind's options at it through __index used to hand Hyprland those flags and now hands it a copy without them. The copy carries the metatable across, so a lookup on it still finds what the original inherited. Assigning into the copy still leaves the caller's table alone, which is what the copy was for. greptile's review on omacom#13923 pointed this out.
The four web app binds that focus an open window match it against their id, so each has to name one rather than inherit a description a translator may move. The assertion for that read the binding files a line at a time, which only holds while every one of those calls is written on a single line: take the id off one and spread the call over four lines and the grep stops seeing the bind at all, counts three of the four, and reports them all well formed. o.bind knows which binds those are however they are written, and it is also the only place the distinction is visible: by the time a record reaches the menu, an id a bind named itself and one that defaulted to the description are the same string. The check now wraps o.bind, loads every binding file Omarchy ships, and asks for each web app bind that focuses a window whether its caller passed an id. greptile's review on omacom#13923 pointed out the line-at-a-time hole.
The menu's Lua scan reports one bind per line as tab separated fields, three of which are the config author's own text: the description, the id this branch adds, and the command. A tab in any of them moves every field after it one column along, and a newline splits the bind into two records. With a tab in a description the reader on the other side takes the second half for the key and the key for the dispatcher, which is a wrong row today and a wrong cache key once rows are keyed on the id. Each field is folded to one line before it is printed. A bind is worth reporting with its label squeezed and not worth losing. greptile's review on omacom#13923 found this on the id field. The description has carried it as long as the scan has, so the fix covers every field rather than the one the review landed on.
|
All four reproduce. One of them is this branch's own and is fixed here; the other three are in the commits it stacks on, so they are fixed on #12557's branch where that code lives. The record count that proved nothingRight, and the problem is not that 150 was the wrong number. A config made to throw after The config's own last line now writes a marker, which runs only if every A tab or a newline in a fieldReproduced, and it predates the id field. On Six fields where the reader expects five, so it reads So the fix folds every field to one line rather than only the field the review landed on, and the test's config now carries a bind whose label holds a tab and whose id holds a newline. Options inherited through a metatableCorrect. The copy carries the metatable across now, and assigning into it still leaves the caller's table alone, which is what the copy was for. The web app id check read a line at a timeReproduced: take the id off one of those four binds and spread the call over four lines, and the grep finds three binds and reports all three well formed. Reading the loaded records instead cannot replace it, though, and the reason is worth putting on the record. All four of those binds declare an id that equals their description, SuitesSame Arch box, same session,
The seven and the one are the same every time and all environmental on that box, wanting an Why three of the four are fixed elsewhereThis branch is based on Thank you for the review. Two of the four were in the tests I wrote to check my own work, and a test that passes for the wrong reason is the one kind of defect that hides itself. 🤖 Generated with Claude Code |
2 participants
A bind can lose its id two ways, and both are silent.
o.bindtakes four arguments, so Lua discards a fifth one. The bind still binds and still works, having quietly dropped whatever was in it. An id set inside the dispatcher table instead of the options table is read by nothing, which ends the same way. Either shape leaves the bind falling back to its description, the keypress unaffected, and the only symptom a row somewhere else in the keybindings menu.Both say so now, on stderr, naming the keys and quoting the id back. Two binds, one of each shape, written on top of the example
config/hypr/bindings.luaalready carries in a comment:stderr rather than stdout on purpose. The keybindings menu loads the whole config under a stub and reads bind records off stdout, so a warning written there would arrive as a row.
Where this came from
@WokoFlipper has been translating their config and publishing it on #12557 and #13888, and it is the best test material this area has had. It declares 262 ids, and three revisions in, some of them still sit where nothing reads them. Nothing has ever said so.
Counted by where the id sat, against each published revision:
d412c9e100f5f3a51671fc21The second row is the first one with two mouse binds fixed. The third is the next attempt, which moved the id out of the fifth argument and into the dispatcher table, where it is just as unread. Three rounds of a reasonable author doing reasonable things, and the config gave back no signal at any point. That is the gap this closes.
Why the check is in
o.bindand not in a linterI wrote a static checker first, splitting each
o.bindcall on its top-level commas. It agrees with the runtime warning exactly, 31 / 29 / 0 and 0 / 0 / 23, which is two independent implementations reaching the same count.What it took to get there is the argument for the runtime. My first two attempts at finding a fifth argument matched brace-balanced table literals and both reported 29 on the first revision instead of 31. The two they missed are the mouse binds, whose third argument is
hl.dsp.window.drag(): a call, not a literal. Anything that reads the source text has to understand Lua to count arguments.select("#", ...)already knows.What the id warning keys on
command_fromhands back the table it was given when it recognises none of its keys, and every branch that does recognise something returns a string or anhl.dsp.global. So a return value that is not the table it was handed is exactly "this was a dispatcher Omarchy understood", with no second copy ofcommand_from's key list to keep in step with it.That scoping is deliberate rather than incidental. A table
command_frompasses straight through could be anything Hyprland's own API hands back, and anidfield in one of those might well be read by the compositor. Warning there would be a guess, so it does not.Nothing changes on a stock machine
The config Omarchy ships calls
o.bind189 times across the six binding modules ando.bind_toggleanother 6. Loading all of them came to 231o.bindinvocations on the box I measured, since some sit in loops and a few are guarded by what is installed. None of them passes a fifth argument and none puts an id in a dispatcher table, counted both from the source text and from insideo.bindas it ran, so a stock machine sees no warning at all. The four binds that do declare an id, thewebappplusfocusones inapplications.lua, already put it in the options table where it is read.Read this as a stack
This is stacked on #12557, which is what introduces
opts.id. Without it there is no options table to point a config author at and the advice in both warnings has nowhere to go. The diff below is five commits; the first two are #12557's and have to land first.Tests
test/shell.d/hyprland-bind-warnings-test.shis new, 10 assertions. Five are the fifth argument: it is reported with the keys that passed it, four arguments stay silent, the warning stays off the stdout the menu parses while the bind still dispatches what it declared, ando.rebindando.bind_toggleeach hand their extras on so the warning names the caller's own keys rather than losing the argument one call earlier. Four are the id: a dispatcher table carrying one is reported with the id quoted back, an id in the options table stays silent, the bind still dispatches exactly what it declared, and a dispatcher table whose keys Omarchy does not recognise is left alone.The tenth is the one that matters most and it was nearly worthless. It loads every binding module Omarchy ships through the menu's own scan and asserts stderr is empty. The scan wraps the config in
pcall, so a module that threw would also leave stderr empty and pass. The config's own last line now writes a marker, which runs only if everyrequireabove it returned. Counting the records instead, which is what this PR opened with, does not work: a config made to throw two modules short of the end still printed 201 of them.Each warning was also run with only its own hunk removed. With the id check gone, the five fifth-argument assertions still pass and the id one is what fails. With both gone, the first assertion in the file is what fails.
hyprland-bind-id-test.shstays at 10 andkeybindings-menu-test.shat 16, and full./test/shellon one Arch box gains exactly the 10 assertions added here:125284e2The base was run twice, before and after the branch, and came out at 3961 / 7 both times, so nothing here is load flakiness. The same seven fail on every run and all seven are environmental on that box, wanting an
omarchy-pkgscheckout, alinux-omarchyheaders package, a browser window to focus, and a migration message.Credit
@WokoFlipper for the translated config, for publishing every revision of it, and for the review on #13888. Three rounds of a config that meant its ids exactly right and lost a slice of them to the API each time is what makes this a missing warning rather than a style note, and none of it would be visible without their work in the open.
@playGitboy's #13276 is what lets a user's own
~/.config/hypr/bindings.luabe scanned at all, which is where most binds written by hand will be. greptile's review on #13888 is what taught me to check whether the thing I claimed was harmless actually was.🤖 Generated with Claude Code