refactor(cava): fix thread-safety, resource leaks, and style violations

[LukashonakV]

Hi @Alexays ,

This MR was created using the deep kimi-k2.6 support. I used it for the first time to identify what could be improved or fixed for the CAVA module. While I don’t fully agree with some of the criticisms (for example, the “high risk” points), I would still say that the AI is quite useful for reviewing existing functionality, improving it, and polishing it.

Even so, the AI helped me find some race conditions, and the fix improved the CAVA module’s performance.

All proposed changes were reviewed by me.

To outline the process:
Using the AI tool, I inspected the project at a high level to gather the necessary information, which I documented in the CONVENTION.md.
Based on that convention, I used the plan in cava_module_refactor_plan.md to construct this MR.

If it’s useful for the project, I can also push the commit for CONVENTION.md.

Please see the commit overview below.

Cava Module Refactor

Summary

This MR addresses all outstanding style violations, architectural risks, thread-safety bugs, and resource leaks in the cava module.

What changed

Code Style & Naming

• Renamed class Cava -> CavaRaw to match filename convention
• Renamed methods to lowerCamelCase (update(), pauseResume(), etc.)
• Renamed signal aliases to PascalCase (SignalUpdate, SignalAudioRawUpdate)
• Replaced NULL with nullptr throughout the backend
• Replaced all C-style casts with static_cast
• Added missing explicit includes: <memory>, <string>, <chrono>
• Fixed member variable naming (frame_counter_, etc.)

Architecture

• CavaGLSL no longer multiply-inherits from Gtk::GLArea; it composes a private GLArea child widget
• Factory returns std::unique_ptr<AModule> instead of raw new
• Added doAction() / actionMap_ support to the GLSL frontend
• Encapsulated delay arithmetic in CavaBackend::AdaptiveDelay

Thread Safety & Correctness

• Backend worker threads now emit waybar::SafeSignal instead of raw sigc::signal, marshalling events safely to the GTK main thread
• Fixed critical data race between loadConfig() and out_thread_ via std::recursive_mutex + atomic shutdown_ flag
• Fixed audio_raw shallow-copy / use-after-free in GLSL frontend by introducing a deep-copy AudioRaw payload
• loadConfig() is now fully exception-safe: validates into a temporary new_prm, tears down old state, then runs risky init inside a try block. On failure freeBackend() scrubs partial allocations and the exception is rethrown
• Destructor now waits up to 100 ms for the blocking read_thread_ to exit before cleanup
• isSilent() now acquires audio_data_.lock
• Replaced recursive doUpdate() with iteration
• Guarded audio_raw_clean() against uninitialized state
• Fixed format-icons underflow and cava_config buffer overflow
• Backend stores Json::Value by value, eliminating dangling references on Waybar config reload
• Frontends cache config safely and refresh on runtime changes
• Fixed signed-char icon lookup bug (unsigned char cast before getIcon)
• Fixed unbounded Json::Value growth by performing lookups through a const reference
• Worker threads now catch const std::exception& instead of only std::runtime_error

Resource Management & OpenGL Robustness

• Persisted VBO/IBO/VAO IDs as members; explicit destructor calls glDelete*
• Fixed shader info-log crash (null pointer write)
• Cached inputTexture uniform location instead of querying every frame
• Zero-initialized gradient color stack array and clamped count to [0, 8]
• Fixed shader time uniform integer division bug (now uses float arithmetic)
• Added explicit glBindVertexArray(vao_) in onRender()
• Runtime changes to bar width, spacing, colors, and gradients are now detected independently of dimension/shader changes and re-uploaded via initSurface()
• Clamped negative gradient_count_ to 0 before glUniform1i

Testing

• Compiled with GCC 15 / -std=c++20
• Runtime tested with PulseAudio input, FIFO input, and GLSL output
• Graceful shutdown verified even when the input backend is blocked in a system call

Follow-up work (out of scope)

Item	Reason
Split singleton config API	Requires changing the public inst() contract and deciding how multi-module config reloading should behave. Best handled in a standalone PR.

Related issues

Fixes style violations, thread-safety bugs, and resource leaks identified in the cava module audit.

[Alexays]

LGTM, yes if you can add CONVENTION.md that’ll be useful!

[LukashonakV]

Hi @Alexays ,
Finally, CONVENTION.md has been incorporated into CONTRIBUTING.md to reduce the number of separate files and keep common information in one place.

[LukashonakV]

@Alexays,
man page is also updated

[Alexays]

Thanks!

[qbe]

"Factory returns std::unique_ptr instead of raw new"

I cannot figure out how this is an improvement, especially since the smart pointer is immediately release()ed in the factory.

The whole patch being a single commit was also "fun" to parse through.

[LukashonakV]

Dear @qbe,
You can agree or disagree. As I already mentioned, I'm not fully in agreement with AI. But if we're talking about
return waybar::modules::cava::getModule(id, config_[name]).release()
Yes, there are two points that don't harm anything and potentially bring some visibility and exception safety.

It was a huge list of points that could/couldn't have been improved. So in such a case, how many commits should be provided here? And each such improvement might lead to the next loop of new potential points.

I think it's good form to have a rule: if you criticize something, suggest something. Otherwise, we end up in a polemic without any progress toward improvement

[qbe]

I did not want to incite an argument, I tried to fix some merge conflicts for #4735 and could not figure out why the module pointer would be wrapped into a smart pointer, only to be immediately turned back into a normal pointer again (and any justification for this could be relevant for my dynamic loading work).

I then tried to find justification for this in the commit message, but since all of the code changes were done in a single commit, the only context I could find was the line I quoted above, which does not address my question, which is why I asked You about it.

IMHO doing separate Ideas in separate commits is just good, common practice (I could have formulated that point in a nicer way tho)

My suggested improvement would be, therefore, to pick apart the massive commit into one commit per Idea, and to provide justification in the commit messages for every single one. I can't force you to do this rework (and I don't want to force anyone to do anything), but I think doing proper single-Idea commits encourages better software development and helps everyone else to read your code.

As for now, I would be happy with any explanation for return waybar::modules::cava::getModule(id, config_[name]).release() (other than "it does not seem to harm anything", because "it is inconsistent with the rest of the codebase" is "harm" when it comes to maintainability)