Skip to content

fix(hyprland): Support addressable workspace IPC - #5324

Open
tonybutt wants to merge 5 commits into
Alexays:masterfrom
tonybutt:hyprland-addressable-workspaces
Open

tonybutt wants to merge 5 commits into
Alexays:masterfrom
tonybutt:hyprland-addressable-workspaces

Conversation

@tonybutt

Copy link
Copy Markdown

Fixes #5316.

Hyprland removed the numeric workspace id from its IPC in hyprwm/Hyprland#16140, replacing it with a string address plus an explicit type ("numbered", "special", "named"). Waybar still reads ["id"].asInt(), which now returns 0 for every workspace, so every workspace compares equal to every other:

  • persistent workspaces are never created
  • every client matches every workspace, so all window icons render on all of them

This matches the reports in #5316 ("both of space show as 1", "No workspace with id 0", and the hyprctl reload workaround that stops working on the next workspace switch).

Does this overlap #5013 / #5043 / #5231?

No. Those fixed the dispatch path — how waybar sends commands to Hyprland under the Lua IPC protocol. This fixes the parse path — how waybar reads workspace identity out of the IPC payload. Different layers.

To confirm rather than assume, I built master at 2a12740b, which contains all three, and ran it against Hyprland main:

master @ 2a12740b with this PR
No workspace with id 0 warnings 11 0
workspaces collapsed onto id 0 7 0
distinct workspaces created 2 7

Master creates only 0 and 1; with this PR the same session creates 1, 6 and five special workspaces by name.

Approach

A single pure function, parseWorkspaceIdentity(), normalises both payload shapes into {address, kind, number}. Detection is by field presence — address when present, otherwise the legacy id — so no call site branches on compositor version and older Hyprland keeps working.

Workspaces are then keyed by address, and their kind comes from type. That last part also removes the arithmetic in sortWorkspaces() that inferred kind from the id's sign and range (-99 special, -98..-1 named special, <= -1377 named): the compositor now states kind explicitly, so waybar no longer has to reverse-engineer it.

Two commits: the parser (additive, with tests), then the migration. Both build and pass tests.

Behaviour changes worth flagging

  • {id} renders the address. For numbered workspaces the output is byte-identical. For named and special ones it replaces a negative sentinel (-99) with something meaningful. Man page updated.
  • sort-by: id groups numbered → named → special, rather than ordering by id sign. Man page updated.
  • The generic special workspace no longer pins last among specials — that behaviour was the -99 magic number this change removes.

Also fixed along the way

  • A persistent workspace named literally special threw std::out_of_range from substr(8).
  • A persistent workspace whose selector matched an existing workspace by name (e.g. special, or a named workspace) was duplicated instead of deduped, and never got persistent-config applied.

Testing

test/hyprland/workspace_identity.cpp and test/hyprland/workspace_order.cpp cover the parser on both schemas and the ordering axioms. The ordering functions live in workspace_identity.cpp rather than inline in workspaces.cpp specifically so they are reachable from the test target.

The orderings are strict weak orderings, which std::ranges::sort requires — violating that is UB. Neither assumes a Numbered workspace has a usable number, since an address can be labelled numbered and still not fit in an int; comparing such a workspace numerically against some peers and by name against others is what produces a comparison cycle.

Verified at runtime against Hyprland main (post-#16140): correct per-workspace icons, persistent workspaces created, no errors. The legacy path is covered by unit tests only, since the machine runs main.

sortWorkspaces() itself is still not directly unit-testable — it needs a live Workspaces& and Gtk::Button, so the extracted orderings are covered but the call site is not.

Out of scope

SortMethod::NUMBER is also not a strict weak ordering — std::stoi throws on a non-numeric name and falls through to a name comparison. That predates this change and is untouched here, but it is in the same switch so it is worth naming.

Happy to rework any of this — the identity abstraction, the commit split, or the behaviour changes above. If there is a constraint or a use case I have not considered, or you would rather this were shaped differently, say so and I will adjust.

cc @kolayne @edeustua — you both mentioned in #5316 that you were planning to look at this, so flagging it to avoid duplicated effort.

Hyprland removed the numeric workspace id from its IPC in hyprwm/Hyprland
16140, replacing it with a string address and an explicit type. Add a
single pure parser that normalises both the new and legacy payloads into
an address, a kind and an optional number, so call sites never branch on
compositor version.

The kind also replaces the arithmetic that previously inferred it from
the id's sign and range. Ordering helpers live here too, beside the data
they order and inside the test target.
Waybar read ["id"].asInt() for every workspace identity comparison. Since
hyprwm/Hyprland 16140 that field is gone, so the read yields 0 and every
workspace compares equal to every other: persistent workspaces are never
created, and every client matches every workspace, rendering all window
icons on all of them.

Key workspaces by their address and take their kind from the type field,
via the shared parser. The legacy id payload still works, so older
Hyprland is unaffected.

Two user-visible changes follow. The {id} format placeholder renders the
address, which is identical for numbered workspaces and replaces a
meaningless negative sentinel for named and special ones. sort-by: id
groups numbered, then named, then special, rather than by id sign.
Comment thread src/modules/hyprland/workspace.cpp Outdated
Comment on lines 56 to 63
if (m_name.starts_with("name:")) {
m_name = m_name.substr(5);
} else if (m_name.starts_with("special")) {
m_name = m_id == -99 ? m_name : m_name.substr(8);
} else if (m_identity.kind == WorkspaceKind::Special) {
m_isSpecial = true;
if (m_name.starts_with("special:")) {
m_name = m_name.substr(8);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also have a special workspace whose name is special:123 (i.e., hyprctl dispatch 'hl.dsp.workspace.toggle_special("special:123")'). This will then cut the prefix off, won't it?

(same for name:)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I'm wrong with this one and you handle this in some other place, I'd argue:

  1. This is a tricky corner case, so we should probably have tests for this (a special namespace whose name starts with special:, a named workspace whose name starts with name:, a special workspace whose name starts with name:, etc).
  2. The fact that I read this piece of code and had this question indicates that these checks are confusing. Would be nice if we could get rid of these checks on workspace name completely in the main module code. Should we shift the workspace parsing logic to the workspace_identity's area of responsibility?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this, and the two halves land differently.

special: is fine as written — substr(8) removes exactly one prefix, so special:special:123 becomes special:123, which is the name you'd want. (isDoubleSpecial() rejects those at createworkspacev2 anyway, but initializeWorkspaces() doesn't, so it is reachable at startup.)

name: is a real bug, and worse than a stripping-too-much bug: it shouldn't be in this path at all. Hyprland never prepends name: to a reported name — it's selector syntax. The only thing producing a name:-prefixed name here is createMonitorWorkspaceData(), i.e. a persistent-workspaces entry. So a workspace genuinely called name:foo was being truncated to foo by selector handling that had leaked into the IPC path.

Both fixed in dc2c2c5 by the split you suggest below.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on both counts, and done in dc2c2c5.

workspace_identity now owns the mapping in both directions:

  • workspaceDisplayName(rawName, kind) — for names Hyprland reports. Strips exactly one special:, and nothing else. Never touches name:.
  • parseWorkspaceSelector(selector) / workspaceRawName(selector) — for selectors the user writes in persistent-workspaces, split into {kind, name} and back.

createMonitorWorkspaceData() runs the selector through that pair, so the placeholder payload is IPC-shaped and the Workspace constructor no longer knows selector syntax exists. The four remaining starts_with("special") / substr(8) sites in workspaces.cpp went the same way.

Writing the round-trip test surfaced something I'd otherwise have missed: special and special:special are different workspaces that display the same name, so the selector has to carry an isGenericSpecial flag or the mapping isn't invertible. That also replaced the name() != "special" sentinel in handleClicked(), which would have misrouted a special workspace the user named special.

Tests in test/hyprland/workspace_identity.cpp cover the cases you listed — special:special:123, name:foo as a real name, special:name:foo, specialfoo (not special), bare special: / name: prefixes, and the selector round-trip.

Comment thread src/modules/hyprland/workspace.cpp Outdated
for (const auto& client : clients_data) {
if (client["workspace"]["id"].asInt() == id()) {
const auto clientWorkspace = parseWorkspaceIdentity(client["workspace"]);
if (clientWorkspace.has_value() && clientWorkspace->address == address()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we issue a warning here if !clientWorkspace.has_value()?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — a client with no parseable workspace means malformed IPC and shouldn't be silent. Added in dc2c2c5, at debug rather than warn: initializeWindowMap() runs once per workspace, so one malformed client would otherwise be reported once per workspace.

case WorkspaceKind::Special:
return 2;
}
return 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be impossible? Log error here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these are gone in dc2c2c5, but by deleting the function rather than logging in it.

workspaceKindRank() was a hand-written table mapping Numbered/Named/Special to 0/1/2 — which is exactly WorkspaceKind's own declaration order. Two copies of one fact, free to drift. It's now constexpr int workspaceKindRank(WorkspaceKind kind) { return static_cast<int>(kind); } in the header, with the enum documented as load-bearing. No table, no unreachable branch, no magic numbers.

I'd have pushed back on logging here regardless: this runs inside a sort comparator, so a log on that branch would fire O(n log n) times if it ever did execute.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, a logger in the comparator probably does not make sense.

Comment on lines +72 to +75
case WorkspaceKind::Named:
break;
}
return "named";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log if the given kind is invalid?

Suggested change
case WorkspaceKind::Named:
break;
}
return "named";
case WorkspaceKind::Named:
return "named";
}
// ------> Log error here? <--------
return "named";

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took your shape for workspaceTypeName() — every case returns, so -Wswitch now fails the build if an enumerator is added without a case, which catches the mistake before it ships rather than at runtime. The trailing return stays only to satisfy -Wreturn-type, with a comment saying so.

Left the log out deliberately: the compiler is the stronger guard, and a log on a branch that can't execute is noise in the source for no runtime benefit. Happy to add one if you'd rather have belt and braces.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler is a stronger guard for sure, but the thing is that C++ does not require that the set of values of WorkspaceKind be the variants you defined (unlike, e.g., Rust enums). For example, both WorkspaceKind::Special | WorkspaceKind::Named and 123 are valid values of type WorkspaceKind in C++.

In particular, the compiler was complaining about -Wreturn-type before you added a catch-all return exactly because there are some valid values of WorkspaceKind, for which the function would end without a return (which would have been undefined behavior).

Now, your current code has well-defined behavior. Still, should something bad happen, it'll be pretty hard to debug, given that the catch-all return returns "named", which matches one of the previous values. It should at least be something like "unknown". We may also log a warning. We may alternatively abort waybar (🥲).

Notably, if these were Rust enums, it would have been exactly like you described: the compiler would have proven that no cases other than the three are possible at runtime, and would give you the opposite warning, telling that your catch-all return at the end of the function is unreachable.

@vp1981

vp1981 commented Sep 16, 2026

Copy link
Copy Markdown

I gave it a try, and in my case waybar with this PR works as expected on Hyprland:

Hyprland 0.56.0 built from branch main at commit c26dbf9337998ba9afcfe84050e36969bb787598 clean (main: check XDG_RUNTIME_DIR later ( 16256)).
Date: Mon Sep 14 18:20:52 2026
Tag: v0.56.0-191-gc26dbf933, commits: 7816

Libraries:
Hyprgraphics: built against 0.5.1, system has 0.5.1
Hyprutils: built against 0.14.2, system has 0.14.2
Hyprcursor: built against 0.1.13, system has 0.1.13
Hyprlang: built against 0.6.8, system has 0.6.8
Aquamarine: built against 0.15.0, system has 0.15.0

Version ABI string: c26dbf9337998ba9afcfe84050e36969bb787598_aq_0.15_hu_0.14_hg_0.5_hc_0.1_hlg_0.6
no flags were set

I'm not familiar with the codebase, but I think @kolayne's comments are worth considering.

@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces a normalized string-based Hyprland workspace identity, migrates workspace and client matching away from numeric ids, updates sorting and formatting semantics, and adds parser and ordering tests. The primary remaining problems are compatibility with legacy special-workspace event identifiers and overly broad display-name reconciliation.

  • Supports both legacy numeric IPC identities and addressable workspace payloads.
  • Uses workspace addresses for active, visible, urgent, client, and lifecycle matching.
  • Adds kind-aware default/id ordering and documents the changed behavior.
  • Adds unit coverage for identity parsing, selector matching, and comparator axioms.

Confidence Score: 2/5

The PR is not yet safe to merge because it can omit legacy special workspaces and conflate or stale distinct addressable workspaces during name-based reconciliation.

Three reachable workspace lifecycle failures remain: legacy special events do not match normalized -99 addresses, named and special workspaces with the same display name are deduplicated, and selector-backed placeholders retain synthetic addresses after their live workspace appears.

Files Needing Attention: src/modules/hyprland/workspaces.cpp, src/modules/hyprland/workspace_identity.cpp, src/modules/hyprland/workspace.cpp

Important Files Changed
Filename Overview
src/modules/hyprland/workspace_identity.cpp Adds legacy/current identity normalization, selector matching, address classification, and strict ordering helpers.
src/modules/hyprland/workspaces.cpp Migrates workspace lifecycle and state reconciliation to addresses, but legacy special events and name-based deduplication can lose or stale workspaces.
src/modules/hyprland/workspace.cpp Stores normalized identities and dispatches clicks by workspace kind and address.
test/hyprland/workspace_identity.cpp Covers identity parsing and basic selector-name matching across both IPC schemas.
test/hyprland/workspace_order.cpp Exercises ordering behavior and strict weak ordering across mixed workspace identities.
man/waybar-hyprland-workspaces.5.scd Documents address-based {id} output and the new kind-aware sorting semantics.
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Hyprland IPC workspace payload] --> B[parseWorkspaceIdentity]
  B --> C[WorkspaceIdentity: address, kind, number]
  C --> D[Workspace objects keyed by address]
  E[Hyprland workspace events] --> F[Raw event address field]
  F --> D
  G[Persistent workspace selector] --> H[Synthetic workspace identity]
  H --> I[Address match]
  I -->|miss| J[Display-name fallback]
  J --> D
  D --> K[Active and visible state]
  D --> L[Client and window counts]
  D --> M[Click dispatch]
  D --> N[Workspace ordering]
Loading
Comments Outside Diff (1)
  1. src/modules/hyprland/workspaces.cpp, line 113-131 (link)

    P1 Persistent identity remains stale

    When a selector-backed persistent placeholder already exists, this path returns without replacing its synthetic identity with the live workspace identity. For example, an initially absent name:web entry retains the address name:web after Hyprland creates the real workspace at address web. Later address-based client, visibility, count, and removal updates therefore do not match the displayed workspace.

Reviews (1): Last reviewed commit: "fix(hyprland): Support addressable works..." | Re-trigger Greptile

Comment thread src/modules/hyprland/workspaces.cpp Outdated
Comment on lines +399 to +401
for (auto workspaceJson : workspacesJson) {
const auto currentId = workspaceJson["id"].asInt();
if (currentId == *workspaceId) {
const auto currentIdentity = parseWorkspaceIdentity(workspaceJson);
if (currentIdentity.has_value() && currentIdentity->address == workspaceAddress) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Legacy special events mismatch

On legacy Hyprland, a generic special-workspace event identifies the workspace as special, while parseWorkspaceIdentity normalizes its IPC id to the address -99. Comparing the raw event field directly with the normalized address therefore fails during createworkspacev2, so the special workspace and its client associations are not created. The same mismatch prevents the rename and id-change handlers from finding it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conclusion holds but not via this mechanism. createworkspacev2 emits the numeric id, not the literal special — the pre-change code parsed it with std::stoi (parseWorkspaceId, workspaces.cpp:1219 at 2a12740). The literal special case in that helper existed for persistent-workspaces selectors, which createWorkspace() and removeWorkspace() already handled via the name fallback.

Where it did bite is onWorkspaceRenamed() and onWorkspaceIdChanged(), which matched address-only and had no fallback. dc2c2c5 routes all four through one findWorkspace() that tries the address and then the selector, so the legacy mapping is restored everywhere rather than in three of four places.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right—the original comment attributed the failure to createworkspacev2, but that event already carries the numeric workspace ID on the legacy path and was handled by parseWorkspaceId(). The actual gap was in onWorkspaceRenamed() and onWorkspaceIdChanged(), whose address-only lookups had no selector/name fallback.

findWorkspace() in dc2c2c5 is the better fix: it first matches the normalized address, then parses the incoming identifier as a selector and matches by kind, generic-special status, and display name. That restores the legacy special mapping consistently for create/remove/rename/id-change flows while retaining address-based matching for the new IPC schema.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Comment thread src/modules/hyprland/workspaces.cpp Outdated
Comment on lines +102 to +110
if (workspace == m_workspaces.end()) {
// A persistent-workspaces entry is addressed by the selector the user
// wrote, which on legacy Hyprland never matches the live workspace's
// numeric address (a configured "special" against address "-99").
const auto selector = workspace_data["name"].asString();
workspace =
std::ranges::find_if(m_workspaces, [&selector](std::unique_ptr<Workspace> const& w) {
return workspaceSelectorMatchesName(selector, w->name());
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Distinct workspaces are conflated

This display-name fallback applies to every queued workspace and does not check its kind. A named workspace spotify and a special workspace special:spotify both expose the display name spotify, so whichever is processed second is treated as the first workspace instead of receiving its own button. Its windows, visibility, and click behavior are then attributed to the wrong workspace.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in dc2c2c5. special:spotify and a named spotify both display spotify, and the fallback compared display names only.

The match now requires equal WorkspaceKind, which separates them (selector spotify classifies Named, special:spotify Special), and is restricted to placeholders that haven't yet adopted a live identity, so it can't fire for two live workspaces at all.

A persistent-workspaces entry has no IPC payload behind it, so its
placeholder was keyed by the selector the user wrote. When the live
workspace appeared, createWorkspace() matched the placeholder by display
name and returned without adopting the real identity, leaving the button
addressed as e.g. "name:web" while Hyprland addressed it "web". Every
later address-keyed lookup -- clients, visibility, window counts,
destroy -- then missed the workspace the user could see.

That name match also ignored kind, so a named workspace "foo" and a
special "special:foo", which display the same name, were folded into one
button.

Match on kind as well as name, restrict the fallback to placeholders
that have not yet adopted an identity, and adopt the live identity on
match.

Workspace name parsing moves into workspace_identity, which now owns the
mapping in both directions: workspaceDisplayName() for names Hyprland
reports, parseWorkspaceSelector()/workspaceRawName() for selectors the
user writes. Three bugs fall out of separating them:

- "name:" is selector syntax Hyprland never prepends to a reported name,
  so a workspace actually called "name:foo" displayed as "foo".
- A workspace merely named "specialfoo" was classified special.
- The generic special workspace was identified as the one displaying
  "special", which is also what a special workspace the user named
  "special" displays; it now carries an explicit flag.

Ordering no longer keeps a hand-written kind-to-rank table, which
duplicated WorkspaceKind's own declaration order and could drift from
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tonybutt

Copy link
Copy Markdown
Author

Pushed dc2c2c5 addressing this round. Summary, including the finding that had no inline thread to reply to.

Persistent placeholder kept its synthetic address

The one I'd call blocking, and the finding greptile raised outside the diff. Confirmed: createMonitorWorkspaceData("name:web") builds address name:web; when the live workspace appeared at address web, createWorkspace() missed on address, matched by display name, and returned early without adopting the live identity. Every later address-keyed lookup — clients, visibility, window counts, destroy — then missed the button on screen.

Bounded to named and special persistent entries: a numbered one ("3" → address 3) matches by address and was never affected. It now adopts the live identity on match.

Named and special workspaces conflated

special:spotify and a named spotify display the same name, and the fallback compared display names only. It now requires equal WorkspaceKind and is restricted to placeholders that have not yet adopted an identity, so it cannot fire between two live workspaces.

Legacy special-99

Real, though not by the route greptile described — createworkspacev2 emits a numeric id, not the literal special. The gap was that onWorkspaceRenamed() and onWorkspaceIdChanged() matched address-only with no selector fallback. All four sites now share one findWorkspace().

Name parsing moved into workspace_identity

@kolayne's structural point, which turned out to be load-bearing rather than cosmetic. Separating "names Hyprland reports" from "selectors the user writes" made three latent bugs fall out:

  • name: is selector syntax Hyprland never prepends to a reported name, so a workspace genuinely called name:foo displayed as foo. That check should never have been in the IPC path.
  • A workspace merely named specialfoo was classified special.
  • The generic special workspace was identified as "the one displaying special" — which is also what a special workspace the user named special displays. It now carries an explicit flag, which also replaced the name() != "special" sentinel in handleClicked().

special: itself was already correct: substr(8) strips one prefix, so special:special:123special:123.

Magic numbers

workspaceKindRank()'s 0/1/2 table duplicated WorkspaceKind's own declaration order — two copies of one fact, free to drift. It is now static_cast<int>(kind) with the enum documented as load-bearing, which also removes the unreachable branch @kolayne flagged. workspaceTypeName() keeps a switch, now exhaustive so -Wswitch catches a new enumerator at build time.

I left the logging out of both rather than adding it: workspaceKindRank() runs inside a sort comparator, and the compiler is the stronger guard than a runtime log on a branch that cannot execute. Say the word if you'd rather have both.

Tests

test/hyprland/workspace_identity.cpp covers the corner cases @kolayne asked for: special:special:123, name:foo as a real name, special:name:foo, specialfoo, bare special: / name: prefixes, and a selector round-trip. That round-trip is what surfaced special vs special:special being distinct workspaces with identical display names — I would not have found it by inspection.

Full suite: 37 cases, 646 assertions, passing. Still verified at runtime against Hyprland main; the legacy path remains unit-test-only.

Hyprland announces a workspace by the identifier it was created with,
which is not always the address `workspaces` reports. Observed on
0.56.0:

  createworkspacev2>>name:web,web            -> address "web"
  createworkspacev2>>special:newspecial,...  -> address "special:newspecial"

Numbered and special workspaces agree, so they worked. A named one is
announced as "name:web" and reported at address "web", so
onWorkspaceCreated() compared the event field against every entry in the
`workspaces` reply, matched none, and never queued the workspace. No
button was created for it at all, and a persistent placeholder for the
same name never adopted the live address.

Resolve the event identifier by address first and by selector second, so
both spellings reach the same workspace.

Found by running the bar against a live Hyprland session; the addressable
schema is only reachable there, so no unit test would have caught it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@edeustua edeustua left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@kolayne kolayne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your fixes!

I had a fresh look and left some more comments.

Comment thread include/modules/hyprland/workspace.hpp Outdated
Comment on lines +46 to +49
// The special workspace with no name -- `togglespecialworkspace` with an
// empty argument. Not the same as a special workspace the user named
// `special`, which displays the same name but toggles by name.
bool isGenericSpecial() const { return m_isGenericSpecial; };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the same as a special workspace the user named special

Is it not, indeed?

At least on my version (built from hyprwm/Hyprland@7ebf13), if I do hyprctl dispatch 'hl.dsp.workspace.toggle_special("special")' and hyprctl dispatch 'hl.dsp.workspace.toggle_special()', the two dispatchers bring me to the same workspace.


While we're at it, I also find this name a bit confusing. Maybe should be, e.g., AnonymousSpecial/NamelessSpecial/UnnamedSpecial?

case WorkspaceKind::Special:
return 2;
}
return 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, a logger in the comparator probably does not make sense.

Comment on lines +72 to +75
case WorkspaceKind::Named:
break;
}
return "named";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler is a stronger guard for sure, but the thing is that C++ does not require that the set of values of WorkspaceKind be the variants you defined (unlike, e.g., Rust enums). For example, both WorkspaceKind::Special | WorkspaceKind::Named and 123 are valid values of type WorkspaceKind in C++.

In particular, the compiler was complaining about -Wreturn-type before you added a catch-all return exactly because there are some valid values of WorkspaceKind, for which the function would end without a return (which would have been undefined behavior).

Now, your current code has well-defined behavior. Still, should something bad happen, it'll be pretty hard to debug, given that the catch-all return returns "named", which matches one of the previous values. It should at least be something like "unknown". We may also log a warning. We may alternatively abort waybar (🥲).

Notably, if these were Rust enums, it would have been exactly like you described: the compiler would have proven that no cases other than the three are possible at runtime, and would give you the opposite warning, telling that your catch-all return at the end of the function is unreachable.

// Classifies a bare address for the one path that has no IPC `type` field to
// read: the `workspacev2`-style rename that moves a workspace between the
// numbered and named namespaces.
WorkspaceKind workspaceKindForAddress(const std::string& address);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should workspaceKindForAddress and parseWorkspaceSelector become just constructors of the corresponding classes?

Maybe some other functions can be converted to methods, too.

Comment on lines +17 to +19
std::string address;
WorkspaceKind kind = WorkspaceKind::Named;
std::optional<int> number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider is if we can change this structure such that it does not contain redundancy. For instance, store all the information that is needed to build the address, and then only generating address when requested, rather than storing it.

It would be ideal if the structure was also such that any representable value is logically valid... We can probably do this with std::variant (to represent that every workspace has either a name or a number). Is the project's C++ standard recent enough to use std::variant?

Comment thread include/modules/hyprland/workspaces.hpp Outdated
Comment on lines +193 to +196
int m_activeWorkspaceId;
std::string m_activeWorkspaceAddress;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the workspace identity can completely abstract parsing details, including the concept of address. Should we have the workspace identity object here and in the methods?

Comment thread src/modules/hyprland/workspaces.cpp Outdated
Comment on lines +927 to +928
return w->kind() == selector.kind && w->isGenericSpecial() == selector.isGenericSpecial &&
w->name() == selector.name;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be a method on either workspace or selector class.

Comment on lines +102 to +111
std::string workspaceDisplayName(const std::string& rawName, WorkspaceKind kind) {
if (kind == WorkspaceKind::Special && rawName.starts_with(kSpecialPrefix)) {
return rawName.substr(kSpecialPrefix.size());
}
return rawName;
}

bool isGenericSpecialName(const std::string& rawName, WorkspaceKind kind) {
return kind == WorkspaceKind::Special && rawName == kGenericSpecial;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suspicious that you check for both. Should the first condition always imply the second one?

If so, I would write this as

if (kind == WorkspaceKind::Special) {
    assert(rawName.starts_with(kSpecialPrefix));
    // ...

If there is no such implication (i.e., we may receive a rawName that has already been stripped of the special: prefix), then there should be a bug when the function is called for special:special:123 if its prefix is stripped: the function will strip it again.

Comment on lines +136 to +138
return identity.kind == selector.kind &&
isGenericSpecialName(rawName, identity.kind) == selector.isGenericSpecial &&
workspaceDisplayName(rawName, identity.kind) == selector.name;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this equality check be a method of WorkspaceIdentity?

The generic special workspace is not distinct from a special workspace
the user named `special`. Hyprland resolves `toggle_special()` and
`toggle_special("special")` to the same workspace, reported as
`special:special` since 0.41 -- verified on 0.56.0 and in
getWorkspaceIDNameFromString across v0.41..v0.56.

Dropping the isGenericSpecial flag that modelled the difference fixes
`persistent-workspaces: ["special"]`. Its placeholder was built with the
raw name `special`, so it never matched the live `special:special`
workspace, kept its synthetic address, and every later address-keyed
lookup missed the button on screen.

WorkspaceIdentity now carries the display name alongside the address and
kind, derives the number from the address rather than storing it, and
owns the matching that callers had open-coded. Selector parsing becomes
a WorkspaceSelector constructor; parseWorkspaceIdentity stays a factory
because it can fail. m_activeWorkspaceAddress becomes
m_activeWorkspaceIdentifier and is compared through the identity, since
Hyprland announces a named workspace by the `name:foo` selector it was
created with rather than by the address it reports.

workspaceTypeName() returns "unknown" and logs for a kind outside the
enumerators rather than silently reading as "named".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hyprland/workspaces won't work on the next release of Hyprland

4 participants