tech-debt: tiers 2+3 refactoring#64
Open
sshcrack wants to merge 65 commits into
Open
Conversation
- Added virtual destructors to Scene, PostProcessingEffect, TransitionEffect, Post
- Rewrote PropertyMacros.h to use std::make_shared (avoids {} forwarding issue)
- Removed void(*)(T*) custom-deleter from unique_ptr return types in factory methods
- Converted all {new T(), [](T*){}} and unique_ptr<T, void(*)(T*)>(new T(), ...)
patterns to std::make_unique<T>()
- Fixed PixelJoint ScrapedPost internal types
- Fixed all create_scenes/create_effects/create_transitions/create_image_providers
factory methods across ~50 plugin files
- Removed unused deleteWrapper/deleteScene lambdas
sshcrack
commented
Jun 18, 2026
sshcrack
left a comment
sshcrack
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review
This is a well-structured refactoring PR that consolidates duplicate code, modernizes C++ usage, improves CI/CD, and tightens security/safety. The extraction of shared color utilities, the apply_pixel_loop template, and the helper functions in scene_management.cpp all reduce code duplication meaningfully. The switch from rand() to <random>, from unique_ptr with custom deleters to make_unique/shared_ptr, and from file(GLOB_RECURSE) to explicit source lists are all good improvements.
I found one bug that will break the desktop app's ability to turn the matrix on/off during shutdown and window iconify.
…ath resolution - Add DEB packaging with debconf configuration, systemd service, and FHS layout - Rename binary from main to led-matrix for service ExecStart consistency - Fix matrix plugin loader to check PLUGIN_DIR env var, then dev plugins/ dir, then FHS path (/usr/lib/led-matrix/plugins/) - matches desktop loader - Set WorkingDirectory=/var/lib/led-matrix in systemd unit so images/ working dir resolves under data directory - Fix Shadertoy plugin custom_shader_dir() to use LED_MATRIX_DATA_DIR instead of get_exec_dir() /data/custom_shaders - Remove stale install rule from shared/matrix/CMakeLists.txt that duplicated the root CMakeLists.txt's install to wrong path - Fix Countdown/WeatherOverview plugin font install destinations - Remove dead countdown_font_dir constant with old path - Rewrite update_service.sh for dpkg -i workflow - Rewrite install_led_matrix.sh for .deb download + dpkg install - Delete obsolete scripts/release.ps1
Migrate to DEB packaging with FHS paths and fix plugin loader path resolution
…897→3 files) - WeatherScene.cpp split into WeatherScene.cpp (core dispatch), weather_rendering.cpp (cloud/lightning/fog/aurora/rainbow/utilities), weather_particles.cpp (rain/snow/shooting-stars), weather_cache.cpp (icon download/cache management) - SortingVisualizerScene.cpp split into SortingVisualizerScene.cpp (core+render), sorting_simple.cpp (bubble/selection/insertion/cocktail/gnome), sorting_complex.cpp (shell/quick/heap/comb/merge) - tier2: fix vcpkg_features.cmake JSON parsing (CMake 4.x MEMBER syntax) - fix: add missing consts.h include to update_routes.cpp, pass .get() for shared_ptr in server.cpp - fix: remove fs::create_directories side effect from GET /api/custom-assets/:type - fix: add /uploads/:filename route for uploaded file serving - fix: wire CollectionProvider upload to /pixeljoint/upload_img endpoint
…and docs - build_upload.sh: remove --delete flag, sync to /usr/ not root - canvas.cpp: add LED_MATRIX_SHARE_DIR fallback guard - desktop/main.cpp: update g_should_turn_off_on_exit on toggle, replace std::async with std::thread - PixelJoint ImageScene: convert remaining custom-deleter unique_ptrs - WeatherParser/WeatherScene: convert localtime() to localtime_r() - PropertyDemoScene: replace C-style casts with static_cast - AGENTS.md, README.md: update CI docs, binary name, DEB install instructions
…cate HTTP) - post_processing_routes.cpp: read params from POST body not query string - main.cpp: remove duplicate shutdown_matrix call and stale globals - CMakeLists.txt: remove undefined LIB_NAMES variable - data.h: remove dead from_json declarations for raw pointers
…Layout useMatch, CI glob, dead code
…60 factor without rescaling)
…tains(), dead code, missing locks, type safety
…tale [ ! -f ] guard)
…ardcode, sudo removal, emulator render, scene exclusion, dangling ref, TOCTOU, hue normalization, thread ordering, gitignore, preview_gen install, cmake version consistency
…web, packaging, and CI - Use-after-free: reorder cleanup in main.cpp (delete config after UpdateManager stop) - Update release asset filter from .tar.gz to .deb (update_routes.cpp) - Quote Spotify credentials in debconf-generated EnvironmentFile (postinst) - Guard dpkg -i with DEBIAN_FRONTEND=noninteractive (update_service.sh) - Make debconf config idempotent: skip prompts when answers already exist - Update config.json version field on upgrades via jq (postinst) - Consolidate db_go calls into single call at end (config) - Replace literal 'false' with '' in build-matrix.yml artifact paths - Add RestartSec=5 to systemd unit (led-matrix.service.in) - Guard vcpkg_features.cmake against empty features object - Use specific glob build/led-matrix-*.deb in build_upload.sh - Fix postrm to remove specific files instead of rm -rf whole dir - Fix FallingSandScene random direction bias (== 0 check) - Change showMainWindow to std::atomic<bool> for thread safety - Add try-catch around WebsocketClient::shared_from_this() (bad_weak_ptr) - Flush D-Bus connection after send in single_instance_manager - Disable noUnusedLocals/Parameters in tsconfig.json - Add try/catch + error toast to confirmUpload in AssetManager - Move ErrorBoundary to wrap Layout, add programmatic reset - Remove pre-existing unused imports and dead code references Fixes identified during comprehensive PR review with 4 parallel subagents covering build system, plugins, daemon/desktop/shared libs, and web frontend.
…ns, web, packaging, CI - build: fix CI JSON syntax, upload path literal false, install script DEB regex, vcpkg guards, DEBIAN_FRONTEND for automated updates - daemon: shutdown order (UpdateManager first), .tar.gz+.deb dual matching, POST body+query fallback, separate error_code vars, upload path traversal guard - desktop: restore atomic<bool> for showMainWindow, shared_from_this() try-catch, dbus_connection_flush, General mutex locking, waitpid -1 guard - shared: portable localtime_r/localtime_s for MSVC builds - plugins: PingPongGameScene backward-compat (revert defaults+60f mul), SnakeGameScene div-by-zero guard, WeatherScene div-by-zero guards, weather_particles render pos fix, particle init on empty vector - web: ErrorBoundary wraps Layout, setState reload, restore upload error handling, ESM-compatible __dirname - packaging: restore SPOTIFY_ID quotes, update config.json on upgrade, debconf only ask unset questions, postrm targeted purge
…ing, debconf purge, path fixes, misc
…data races, misc bugs
Build Artifacts
|
Issue 1: Console window flashing — _popen() spawns cmd.exe which creates a visible console in a GUI app. Replaced with CreateProcessA + CREATE_NO_WINDOW via new run_command_and_get_output() utility. Issue 2: Access violation on close — VideoStreamEngine::stop() restores on_status_change callback after joining threads. SpotifyMVDesktop destructor cleared callbacks before stop(), so stop() restored the lambda (capturing his) which then dangled when ~VideoStreamEngine ran via unique_ptr cleanup. Fixed by clearing callbacks AFTER stop().
… Clang on Windows The newer Windows SDK (10.0.26100.0) dropped XP-era definitions that portaudio relies on when _WIN32_WINNT=0x0501. Override to 0x0601 (Windows 7) from the parent CMakeLists.txt to fix desktop-windows builds with Clang.
…top GUI Add a collapsible Debug Info section showing: - Total tracks played, errors, and engine swap counters - Search and crossfade active indicators - Status overview bar with colored state label - Engine status cards side-by-side (current vs pending) - Crossfade progress bar with elapsed time - Track IDs and URLs for both engines
VideoStreamEngine::start() calls stop() internally, which nulls out on_first_frame_ready and on_status_change. Since these were set before the search thread called engine->start(), they got wiped and on_pending_first_frame() never fired — the engines were never swapped, current_engine_ remained idle, and no frames reached the matrix. Fix by re-setting the callbacks immediately after start() returns. By the time the processing thread fires on_first_frame_ready (after network download and fast-clip playback), the callbacks are already re-established.
…d-song chunk alignment - Shutdown crash: delete ConfigManager before destroying plugins so ~ConfigManager::saveConfig() iterates live plugin pointers. - Crossfade warnings: skip blend when old_last_frame_ is empty (first track after app launch). - Freeze-frame fallback: keep old_last_frame_ alive after crossfade; re-send it when the engine is between chunks so the matrix never goes dark during transitions. - Mid-song chunk alignment: download_and_process_chunk() accepts start_sec_override; the initial full chunk starts at the seek point instead of the chunk boundary, giving a full chunk_duration_sec_ of play time before the next prefetch is needed.
Extract a PluginLoader<PluginBase> header-only template into shared/common/, replacing two near-identical plugin loader implementations (matrix: 213 lines, desktop: 281 lines). The template handles directory scanning, dlopen/LoadLibraryEx (platform ifdef written once), symbol lookup, error logging, and destroy lifecycle. Each binary's PluginManager becomes a thin derived class providing only its plugin base type and own singleton. -412 lines net
Add REGISTER_PLUGIN(Name, Type) and REGISTER_PLUGIN_CUSTOM_DESTROY macros to shared/common/plugin_macros.h that generate the extern C create/destroy pair. Replace 21 manual extern C blocks across all 17 matrix plugins and 4 desktop plugins. Remove redundant .h declarations from 4 plugin headers. -132 lines net
Extract all init/teardown logic from main.cpp into a Daemon class whose members own their resources via unique_ptr. Destruction is automatic in reverse-declaration order — the four manual teardown paths are gone. src_matrix/main.cpp: 395 -> 8 lines (no change to behaviour) src_matrix/daemon.h — Daemon class declaration src_matrix/daemon.cpp — init ordering, safe teardown, emulator helpers
Split src_matrix/matrix_control/canvas.cpp into three deep modules:
- SceneScheduler — weighted scene selection, transition resolution,
exclusion/desktop filtering (stateless, no globals)
- SceneRenderer — render_scene_phase + render_fallback (injected matrix)
- TransitionEngine — render_transition_phase + crossfade blending
(injected matrix)
CanvasCoordinator owns the loop, delegates to all three, and manages
the internal forced_scene_ state as a member instead of an out-param.
canvas.cpp: 408 -> 129 lines
+3 new files: SceneScheduler.{h,cpp}, SceneRenderer.{h,cpp},
TransitionEngine.{h,cpp}
hardware.cpp uses CanvasCoordinator directly instead of manual canvas
pointer management + update_canvas().
- Create TimeSource interface + WallClock in shared/common/timesource/ - SceneRenderer: injected TimeSource* replaces GetTimeInMillis() calls; injected PostProcessor* replaces Constants::global_post_processor - TransitionEngine: injected TimeSource*, PostProcessor*, TransitionManager* replace global accesses - CanvasCoordinator: receives all deps and forwards them - hardware.cpp: creates WallClock, passes globals at construction time Remaining leaks (not yet addressed): - #ifdef ENABLE_EMULATOR — three places still cast to EmulatorMatrix - config, Server::, interrupt_received, exit_canvas_update — still globals throughout CanvasCoordinator
…tion, no globals in render/canvas layer
Bump version from 0.1.6 to 0.1.7. Includes: - Track active_buffer_ in created_frames_ for proper cleanup - Fix uninitialized shared_pixel_mapper_ (UB + memory leak)
…sing <optional> includes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements medium and low priority refactoring from plans/tier2-medium.md and plans/tier3-low.md.