fix(hyprland): detect dispatch protocol via configProvider - #5231
fix(hyprland): detect dispatch protocol via configProvider#5231CristianMz21 wants to merge 2 commits into
Conversation
94fbe40 to
f3fc651
Compare
|
Now running in production on a real machine, not just in tests. Backported onto the 0.14.0 packaging and installed system-wide (Fedora 43, Hyprland 0.56.1, Lua config). Clicking a workspace button switches, and the detection resolves through the new path on the first dispatch: No Still the same caveat as above: the legacy branch is covered by the unit tests but I have no pre-0.54 instance to confirm it against. |
| @@ -333,9 +386,9 @@ bool IPC::isLuaProtocol() { | |||
| } | |||
|
|
|||
| if (luaProto) { | |||
| spdlog::info("Hyprland IPC: detected Lua-based dispatch protocol (Hyprland >= 0.54)"); | |||
| spdlog::info("Hyprland IPC: detected Lua-based dispatch protocol (version >= 0.54)"); | |||
| } else { | |||
There was a problem hiding this comment.
Is the code here make no sense any more, can we just remove them
There was a problem hiding this comment.
You are right, and it turned out to be stronger than "no longer needed" — the fallback was actively wrong. Removed in bfd0cfe.
Checked when each piece landed in Hyprland:
| v0.54.0 released | 2026-02-27 |
config/lua: init lua config manager (#13817) |
2026-04-26 |
| v0.55.0 released | 2026-05-09 |
src/config/lua/ does not exist in the v0.54.0 tree; it appears in v0.55.0. configProvider follows exactly the same line — absent in v0.54.0, present from v0.55.0 on.
Two consequences:
- There is no version that speaks Lua but does not report
configProvider, so the field is sufficient on its own and the fallback is dead code. minor >= 54was off by one. On a real 0.54 instance, which has no Lua config manager at all, it claimed Lua and broke exactly the clicks this is meant to fix. Falling back to it kept that bug alive.
So absent field now means legacy, which is correct for every release predating 0.55.
Net effect is smaller than before: -36 lines from the old heuristic, and one detection path instead of two.
Waybar decided between the legacy and the Lua dispatch protocol from the
Hyprland version alone, assuming >= 0.54 always means Lua. Hyprland does
not work that way: it only loads the Lua config manager when the config
file name ends in ".lua", so an instance running a traditional
hyprland.conf still speaks the legacy protocol no matter its version.
Those users got Lua-formatted dispatches, Hyprland answered "Invalid
dispatcher", and clicking a workspace button did nothing.
Hyprland already reports which manager it loaded, in the "systeminfo"
reply:
configProvider: lua
Read that instead. The field landed together with the Lua config manager
in 0.55, so its absence means the instance predates Lua support entirely
and necessarily speaks the legacy protocol, which makes the field
sufficient on its own and lets the version heuristic go away.
Dropping that heuristic also fixes 0.54, which it misread: Lua config
support only arrived in 0.55, so ">= 0.54" claimed Lua on a release that
has no Lua config manager at all.
The query is read-only, so detection keeps none of the side effects that
motivated replacing the earlier dispatch-based probe.
The parsing lives in a pure parseConfigProvider() helper so it can be
covered without a live compositor. Tests exercise both managers, a
realistic full systeminfo reply, the absent field, formatting variations
(tab and multi-space separators, CRLF, no trailing newline, empty value),
and the socket failure path.
Fixes Alexays#5198
f3fc651 to
bfd0cfe
Compare
|
One more data point, since #5198 raised the question of what happens when Hyprland drops legacy config support (hyprwm/Hyprland#15539, merged 2026-07-22): the field this PR relies on survives that removal. On Hyprland // src/helpers/SystemInfo.cpp
configProvider: {}
// ...
Config::typeToString(Config::mgr()->type())// src/config/ConfigManager.cpp
const char* Config::typeToString(eConfigManagerType t) {
switch (t) {
case CONFIG_LUA: return "lua";
default: return "error";
}
}So the three cases this PR handles stay correct across that transition:
Worth contrasting with the version heuristic this replaces: it would have kept drifting, since "new enough" stops implying anything about the parser once the config file name is no longer the deciding factor. Also note |
| /// Extract the "configProvider:" field from a "systeminfo" reply. | ||
| /// Returns true for the Lua manager, false for anything else, and nullopt | ||
| /// when the field is absent, as on versions predating the Lua config manager. | ||
| static std::optional<bool> parseConfigProvider(const std::string& systemInfo); |
There was a problem hiding this comment.
The name parseConfigProvider suggests that the function returns a parsed config provider, whereas it actually answers the question "is the config provider Lua?". Since the return type is std::optional<bool> it would be better to use a name similar to isLuaProtocol, where the boolean states are clearly understandable.
There was a problem hiding this comment.
Agreed — renamed to isLuaConfigProvider in 92742e4, so the bool it returns is the question the name asks.
| versionStr); | ||
| } | ||
| const size_t end = systemInfo.find_first_of("\r\n", valuePos); | ||
| std::string provider = systemInfo.substr(valuePos, end - valuePos); |
There was a problem hiding this comment.
Consider adding a check for end == std::string::npos. This code will usually work (unless someone tries to build it on some weird platform), but I'm not sure if it's considered as a good pattern.
std::string::npos is defined as static constexpr size_type npos = size_type(-1);, where size_type is defined as an unsigned integer type. So npos actually equals to the maximum positive value of size_type.
This line relies on end being a huge number in such case, and valuePos will usually have a significantly lower value, so the result of the subtraction will still exceed the remaining length of the string.
Technically this is fine, because according to cppreference substr
Returns a substring [pos, pos + count). If the requested substring extends past the end of the string, i.e. the count is greater than size() - pos (e.g. if count == npos), the returned substring is [pos, size()).
There was a problem hiding this comment.
Made it an explicit branch in 92742e4: lineEnd == npos ? substr(valuePos) : substr(valuePos, lineEnd - valuePos). You are right that the clamping behaviour is guaranteed by the standard, but the explicit form reads as intent instead of as a coincidence that happens to be well-defined.
| } | ||
| const size_t end = systemInfo.find_first_of("\r\n", valuePos); | ||
| std::string provider = systemInfo.substr(valuePos, end - valuePos); | ||
| while (!provider.empty() && std::isspace(static_cast<unsigned char>(provider.back()))) { |
There was a problem hiding this comment.
include/util/string.hpp contains an rtrim function, would that be sufficient?
There was a problem hiding this comment.
Better than sufficient — switched to trim() from util/string.hpp in 92742e4. It also absorbs the leading separator whitespace and the trailing CR, which removed the hand-rolled find_first_not_of step and the pop_back loop entirely.
| // speaks the legacy protocol. That makes the field sufficient on its own, and | ||
| // it is read-only, so detection has none of the side effects of an actual | ||
| // dispatch probe. | ||
| const bool luaProto = luaProtocolFromSystemInfo().value_or(false); |
There was a problem hiding this comment.
Is it worth it to have those functions return an optional bool considering that in the end nullopt follows the same path as false?
There was a problem hiding this comment.
You are right, and the history explains how it got there: the tri-state existed to route "field absent" to the version fallback. Once Mrpaoo's comment removed that fallback, nullopt and false converged on the same path and the optional answered nothing the caller could act on. Dropped in 92742e4 — plain bool, absent field documented as "pre-Lua instance, hence legacy".
| } | ||
| } | ||
|
|
||
| bool IPC::isLuaProtocol() { |
There was a problem hiding this comment.
Are you sure that this function needs to be split into 3? In this case I feel like it increases complexity and makes code harder to read. I suppose it makes it slightly easier to write UTs, but imo it's not a significant difference.
There was a problem hiding this comment.
Collapsed to two in 92742e4: isLuaProtocol (socket + cache + logging) and isLuaConfigProvider (pure parsing). I kept that one seam deliberately: CI has no compositor, so the parsing paths — absent field, legacy value, formatting variations — are only reachable through a function that takes the reply as a string. Folding it into isLuaProtocol would make those branches untestable rather than simpler. If you still prefer a single function I will fold it, but this is the trade-off as I see it.
Address review feedback from Bart97: - Collapse parseConfigProvider + luaProtocolFromSystemInfo into a single isLuaConfigProvider, whose name states the boolean it answers. Two functions instead of three; the pure helper stays separate from the socket call only because CI has no compositor, so the parsing paths (absent field, legacy value, formatting) are only coverable through it. - Drop the optional<bool>: since the version fallback was removed, an absent field and a non-lua value both mean legacy, so the tri-state answered nothing the caller could act on. - Handle the end-of-reply case with an explicit npos branch instead of relying on substr clamping. - Reuse trim() from util/string.hpp instead of a hand-rolled rtrim loop; it also covers the leading separator whitespace and trailing CR.
4a97df6 to
92742e4
Compare
|
Changes look fine to me now, for some reason github doesn't let me mark my comments as resolved. |
Fixes #5198.
Problem
IPC::isLuaProtocol()decides between the legacy and the Lua dispatch protocol from the Hyprland version alone:That is not the rule Hyprland uses. It only loads the Lua config manager when the config file name ends in
.lua; an instance started with a traditionalhyprland.confkeeps the legacy parser regardless of its version.So on Hyprland >= 0.54 with a
.conf, Waybar sends Lua-formatted dispatches, Hyprland answersInvalid dispatcher, and clicking a workspace button does nothing.Fix
Hyprland already reports which config manager it loaded, in the
systeminforeply:This reads that field first and falls back to the existing version heuristic when it is absent, which is the case on versions predating the Lua config manager. The query is read-only, so detection stays free of the side effects that motivated replacing the earlier
dispatch workspace __waybar_probe__approach.Behaviour by case:
configProviderluaThe parsing sits in a pure
parseConfigProvider()helper, separate from the socket call, so it is covered by unit tests without needing a live compositor.Tests
Added to
test/hyprland/backend.cpp:parseConfigProvider reads the config manager Hyprland loaded— a realistic fullsysteminforeply, barelua, barelegacy,legacysubstituted into the full reply (the exact hyprland/workspaces: Workspace switching buttons are unclickable on version >= 0.55 with traditional config format #5198 case), and an unrecognised value that must not be treated as Lua.parseConfigProvider tolerates formatting variations— tab separator, multiple spaces, CRLF line ending, value on the last line with no trailing newline, and an empty value.parseConfigProvider returns nullopt when the field is absent— an older reply and an empty reply, both of which must fall through to version detection rather than assume legacy.luaProtocolFromSystemInfo returns nullopt when Hyprland is not running—getSocket1Replythrows; detection has to degrade to the version fallback instead of propagating and breaking the click.Result on this branch:
Also verified end to end on Hyprland 0.56.1 with a Lua config:
systeminfoover the IPC socket reportsconfigProvider: lua, and the resulting/dispatch hl.dsp.focus({ workspace = "3" })returnsokand switches workspace.Caveat worth stating plainly: I could not exercise the legacy branch against a live pre-0.54 instance, only the parsing that selects it. Confirmation from someone running a traditional
hyprland.confwould be welcome.🤖 Generated with Claude Code