Fix timeout created by most recent gtk-layer-shell release when creating bars for multiple monitors.#4984
Conversation
|
Can confirm, this works for me on sway and fixes the issue! |
|
This might need testing for it it still interacts properly with monitor hotplugging, resizing, and position changes. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to avoid startup timeouts with gtk-layer-shell 0.10.1 when initializing bars across multiple monitors by deferring bar creation until output discovery has completed for a batch of outputs.
Changes:
- Accumulate outputs on
zxdg_output_v1.doneand schedule bar creation once via an idle callback. - Add
Client::createBarsBatch()to create bars for all pending outputs in one pass. - Track pending outputs and whether bar creation is already scheduled (
pending_outputs_,bars_scheduled_).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/client.cpp |
Defers bar creation to a single idle callback and creates bars in a batch. |
include/client.hpp |
Adds batching method declaration and new state to track pending outputs and scheduling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string m_cssFile; | ||
| sigc::connection monitor_added_connection_; | ||
| sigc::connection monitor_removed_connection_; | ||
| std::vector<waybar_output*> pending_outputs_; |
There was a problem hiding this comment.
pending_outputs_ stores raw pointers to elements of outputs_, but outputs_ is erased in handleDeferredMonitorRemoval() and cleared in bindInterfaces(). If a monitor is removed (or a reload happens) before the scheduled batch runs, createBarsBatch() may dereference a dangling pointer. Consider clearing pending_outputs_/resetting bars_scheduled_ when outputs are cleared/removed, or store a stable key (e.g., monitor id) and re-resolve via getOutput() at batch time.
| std::vector<waybar_output*> pending_outputs_; | |
| std::vector<void*> pending_outputs_; |
| void waybar::Client::createBarsBatch() { | ||
| for (auto* output : pending_outputs_) { | ||
| try { | ||
| auto configs = getOutputConfigs(*output); | ||
| if (!configs.empty()) { | ||
| for (const auto& config : configs) { | ||
| client->bars.emplace_back(std::make_unique<Bar>(&output, config)); | ||
| bars.emplace_back(std::make_unique<Bar>(output, config)); | ||
| } |
There was a problem hiding this comment.
createBarsBatch() dereferences pointers collected in pending_outputs_ without verifying they still refer to an element in outputs_. Since outputs can be removed asynchronously (see handleMonitorRemoved() -> deferred removal), this can become a use-after-free. Suggest re-validating each pointer (e.g., resolve via getOutput(static_cast<void*>(output)) and skip on failure) or using a representation that can't dangle.
| } | ||
|
|
||
| void waybar::Client::createBarsBatch() { | ||
| pending_outputs_.remove_if([this](auto* output) { return std::none_of(outputs_.begin(), outputs_.end(), [&output](const auto& o) { return &o == output; }); }); |
There was a problem hiding this comment.
I think this fixes potential use-after-free but I could be wrong.
Attempts to fix #4978
I do not think I am great at C++, so this might be terrible, but this appears to work fine. At the very least, if this is bad, it is an example of the base steps needed to fix the problem.
This first gathers all the monitor information before creating any bars rather than creating them each individually per each .done