Skip to content

Make CssReloadHelper watch CSS new style on appearance change #4804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BBaoVanC wants to merge 1 commit into
base: master
Choose a base branch
from
+16 −6
Open

Make CssReloadHelper watch CSS new style on appearance change #4804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions include/util/css_reload_helper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ struct pollfd;
namespace waybar {
class CssReloadHelper {
public:
CssReloadHelper(std::string cssFile, std::function<void()> callback);
CssReloadHelper(std::string cssFile, std::function<void(const std::string&)> callback);

virtual ~CssReloadHelper() = default;

virtual void monitorChanges();

virtual void changeCssFile(const std::string& newCssFile);

protected:
std::vector<std::string> parseImports(const std::string& cssFile);

Expand All @@ -42,7 +44,7 @@ class CssReloadHelper {
private:
std::string m_cssFile;

std::function<void()> m_callback;
std::function<void(const std::string&)> m_callback;

std::vector<std::tuple<Glib::RefPtr<Gio::FileMonitor>>> m_fileMonitors;
};
Expand Down
3 changes: 2 additions & 1 deletion src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,10 @@ int waybar::Client::main(int argc, char* argv[]) {
}
m_cssFile = getStyle(style_opt);
setupCss(m_cssFile);
m_cssReloadHelper = std::make_unique<CssReloadHelper>(m_cssFile, [&]() { setupCss(m_cssFile); });
m_cssReloadHelper = std::make_unique<CssReloadHelper>(m_cssFile, [&](const std::string& css_file) { setupCss(css_file); });
portal->signal_appearance_changed().connect([&](waybar::Appearance appearance) {
auto css_file = getStyle(style_opt, appearance);
m_cssReloadHelper->changeCssFile(css_file);
setupCss(css_file);
});

Expand Down
11 changes: 9 additions & 2 deletions src/util/css_reload_helper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ namespace {
const std::regex IMPORT_REGEX(R"(@import\s+(?:url\()?(?:"|')([^"')]+)(?:"|')\)?;)");
}

waybar::CssReloadHelper::CssReloadHelper(std::string cssFile, std::function<void()> callback)
waybar::CssReloadHelper::CssReloadHelper(std::string cssFile,
std::function<void(const std::string&)> callback)
: m_cssFile(std::move(cssFile)), m_callback(std::move(callback)) {}

std::string waybar::CssReloadHelper::getFileContents(const std::string& filename) {
Expand Down Expand Up @@ -88,14 +89,20 @@ void waybar::CssReloadHelper::monitorChanges() {
}
}

void waybar::CssReloadHelper::changeCssFile(const std::string& newCssFile) {
m_fileMonitors.clear();
m_cssFile = newCssFile;
Comment on lines +93 to +94
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

There's a potential race condition in changeCssFile. If a file change event from the old monitors fires after m_fileMonitors.clear() but during or after m_cssFile = newCssFile, the handleFileChange callback will be invoked with the new CSS file path for a change in the old file. To fix this, the new CSS file path should be assigned after setting up the new monitors, or the old signal connections should be explicitly disconnected before clearing the monitors.

Suggested change
m_fileMonitors.clear();
m_cssFile = newCssFile;
// Cancel existing monitors to avoid receiving further events from the old CSS file.
for (auto& monitor : m_fileMonitors) {
if (monitor) {
monitor->cancel();
}
}
m_fileMonitors.clear();
m_cssFile = std::move(newCssFile);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@BBaoVanC BBaoVanC Feb 4, 2026

Choose a reason for hiding this comment

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

If I am not mistaken, clearing m_fileMonitors drops all of the old monitors since they no longer have any references.

monitorChanges();
}

void waybar::CssReloadHelper::handleFileChange(Glib::RefPtr<Gio::File> const& file,
Glib::RefPtr<Gio::File> const& other_type,
Gio::FileMonitorEvent event_type) {
// Multiple events are fired on file changed (attributes, write, changes done hint, etc.), only
// fire for one
if (event_type == Gio::FileMonitorEvent::FILE_MONITOR_EVENT_CHANGES_DONE_HINT) {
spdlog::debug("Reloading style, file changed: {}", file->get_path());
m_callback();
m_callback(m_cssFile);
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/utils/css_reload_helper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

class CssReloadHelperTest : public waybar::CssReloadHelper {
public:
CssReloadHelperTest() : CssReloadHelper("/tmp/waybar_test.css", [this]() { callback(); }) {}
CssReloadHelperTest() : CssReloadHelper("/tmp/waybar_test.css", [this](const std::string&) { callback(); }) {}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The new changeCssFile method lacks test coverage. Since the test file already contains comprehensive tests for other methods in CssReloadHelper, tests should be added for changeCssFile to verify that it correctly updates the monitored CSS file and clears/re-establishes file monitors.

Copilot uses AI. Check for mistakes.

void callback() { m_callbackCounter++; }

Expand Down