From 93a94ea838db7328f692fc4fdcc5949249383772 Mon Sep 17 00:00:00 2001 From: Brackyt <60280126+Brackyt@users.noreply.github.com> Date: Wed, 28 Jan 2026 23:54:54 +0100 Subject: [PATCH] feat: bug fixes and optimizations - fix double-free - safe int/float parse - grid division guard - list string allocations - label vector reserve - skip second layout pass for hstack/vstack --- lib/ThemeEngine/include/BasicElements.h | 22 +++++++++++++-- lib/ThemeEngine/include/LayoutElements.h | 36 +++++++++++++----------- lib/ThemeEngine/include/ThemeManager.h | 32 +++++++++++++++++++++ lib/ThemeEngine/src/BasicElements.cpp | 26 +++++++++++++++-- lib/ThemeEngine/src/ThemeManager.cpp | 3 -- 5 files changed, 94 insertions(+), 25 deletions(-) diff --git a/lib/ThemeEngine/include/BasicElements.h b/lib/ThemeEngine/include/BasicElements.h index bd3922c4..8d32adc5 100644 --- a/lib/ThemeEngine/include/BasicElements.h +++ b/lib/ThemeEngine/include/BasicElements.h @@ -11,6 +11,22 @@ namespace ThemeEngine { +// Safe integer parsing (no exceptions) +inline int parseIntSafe(const std::string& s, int defaultVal = 0) { + if (s.empty()) return defaultVal; + char* end; + long val = strtol(s.c_str(), &end, 10); + return (end != s.c_str()) ? static_cast(val) : defaultVal; +} + +// Safe float parsing (no exceptions) +inline float parseFloatSafe(const std::string& s, float defaultVal = 0.0f) { + if (s.empty()) return defaultVal; + char* end; + float val = strtof(s.c_str(), &end); + return (end != s.c_str()) ? val : defaultVal; +} + // --- Container --- class Container : public UIElement { protected: @@ -263,8 +279,8 @@ class ProgressBar : public UIElement { std::string valStr = context.evaluatestring(valueExpr); std::string maxStr = context.evaluatestring(maxExpr); - int value = valStr.empty() ? 0 : std::stoi(valStr); - int maxVal = maxStr.empty() ? 100 : std::stoi(maxStr); + int value = parseIntSafe(valStr, 0); + int maxVal = parseIntSafe(maxStr, 100); if (maxVal <= 0) maxVal = 100; float ratio = static_cast(value) / static_cast(maxVal); @@ -367,7 +383,7 @@ class BatteryIcon : public UIElement { if (!isVisible(context)) return; std::string valStr = context.evaluatestring(valueExpr); - int percentage = valStr.empty() ? 0 : std::stoi(valStr); + int percentage = parseIntSafe(valStr, 0); std::string colStr = context.evaluatestring(colorExpr); uint8_t color = Color::parse(colStr).value; diff --git a/lib/ThemeEngine/include/LayoutElements.h b/lib/ThemeEngine/include/LayoutElements.h index fb58877e..53841430 100644 --- a/lib/ThemeEngine/include/LayoutElements.h +++ b/lib/ThemeEngine/include/LayoutElements.h @@ -85,7 +85,11 @@ class HStack : public Container { // Add child's own Y offset to the calculated position childY += childYOffset; - child->layout(context, currentX, childY, childW, childH); + // Only do second layout pass if position changed from first pass + int firstPassY = child->getAbsY(); + if (childY != firstPassY) { + child->layout(context, currentX, childY, childW, childH); + } currentX += childW + spacing; availableW -= (childW + spacing); if (availableW < 0) availableW = 0; @@ -168,7 +172,11 @@ class VStack : public Container { // Add child's own X offset to the calculated position childX += childXOffset; - child->layout(context, childX, currentY, childW, childH); + // Only do second layout pass if position changed from first pass + int firstPassX = child->getAbsX(); + if (childX != firstPassX) { + child->layout(context, childX, currentY, childW, childH); + } currentY += childH + spacing; availableH -= (childH + spacing); if (availableH < 0) availableH = 0; @@ -211,8 +219,10 @@ class Grid : public Container { if (children.empty()) return; - int availableW = absW - 2 * padding - (columns - 1) * colSpacing; - int cellW = availableW / columns; + // Guard against division by zero + int cols = columns > 0 ? columns : 1; + int availableW = absW - 2 * padding - (cols - 1) * colSpacing; + int cellW = availableW / cols; int availableH = absH - 2 * padding; int row = 0, col = 0; @@ -228,7 +238,7 @@ class Grid : public Container { if (childH > maxRowHeight) maxRowHeight = childH; col++; - if (col >= columns) { + if (col >= cols) { col = 0; row++; currentY += maxRowHeight + rowSpacing; @@ -587,15 +597,7 @@ class TabBar : public Container { // Draw selection indicator if (showIndicator && !children.empty()) { std::string selStr = context.evaluatestring(selectedExpr); - int selectedIdx = 0; - if (!selStr.empty()) { - // Try to parse as number - try { - selectedIdx = std::stoi(selStr); - } catch (...) { - selectedIdx = 0; - } - } + int selectedIdx = parseIntSafe(selStr, 0); if (selectedIdx >= 0 && selectedIdx < static_cast(children.size())) { UIElement* tab = children[selectedIdx]; @@ -686,9 +688,9 @@ class ScrollIndicator : public UIElement { std::string totalStr = context.evaluatestring(totalExpr); std::string visStr = context.evaluatestring(visibleExpr); - float position = posStr.empty() ? 0 : std::stof(posStr); - int total = totalStr.empty() ? 1 : std::stoi(totalStr); - int visible = visStr.empty() ? 1 : std::stoi(visStr); + float position = parseFloatSafe(posStr, 0.0f); + int total = parseIntSafe(totalStr, 1); + int visible = parseIntSafe(visStr, 1); if (total <= visible) { // No need to show scrollbar diff --git a/lib/ThemeEngine/include/ThemeManager.h b/lib/ThemeEngine/include/ThemeManager.h index 1bee3566..6f143ac4 100644 --- a/lib/ThemeEngine/include/ThemeManager.h +++ b/lib/ThemeEngine/include/ThemeManager.h @@ -26,6 +26,7 @@ struct ScreenCache { uint32_t contextHash = 0; // Hash of context data to detect changes bool valid = false; + ScreenCache() = default; ~ScreenCache() { if (buffer) { free(buffer); @@ -33,6 +34,37 @@ struct ScreenCache { } } + // Prevent double-free from copy + ScreenCache(const ScreenCache&) = delete; + ScreenCache& operator=(const ScreenCache&) = delete; + + // Allow move + ScreenCache(ScreenCache&& other) noexcept + : buffer(other.buffer), + bufferSize(other.bufferSize), + screenName(std::move(other.screenName)), + contextHash(other.contextHash), + valid(other.valid) { + other.buffer = nullptr; + other.bufferSize = 0; + other.valid = false; + } + + ScreenCache& operator=(ScreenCache&& other) noexcept { + if (this != &other) { + if (buffer) free(buffer); + buffer = other.buffer; + bufferSize = other.bufferSize; + screenName = std::move(other.screenName); + contextHash = other.contextHash; + valid = other.valid; + other.buffer = nullptr; + other.bufferSize = 0; + other.valid = false; + } + return *this; + } + void invalidate() { valid = false; } }; diff --git a/lib/ThemeEngine/src/BasicElements.cpp b/lib/ThemeEngine/src/BasicElements.cpp index 4f81b0b3..a9ef60bb 100644 --- a/lib/ThemeEngine/src/BasicElements.cpp +++ b/lib/ThemeEngine/src/BasicElements.cpp @@ -126,6 +126,7 @@ void Label::draw(const GfxRenderer& renderer, const ThemeContext& context) { int lineHeight = renderer.getLineHeight(fontId); std::vector lines; + lines.reserve(maxLines); // Pre-allocate to avoid reallocations if (absW > 0 && textWidth > absW && maxLines > 1) { // Logic to wrap text std::string remaining = finalStr; @@ -374,6 +375,13 @@ void List::draw(const GfxRenderer& renderer, const ThemeContext& context) { int itemW = getItemWidth(); int itemH = getItemHeight(); + // Pre-allocate string buffers to avoid repeated allocations + std::string prefix; + prefix.reserve(source.length() + 16); + std::string key; + key.reserve(source.length() + 32); + char numBuf[12]; + // Handle different layout modes if (direction == Direction::Horizontal || layoutMode == LayoutMode::Grid) { // Horizontal or Grid layout @@ -389,9 +397,16 @@ void List::draw(const GfxRenderer& renderer, const ThemeContext& context) { } for (int i = 0; i < count; ++i) { + // Build prefix efficiently: "source.i." + prefix.clear(); + prefix += source; + prefix += '.'; + snprintf(numBuf, sizeof(numBuf), "%d", i); + prefix += numBuf; + prefix += '.'; + // Create item context with scoped variables ThemeContext itemContext(&context); - std::string prefix = source + "." + std::to_string(i) + "."; // Standard list item variables - include all properties for full flexibility std::string nameVal = context.getString(prefix + "Name"); @@ -470,9 +485,16 @@ void List::draw(const GfxRenderer& renderer, const ThemeContext& context) { break; } + // Build prefix efficiently: "source.i." + prefix.clear(); + prefix += source; + prefix += '.'; + snprintf(numBuf, sizeof(numBuf), "%d", i); + prefix += numBuf; + prefix += '.'; + // Create item context with scoped variables ThemeContext itemContext(&context); - std::string prefix = source + "." + std::to_string(i) + "."; // Standard list item variables - include all properties for full flexibility std::string nameVal = context.getString(prefix + "Name"); diff --git a/lib/ThemeEngine/src/ThemeManager.cpp b/lib/ThemeEngine/src/ThemeManager.cpp index 321a1432..2902f5b2 100644 --- a/lib/ThemeEngine/src/ThemeManager.cpp +++ b/lib/ThemeEngine/src/ThemeManager.cpp @@ -55,9 +55,6 @@ UIElement* ThemeManager::createElement(const std::string& id, const std::string& return nullptr; } -// Parse integer safely - returns 0 on error -static int parseIntSafe(const std::string& val) { return static_cast(std::strtol(val.c_str(), nullptr, 10)); } - void ThemeManager::applyProperties(UIElement* elem, const std::map& props) { const auto elemType = elem->getType();