Migrate Hyprland workspace events to v2 #3878
Conversation
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
10 times, most recently
from
3e92344 to
24d391b
Compare
src/modules/hyprland/workspaces.cpp
Outdated
| // TODO this will be in the payload when we upgrade to focusedmonv2 | ||
| for (auto &workspace : m_workspaces) { | ||
| if (workspace->name() == workspaceName) { | ||
| m_activeWorkspaceId = workspace->id(); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
focusedmonv2 has been added in hyprwm/Hyprland#8921, perhaps we can already upgrade to it?
There was a problem hiding this comment.
Sure. I migrated all the v2 events that were in the hyprland docs.
src/modules/hyprland/workspaces.cpp
Outdated
| if (eventName == "workspacev2") { | ||
| onWorkspaceActivated(payload); | ||
| } else if (eventName == "activespecial") { | ||
| onSpecialWorkspaceActivated(payload); | ||
| } else if (eventName == "destroyworkspace") { | ||
| } else if (eventName == "destroyworkspacev2") { | ||
| onWorkspaceDestroyed(payload); | ||
| } else if (eventName == "createworkspace") { | ||
| } else if (eventName == "createworkspacev2") { | ||
| onWorkspaceCreated(payload); | ||
| } else if (eventName == "focusedmon") { | ||
| onMonitorFocused(payload); |
There was a problem hiding this comment.
According to the wiki, focusedmonv2, moveworkspacev2 and movewindowv2 are available. Perhaps you can upgrade those as well?
There was a problem hiding this comment.
general comment: can you add try/catches around the std::stoi calls? If those throw, they would kill the module (or the whole bar? not sure), and the exception messages for failing stoi calls are always very unclear.
There was a problem hiding this comment.
and maybe add const to the temporary variables you create if you don't modify or move them
There was a problem hiding this comment.
No problem.
- Added
constto the appropriate variables. - put all the
stoicalls in aparseWorkspaceIdfunction that catches and logs the exception
src/modules/hyprland/workspaces.cpp
Outdated
| int id; | ||
| std::string name; | ||
|
|
||
| try { | ||
| // If this succeeds, we have a workspace ID. | ||
| id = std::stoi(workspaceString); | ||
| } catch (const std::exception &e) { | ||
| // TODO: At some point we want to support all workspace selectors | ||
| // This is just a subset. | ||
| // https://wiki.hyprland.org/Configuring/Workspace-Rules/#workspace-selectors | ||
| if (workspaceString.starts_with("special:")) { | ||
| name = workspaceString.substr(8); | ||
| } else if (workspaceString.starts_with("name:")) { | ||
| name = workspaceString.substr(5); | ||
| } else { | ||
| name = workspaceString; | ||
| } | ||
| } | ||
|
|
||
| auto workspace = | ||
| std::find_if(m_workspaces.begin(), m_workspaces.end(), [&](std::unique_ptr<Workspace> &x) { | ||
| return (name.starts_with("special:") && name.substr(8) == x->name()) || name == x->name(); | ||
| if (name.empty()) { | ||
| return id == x->id(); | ||
| } | ||
| return name == x->name(); |
There was a problem hiding this comment.
Possible small improvement: make the id on line 711 an std::optional, and then check in find_if if that optional has value, instead of checking if the name is empty. I feel like that makes the intent clearer that the name is used as a kind of 'fallback' if the stoi fails. (but I might be nitpicking here)
There was a problem hiding this comment.
Some refactoring changed this a bit, but I think it's still aligned with making the intent clearer.
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
from
f4255c0 to
65a97fc
Compare
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
5 times, most recently
from
bb6b639 to
0a04d6e
Compare
src/modules/hyprland/workspaces.cpp
Outdated
| return workspaceIdStr == "special" ? -99 : std::stoi(workspaceIdStr); | ||
| } catch (std::exception const &e) { | ||
| spdlog::error("Failed to parse workspace ID: {}", e.what()); | ||
| return INVALID_WORKSPACE_ID; |
There was a problem hiding this comment.
Not a big fan of this INVALID_WORKSPACE_ID. If at any point Hyprland starts allowing negative workspaces, we'll have to change this.
If you make parseWorkspaceId return an std::optional<int>, and return std::nullopt, this implementation would be independant from any potential change from Hyprland. Then you also don't need to add the extra variable. Instead of checking whether the parsed workspace ID is equal to the INVALID_WORKSPACE_ID, you can simply use if (id.has_value()) { ... }.
There was a problem hiding this comment.
If at any point Hyprland starts allowing negative workspaces, we'll have to change this.
Good point. Right now workspaces only go down to -99, but optional is better. Updated.
Thanks for the suggestion.
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
4 times, most recently
from
ad54a8d to
41054e5
Compare
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
from
41054e5 to
0cc77a4
Compare
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
from
0cc77a4 to
74f4808
Compare
matt-fff
force-pushed
the
mw/hyprland-events-workspaces-v2
branch
from
74f4808 to
f7b4451
Compare
|
@Alexays When you get the chance, could you approve the workflow or let me know if there's something I need to address? Thanks! |
The Hyprland v2 workspace events add the workspace ID to the event payload.
Currently Waybar relies on workspace names because that was the best the old events provided. By transitioning to IDs instead of names, Waybar can provide a more robust experience, avoid hairy issues like #3830.
At some point I'd like to follow up with a similar change in special workspaces, but
activespecialv2doesn't exist yet in Hyprland.