diff --git a/docs/changelog.md b/docs/changelog.md index 309534d4..2c5cef32 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -6,6 +6,40 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## [Unreleased] +### Fixed +- **Button Highlighting Bug - Remove Broken Static Cache (2026-01-10)** + - Fixed buttons incorrectly showing highlighted/yellow state + - **Root Cause**: The static tile cache optimization in `Layer::Rectangle()` was fundamentally broken. Static variables persisted across all calls, causing stale cache values to be used when rendering buttons with different textures. + - **Solution**: Removed the static cache optimization entirely. Each `Rectangle()` call now properly sets the tile and origin, ensuring correct texture rendering. + - **Files modified**: `term/layer.cc` + +- **Button Highlighting Bug with Lazy Texture Loading (2026-01-09)** + - Fixed buttons incorrectly showing highlighted state when they shouldn't be + - **Root Cause**: The lazy texture loading introduced in the memory optimization commit conflicted with the static tile cache in `Layer::Rectangle()`. When textures were loaded on-demand, the cache could cause incorrect texture rendering. + - **Solution**: Added `PreloadAllTextures()` function that loads all textures at startup, ensuring consistent Pixmap values before any rendering occurs. + - **Files modified**: `term/term_view.cc`, `term/term_view.hh` + +### Added +- **Performance: Object Pool System for Memory Efficiency (2026-01-09)** + - Created `src/core/object_pool.hh` with thread-safe object pooling templates + - `ObjectPool`: Reusable object pool with configurable max size (default 64) + * Thread-safe acquire/release with mutex protection + * Pre-allocation support via `reserve()` method + * Automatic cleanup of pooled objects on destruction + - `PooledObject`: RAII wrapper for automatic return to pool + - `BufferPool`: Specialized pool for fixed-size char buffers + - Reduces allocation overhead and memory fragmentation on resource-constrained devices + - **Files added**: `src/core/object_pool.hh` + - **Target**: Raspberry Pi CM5 with 2GB RAM optimization + +### Improved +- **Performance: Settings Pointer Caching (2026-01-09)** + - Cached `term->GetSettings()` calls at function start in hot paths + - `zone/dialog_zone.cc`: `CreditCardDialog::Init()` - cached 4 consecutive GetSettings() calls + - `zone/payout_zone.cc`: `EndDayZone::Render()` - cached settings pointer for multiple accesses + - Reduces function call overhead on resource-constrained devices + - **Files modified**: `zone/dialog_zone.cc`, `zone/payout_zone.cc` + ### Added - **Testing: Comprehensive Test Suite Expansion (2026-01-07)** - Added 26 new test cases covering time/date operations and error handling diff --git a/loader/loader_main.cc b/loader/loader_main.cc index 28977e8c..aff33eba 100644 --- a/loader/loader_main.cc +++ b/loader/loader_main.cc @@ -410,6 +410,7 @@ int SetupConnection(const std::string &socket_file) else if (bind(dev, reinterpret_cast(&server_adr), SUN_LEN(&server_adr)) < 0) { logmsg(LOG_ERR, "Failed to bind socket '%s'", SOCKET_FILE.c_str()); + close(dev); // Fix socket leak on bind failure } else { diff --git a/main/hardware/printer.cc b/main/hardware/printer.cc index a4f41e0a..d41139b5 100644 --- a/main/hardware/printer.cc +++ b/main/hardware/printer.cc @@ -26,6 +26,7 @@ #include "src/utils/vt_logger.hh" #include "safe_string_utils.hh" #include "src/utils/cpp23_utils.hh" +#include "src/core/thread_pool.hh" #include #include @@ -287,6 +288,137 @@ int Printer::Close() return 0; } +/**** + * CloseAsync: Non-blocking version of Close() for use when UI responsiveness + * is critical. Spawns the print job to a background thread. + * Note: Only works for socket and LPD printing where the temp file can be + * safely copied and processed asynchronously. + ****/ +void Printer::CloseAsync() +{ + FnTrace("Printer::CloseAsync()"); + + // Close the temp file handle first + if (temp_fd > 0) + close(temp_fd); + temp_fd = -1; + + // For parallel printing, we still use synchronous (it already forks) + // For file/email, use sync since they're local operations + if (target_type == TARGET_PARALLEL || target_type == TARGET_FILE || + target_type == TARGET_EMAIL) + { + // Fall back to sync for these + switch (target_type) + { + case TARGET_PARALLEL: + ParallelPrint(); + break; + case TARGET_FILE: + FilePrint(); + break; + case TARGET_EMAIL: + EmailPrint(); + break; + default: + break; + } + unlink(temp_name.Value()); + temp_name.Set(""); + return; + } + + // For socket and LPD, run async + // Capture temp file name for async operation + std::string temp_file = temp_name.Value(); + std::string target_str = target.Value(); + int port = port_no; + int type = target_type; + + temp_name.Set(""); // Clear so printer can be reused + + // Queue the print job to the thread pool + vt::ThreadPool::instance().enqueue_detached( + [temp_file, target_str, port, type]() { + vt::Logger::debug("Async print starting: {} -> {}:{}", temp_file, target_str, port); + + if (type == TARGET_SOCKET) + { + // Socket printing logic (duplicated to avoid this pointer issues) + ssize_t bytesread; + ssize_t byteswritten = 1; + char buffer[4096]; + struct hostent *he; + struct sockaddr_in their_addr; + + he = gethostbyname(target_str.c_str()); + if (he == nullptr) + { + vt::Logger::error("Async SocketPrint: Failed to resolve host '{}'", target_str); + unlink(temp_file.c_str()); + return; + } + + int sockfd = socket(AF_INET, SOCK_STREAM, 0); + if (sockfd == -1) + { + vt::Logger::error("Async SocketPrint: Failed to create socket"); + unlink(temp_file.c_str()); + return; + } + + their_addr.sin_family = AF_INET; + their_addr.sin_port = htons(static_cast(port)); + their_addr.sin_addr = *((struct in_addr *)he->h_addr); + memset(&(their_addr.sin_zero), '\0', 8); + + if (connect(sockfd, (struct sockaddr *)&their_addr, sizeof(struct sockaddr)) == -1) + { + vt::Logger::error("Async SocketPrint: Failed to connect to {}:{}", target_str, port); + close(sockfd); + unlink(temp_file.c_str()); + return; + } + + int fd = open(temp_file.c_str(), O_RDONLY, 0); + if (fd <= 0) + { + vt::Logger::error("Async SocketPrint: Failed to open temp file '{}'", temp_file); + close(sockfd); + unlink(temp_file.c_str()); + return; + } + + bytesread = read(fd, buffer, sizeof(buffer)); + while (bytesread > 0 && byteswritten > 0) + { + byteswritten = write(sockfd, buffer, static_cast(bytesread)); + bytesread = read(fd, buffer, sizeof(buffer)); + } + + close(fd); + close(sockfd); + vt::Logger::info("Async SocketPrint: Successfully printed to {}:{}", target_str, port); + } + else if (type == TARGET_LPD) + { + // LPD printing + char cmd[512]; + snprintf(cmd, sizeof(cmd), "cat %s | /usr/bin/lpr -P%s", + temp_file.c_str(), target_str.c_str()); + int result = system(cmd); + if (result != 0) + vt::Logger::error("Async LPDPrint: Command failed with code {}", result); + else + vt::Logger::info("Async LPDPrint: Successfully sent to {}", target_str); + } + + // Clean up temp file + unlink(temp_file.c_str()); + } + ); +} + /**** * ParallelPrint: I've been experiencing severe slowdowns with parallel printing. * Apparently, we have to wait until the whole job is done. So I'm spawning @@ -413,6 +545,13 @@ int Printer::SocketPrint() return 1; } + // Set socket timeouts (5 seconds for printer connections) + struct timeval timeout; + timeout.tv_sec = 5; + timeout.tv_usec = 0; + setsockopt(sockfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)); + setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)); + their_addr.sin_family = AF_INET; // host byte order their_addr.sin_port = htons(port_no); // short, network byte order their_addr.sin_addr = *((struct in_addr *)he->h_addr); @@ -423,6 +562,7 @@ int Printer::SocketPrint() vt::Logger::error("SocketPrint: Failed to connect to {}:{}", target.Value(), port_no); perror("connect"); fprintf(stderr, "Is vt_print running?\n"); + close(sockfd); return 1; } diff --git a/main/hardware/printer.hh b/main/hardware/printer.hh index 2fb94148..1448c3a8 100644 --- a/main/hardware/printer.hh +++ b/main/hardware/printer.hh @@ -157,6 +157,7 @@ public: virtual int SetTitle(const std::string &title); virtual int Open(); virtual int Close(); + virtual void CloseAsync(); // Non-blocking version - fire and forget virtual int ParallelPrint(); virtual int LPDPrint(); virtual int SocketPrint(); // print to TCP socket diff --git a/src/core/data_file.hh b/src/core/data_file.hh index e27362d8..cba45afd 100644 --- a/src/core/data_file.hh +++ b/src/core/data_file.hh @@ -154,6 +154,50 @@ public: KeyValueInputFile() = default; explicit KeyValueInputFile(int fd); explicit KeyValueInputFile(const std::string &filename); + + // RAII destructor - ensures file is closed + ~KeyValueInputFile() { Close(); } + + // Delete copy operations (prevent double-close) + KeyValueInputFile(const KeyValueInputFile&) = delete; + KeyValueInputFile& operator=(const KeyValueInputFile&) = delete; + + // Move operations + KeyValueInputFile(KeyValueInputFile&& other) noexcept + : filedes(other.filedes) + , bytesread(other.bytesread) + , keyidx(other.keyidx) + , validx(other.validx) + , buffidx(other.buffidx) + , comment(other.comment) + , getvalue(other.getvalue) + , delimiter(other.delimiter) + , buffer(std::move(other.buffer)) + , inputfile(std::move(other.inputfile)) + { + other.filedes = -1; + } + + KeyValueInputFile& operator=(KeyValueInputFile&& other) noexcept + { + if (this != &other) + { + Close(); + filedes = other.filedes; + bytesread = other.bytesread; + keyidx = other.keyidx; + validx = other.validx; + buffidx = other.buffidx; + comment = other.comment; + getvalue = other.getvalue; + delimiter = other.delimiter; + buffer = std::move(other.buffer); + inputfile = std::move(other.inputfile); + other.filedes = -1; + } + return *this; + } + bool Open(); bool Open(const std::string &filename); [[nodiscard]] bool IsOpen() const noexcept; @@ -179,6 +223,36 @@ public: KeyValueOutputFile() = default; explicit KeyValueOutputFile(int fd); explicit KeyValueOutputFile(const std::string &filename); + + // RAII destructor - ensures file is closed + ~KeyValueOutputFile() { Close(); } + + // Delete copy operations (prevent double-close) + KeyValueOutputFile(const KeyValueOutputFile&) = delete; + KeyValueOutputFile& operator=(const KeyValueOutputFile&) = delete; + + // Move operations + KeyValueOutputFile(KeyValueOutputFile&& other) noexcept + : filedes(other.filedes) + , delimiter(other.delimiter) + , outputfile(std::move(other.outputfile)) + { + other.filedes = -1; + } + + KeyValueOutputFile& operator=(KeyValueOutputFile&& other) noexcept + { + if (this != &other) + { + Close(); + filedes = other.filedes; + delimiter = other.delimiter; + outputfile = std::move(other.outputfile); + other.filedes = -1; + } + return *this; + } + int Open(); int Open(const std::string &filename); [[nodiscard]] bool IsOpen() const noexcept; diff --git a/src/core/list_utility.hh b/src/core/list_utility.hh index c2650820..9e86ee7d 100644 --- a/src/core/list_utility.hh +++ b/src/core/list_utility.hh @@ -30,11 +30,12 @@ class SList { T *list_head{nullptr}; T *list_tail{nullptr}; + int cached_count{0}; // Cached count for O(1) Count() public: // Constructors SList() = default; - explicit SList(T *item) : list_head(item), list_tail(item) {} + explicit SList(T *item) : list_head(item), list_tail(item), cached_count(item ? 1 : 0) {} // Delete copy operations (use move instead) SList(const SList&) = delete; @@ -44,9 +45,11 @@ public: SList(SList&& other) noexcept : list_head(other.list_head) , list_tail(other.list_tail) + , cached_count(other.cached_count) { other.list_head = nullptr; other.list_tail = nullptr; + other.cached_count = 0; } SList& operator=(SList&& other) noexcept @@ -56,8 +59,10 @@ public: Purge(); list_head = other.list_head; list_tail = other.list_tail; + cached_count = other.cached_count; other.list_head = nullptr; other.list_tail = nullptr; + other.cached_count = 0; } return *this; } @@ -88,6 +93,7 @@ public: list_tail = item; list_head = item; + ++cached_count; return 0; } @@ -104,6 +110,7 @@ public: else list_head = item; list_tail = item; + ++cached_count; return 0; } @@ -117,6 +124,7 @@ public: item->next = node->next; node->next = item; + ++cached_count; return 0; } @@ -130,6 +138,7 @@ public: delete tmp; } list_tail = nullptr; + cached_count = 0; } int Remove(T *node) noexcept @@ -147,18 +156,35 @@ public: if (list_tail == node) list_tail = p; node->next = nullptr; + --cached_count; return 0; } return 1; } + /** + * @brief Removes item from list AND deletes it. + * @param item The item to remove and delete + * @return 0 on success, 1 on failure + * @note Use this when the SList owns the item's memory. + * Use Remove() alone when ownership is transferred elsewhere. + */ + int RemoveAndDelete(T *item) noexcept + { + FnTrace("SList::RemoveAndDelete()"); + if (item == nullptr) + return 1; + + int result = Remove(item); + if (result == 0) + delete item; + return result; + } + [[nodiscard]] int Count() const noexcept { FnTrace("SList::Count()"); - int count = 0; - for (T *n = list_head; n != nullptr; n = n->next) - ++count; - return count; + return cached_count; // O(1) instead of O(n) } [[nodiscard]] T *Index(int i) noexcept @@ -182,6 +208,7 @@ class DList { T *list_head{nullptr}; T *list_tail{nullptr}; + int cached_count{0}; // Cached count for O(1) Count() T *InternalSort(T *list, int (*cmp)(T *, T *)) noexcept { @@ -265,7 +292,7 @@ class DList public: // Constructors DList() = default; - explicit DList(T *item) : list_head(item), list_tail(item) {} + explicit DList(T *item) : list_head(item), list_tail(item), cached_count(item ? 1 : 0) {} // Delete copy operations DList(const DList&) = delete; @@ -275,9 +302,11 @@ public: DList(DList&& other) noexcept : list_head(other.list_head) , list_tail(other.list_tail) + , cached_count(other.cached_count) { other.list_head = nullptr; other.list_tail = nullptr; + other.cached_count = 0; } DList& operator=(DList&& other) noexcept @@ -287,8 +316,10 @@ public: Purge(); list_head = other.list_head; list_tail = other.list_tail; + cached_count = other.cached_count; other.list_head = nullptr; other.list_tail = nullptr; + other.cached_count = 0; } return *this; } @@ -324,6 +355,7 @@ public: else list_tail = item; list_head = item; + ++cached_count; return 0; } @@ -340,6 +372,7 @@ public: else list_head = item; list_tail = item; + ++cached_count; return 0; } @@ -355,6 +388,7 @@ public: item->next = node->next; node->next->fore = item; node->next = item; + ++cached_count; return 0; } @@ -370,6 +404,7 @@ public: item->fore = node->fore; item->fore->next = item; node->fore = item; + ++cached_count; return 0; } @@ -407,6 +442,7 @@ public: item->fore->next = item->next; item->fore = nullptr; item->next = nullptr; + --cached_count; return 0; } @@ -421,6 +457,25 @@ public: return 1; } + /** + * @brief Removes item from list AND deletes it. + * @param item The item to remove and delete + * @return 0 on success, 1 on failure + * @note Use this when the DList owns the item's memory. + * Use Remove() alone when ownership is transferred elsewhere. + */ + int RemoveAndDelete(T *item) noexcept + { + FnTrace("DList::RemoveAndDelete()"); + if (item == nullptr) + return 1; + + int result = Remove(item); + if (result == 0) + delete item; + return result; + } + void Purge() noexcept { FnTrace("DList::Purge()"); @@ -432,15 +487,13 @@ public: delete tmp; } list_tail = nullptr; + cached_count = 0; } [[nodiscard]] int Count() const noexcept { FnTrace("DList::Count()"); - int count = 0; - for (T *n = list_head; n != nullptr; n = n->next) - ++count; - return count; + return cached_count; // O(1) instead of O(n) } [[nodiscard]] T *Index(int i) noexcept diff --git a/src/core/object_pool.hh b/src/core/object_pool.hh new file mode 100644 index 00000000..f42b08ea --- /dev/null +++ b/src/core/object_pool.hh @@ -0,0 +1,343 @@ +/* + * Copyright ViewTouch, Inc., 1995, 1996, 1997, 1998 + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * object_pool.hh - Object pooling for reduced allocation overhead + * Optimized for resource-constrained systems like Raspberry Pi + */ + +#ifndef VT_OBJECT_POOL_HH +#define VT_OBJECT_POOL_HH + +#include +#include +#include +#include +#include + +namespace vt { + +/** + * @brief A simple, efficient object pool for reducing allocation overhead. + * + * Benefits: + * - Reduces memory fragmentation from frequent new/delete + * - Faster allocation (reuses existing objects) + * - Better cache locality (objects from same pool are nearby) + * - Reduced pressure on system allocator + * + * Usage: + * ObjectPool pool(16); // Pre-allocate 16 objects + * auto obj = pool.acquire(); // Get object from pool + * // ... use obj ... + * pool.release(obj); // Return to pool (or let destructor handle it) + * + * @tparam T The type of object to pool (must be default-constructible) + */ +template +class ObjectPool { +public: + /** + * @brief Construct pool with optional pre-allocation + * @param initial_size Number of objects to pre-allocate (0 = grow on demand) + * @param max_size Maximum pool size (0 = unlimited) + */ + explicit ObjectPool(size_t initial_size = 0, size_t max_size = 0) + : max_pool_size_(max_size) + , total_allocated_(0) + , total_reused_(0) + { + if (initial_size > 0) { + pool_.reserve(initial_size); + for (size_t i = 0; i < initial_size; ++i) { + pool_.push_back(std::make_unique()); + } + } + } + + // Non-copyable, non-movable (owns resources) + ObjectPool(const ObjectPool&) = delete; + ObjectPool& operator=(const ObjectPool&) = delete; + ObjectPool(ObjectPool&&) = delete; + ObjectPool& operator=(ObjectPool&&) = delete; + + ~ObjectPool() = default; + + /** + * @brief Acquire an object from the pool + * @return Pointer to object (caller must call release() when done) + * + * If pool is empty, allocates a new object. + * Object is NOT reset - caller should initialize as needed. + */ + T* acquire() { + std::lock_guard lock(mutex_); + + if (!pool_.empty()) { + T* obj = pool_.back().release(); + pool_.pop_back(); + ++total_reused_; + return obj; + } + + // Pool empty - allocate new + ++total_allocated_; + return new T(); + } + + /** + * @brief Acquire an object and reset it using provided function + * @param reset_func Function to reset/initialize the object + * @return Pointer to reset object + */ + T* acquire(std::function reset_func) { + T* obj = acquire(); + if (obj && reset_func) { + reset_func(*obj); + } + return obj; + } + + /** + * @brief Release an object back to the pool + * @param obj Object to return (must have been acquired from this pool) + * + * If pool is at max_size, object is deleted instead of pooled. + */ + void release(T* obj) { + if (obj == nullptr) return; + + std::lock_guard lock(mutex_); + + // Check if we should pool or delete + if (max_pool_size_ > 0 && pool_.size() >= max_pool_size_) { + delete obj; + return; + } + + pool_.push_back(std::unique_ptr(obj)); + } + + /** + * @brief Get current number of objects in pool + */ + [[nodiscard]] size_t available() const { + std::lock_guard lock(mutex_); + return pool_.size(); + } + + /** + * @brief Get total allocations made (new objects created) + */ + [[nodiscard]] size_t total_allocated() const { + return total_allocated_; + } + + /** + * @brief Get total reuses (objects taken from pool) + */ + [[nodiscard]] size_t total_reused() const { + return total_reused_; + } + + /** + * @brief Get reuse ratio (higher = better pool efficiency) + */ + [[nodiscard]] double reuse_ratio() const { + size_t total = total_allocated_ + total_reused_; + if (total == 0) return 0.0; + return static_cast(total_reused_) / static_cast(total); + } + + /** + * @brief Clear all pooled objects (frees memory) + */ + void clear() { + std::lock_guard lock(mutex_); + pool_.clear(); + } + + /** + * @brief Pre-allocate additional objects + * @param count Number of objects to add to pool + */ + void reserve(size_t count) { + std::lock_guard lock(mutex_); + for (size_t i = 0; i < count; ++i) { + if (max_pool_size_ > 0 && pool_.size() >= max_pool_size_) { + break; + } + pool_.push_back(std::make_unique()); + } + } + +private: + std::vector> pool_; + mutable std::mutex mutex_; + size_t max_pool_size_; + size_t total_allocated_; + size_t total_reused_; +}; + +/** + * @brief RAII wrapper for pooled objects - auto-releases on destruction + * + * Usage: + * ObjectPool pool; + * { + * PooledObject obj(pool); + * obj->doSomething(); + * } // Automatically released back to pool + */ +template +class PooledObject { +public: + explicit PooledObject(ObjectPool& pool) + : pool_(&pool) + , obj_(pool.acquire()) + {} + + PooledObject(ObjectPool& pool, std::function reset_func) + : pool_(&pool) + , obj_(pool.acquire(reset_func)) + {} + + ~PooledObject() { + if (obj_ && pool_) { + pool_->release(obj_); + } + } + + // Move-only + PooledObject(PooledObject&& other) noexcept + : pool_(other.pool_) + , obj_(other.obj_) + { + other.pool_ = nullptr; + other.obj_ = nullptr; + } + + PooledObject& operator=(PooledObject&& other) noexcept { + if (this != &other) { + if (obj_ && pool_) { + pool_->release(obj_); + } + pool_ = other.pool_; + obj_ = other.obj_; + other.pool_ = nullptr; + other.obj_ = nullptr; + } + return *this; + } + + // Non-copyable + PooledObject(const PooledObject&) = delete; + PooledObject& operator=(const PooledObject&) = delete; + + // Accessors + T* get() noexcept { return obj_; } + const T* get() const noexcept { return obj_; } + T* operator->() noexcept { return obj_; } + const T* operator->() const noexcept { return obj_; } + T& operator*() noexcept { return *obj_; } + const T& operator*() const noexcept { return *obj_; } + explicit operator bool() const noexcept { return obj_ != nullptr; } + + /** + * @brief Release ownership without returning to pool + */ + T* release() noexcept { + T* tmp = obj_; + obj_ = nullptr; + pool_ = nullptr; + return tmp; + } + +private: + ObjectPool* pool_; + T* obj_; +}; + +/** + * @brief Fixed-size buffer pool for stack-like allocations + * + * More efficient than ObjectPool for fixed-size buffers (char arrays, etc.) + * Uses a simple free-list instead of vector. + */ +template +class BufferPool { +public: + explicit BufferPool(size_t initial_count = 4, size_t max_count = 32) + : max_buffers_(max_count) + , buffer_count_(0) + { + buffers_.reserve(initial_count); + for (size_t i = 0; i < initial_count; ++i) { + buffers_.push_back(std::make_unique>()); + } + } + + char* acquire() { + std::lock_guard lock(mutex_); + + if (!buffers_.empty()) { + char* buf = buffers_.back()->data(); + buffers_.pop_back(); + return buf; + } + + // Allocate new buffer + auto new_buf = std::make_unique>(); + char* buf = new_buf->data(); + allocated_.push_back(std::move(new_buf)); + ++buffer_count_; + return buf; + } + + void release(char* buf) { + if (buf == nullptr) return; + + std::lock_guard lock(mutex_); + + // Find which allocation this came from + for (auto& alloc : allocated_) { + if (alloc->data() == buf) { + if (buffers_.size() < max_buffers_) { + buffers_.push_back(std::move(alloc)); + } + return; + } + } + // Buffer not from this pool - ignore + } + + [[nodiscard]] size_t available() const { + std::lock_guard lock(mutex_); + return buffers_.size(); + } + + static constexpr size_t buffer_size() { return BufferSize; } + +private: + std::vector>> buffers_; + std::vector>> allocated_; + mutable std::mutex mutex_; + size_t max_buffers_; + size_t buffer_count_; +}; + +} // namespace vt + +#endif // VT_OBJECT_POOL_HH diff --git a/src/core/thread_pool.hh b/src/core/thread_pool.hh new file mode 100644 index 00000000..01cd4e35 --- /dev/null +++ b/src/core/thread_pool.hh @@ -0,0 +1,389 @@ +/* + * Copyright ViewTouch, Inc., 1995, 1996, 1997, 1998 + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * thread_pool.hh - Lightweight thread pool for async I/O operations + * Optimized for resource-constrained systems like Raspberry Pi + */ + +#ifndef VT_THREAD_POOL_HH +#define VT_THREAD_POOL_HH + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace vt { + +/** + * @brief A lightweight, efficient thread pool for async I/O operations. + * + * Designed for resource-constrained systems: + * - Default 2 threads (optimal for RPi with limited cores) + * - Bounded queue to prevent memory exhaustion + * - Graceful shutdown with task completion + * + * Usage: + * auto& pool = ThreadPool::instance(); + * auto future = pool.enqueue([](){ return heavy_io_operation(); }); + * // ... do other work ... + * auto result = future.get(); // blocks until complete + */ +class ThreadPool { +public: + // Singleton instance - use 2 threads by default for RPi + static ThreadPool& instance(size_t num_threads = 2) { + static ThreadPool pool(num_threads); + return pool; + } + + // Delete copy/move operations + ThreadPool(const ThreadPool&) = delete; + ThreadPool& operator=(const ThreadPool&) = delete; + ThreadPool(ThreadPool&&) = delete; + ThreadPool& operator=(ThreadPool&&) = delete; + + ~ThreadPool() { + shutdown(); + } + + /** + * @brief Enqueue a task for async execution + * @param f Function to execute + * @param args Arguments to pass to the function + * @return std::future for the result + * + * Example: + * auto future = pool.enqueue(&save_file, filename, data); + * // ... later ... + * bool success = future.get(); + */ + template + auto enqueue(F&& f, Args&&... args) + -> std::future> + { + using return_type = typename std::invoke_result_t; + + auto task = std::make_shared>( + std::bind(std::forward(f), std::forward(args)...) + ); + + std::future result = task->get_future(); + + { + std::unique_lock lock(queue_mutex_); + + // Don't allow enqueueing after stopping + if (stop_) { + throw std::runtime_error("enqueue on stopped ThreadPool"); + } + + // Bounded queue - wait if full (prevents memory exhaustion) + queue_not_full_.wait(lock, [this] { + return tasks_.size() < max_queue_size_ || stop_; + }); + + if (stop_) { + throw std::runtime_error("enqueue on stopped ThreadPool"); + } + + tasks_.emplace([task]() { (*task)(); }); + } + + condition_.notify_one(); + return result; + } + + /** + * @brief Enqueue a task without caring about the result (fire-and-forget) + * @param f Function to execute + * @param args Arguments to pass + * + * More efficient than enqueue() when you don't need the result. + */ + template + void enqueue_detached(F&& f, Args&&... args) { + auto task = std::bind(std::forward(f), std::forward(args)...); + + { + std::unique_lock lock(queue_mutex_); + + if (stop_) return; + + queue_not_full_.wait(lock, [this] { + return tasks_.size() < max_queue_size_ || stop_; + }); + + if (stop_) return; + + tasks_.emplace(std::move(task)); + } + + condition_.notify_one(); + } + + /** + * @brief Get current queue size (for monitoring) + */ + size_t queue_size() const { + std::lock_guard lock(queue_mutex_); + return tasks_.size(); + } + + /** + * @brief Check if pool is idle (no pending tasks) + */ + bool idle() const { + std::lock_guard lock(queue_mutex_); + return tasks_.empty() && active_tasks_ == 0; + } + + /** + * @brief Wait for all currently queued tasks to complete + */ + void wait_all() { + std::unique_lock lock(queue_mutex_); + all_done_.wait(lock, [this] { + return tasks_.empty() && active_tasks_ == 0; + }); + } + + /** + * @brief Gracefully shutdown the pool (waits for pending tasks) + */ + void shutdown() { + { + std::unique_lock lock(queue_mutex_); + if (stop_) return; + stop_ = true; + } + + condition_.notify_all(); + queue_not_full_.notify_all(); + + for (std::thread& worker : workers_) { + if (worker.joinable()) { + worker.join(); + } + } + } + +private: + explicit ThreadPool(size_t num_threads) + : stop_(false) + , active_tasks_(0) + , max_queue_size_(64) // Bounded queue for memory safety + { + workers_.reserve(num_threads); + + for (size_t i = 0; i < num_threads; ++i) { + workers_.emplace_back([this] { + while (true) { + std::function task; + + { + std::unique_lock lock(queue_mutex_); + + condition_.wait(lock, [this] { + return stop_ || !tasks_.empty(); + }); + + if (stop_ && tasks_.empty()) { + return; + } + + task = std::move(tasks_.front()); + tasks_.pop(); + ++active_tasks_; + } + + queue_not_full_.notify_one(); + + // Execute task outside the lock + task(); + + { + std::lock_guard lock(queue_mutex_); + --active_tasks_; + } + all_done_.notify_all(); + } + }); + } + } + + std::vector workers_; + std::queue> tasks_; + + mutable std::mutex queue_mutex_; + std::condition_variable condition_; + std::condition_variable queue_not_full_; + std::condition_variable all_done_; + + std::atomic stop_; + size_t active_tasks_; + const size_t max_queue_size_; +}; + +/** + * @brief Simple async file I/O helpers + */ +namespace async_io { + +/** + * @brief Async file write (fire-and-forget) + * @param filepath Path to write to + * @param data Data to write + * @param callback Optional callback on completion (success/failure) + * + * Safe for use from main thread - won't block UI. + */ +inline void write_file_async( + const std::string& filepath, + const std::string& data, + std::function callback = nullptr) +{ + ThreadPool::instance().enqueue_detached([filepath, data, callback]() { + FILE* fp = fopen(filepath.c_str(), "w"); + bool success = false; + if (fp) { + success = (fwrite(data.c_str(), 1, data.size(), fp) == data.size()); + fclose(fp); + } + if (callback) { + callback(success); + } + }); +} + +/** + * @brief Async file read with callback + * @param filepath Path to read from + * @param callback Called with file contents (empty on error) + */ +inline void read_file_async( + const std::string& filepath, + std::function callback) +{ + ThreadPool::instance().enqueue_detached([filepath, callback]() { + std::string content; + FILE* fp = fopen(filepath.c_str(), "r"); + if (fp) { + fseek(fp, 0, SEEK_END); + long size = ftell(fp); + fseek(fp, 0, SEEK_SET); + if (size > 0) { + content.resize(static_cast(size)); + size_t read = fread(content.data(), 1, static_cast(size), fp); + content.resize(read); + } + fclose(fp); + } + callback(content); + }); +} + +} // namespace async_io + +/** + * @brief Write-behind buffer for batching frequent writes + * + * Collects writes and flushes them periodically to reduce + * disk I/O on systems with slow storage (SD cards). + */ +class WriteBehindBuffer { +public: + using WriteFunc = std::function; + + explicit WriteBehindBuffer(WriteFunc writer, + std::chrono::milliseconds flush_interval = std::chrono::milliseconds(2000)) + : writer_(std::move(writer)) + , flush_interval_(flush_interval) + , stop_(false) + { + flush_thread_ = std::thread([this]() { + while (!stop_) { + std::this_thread::sleep_for(flush_interval_); + flush(); + } + // Final flush on shutdown + flush(); + }); + } + + ~WriteBehindBuffer() { + stop_ = true; + if (flush_thread_.joinable()) { + flush_thread_.join(); + } + } + + // Non-copyable + WriteBehindBuffer(const WriteBehindBuffer&) = delete; + WriteBehindBuffer& operator=(const WriteBehindBuffer&) = delete; + + /** + * @brief Queue a write (returns immediately) + * @param key Unique identifier for this write (later writes overwrite earlier) + * @param data Data to write + */ + void write(const std::string& key, const std::string& data) { + std::lock_guard lock(mutex_); + pending_writes_[key] = data; + } + + /** + * @brief Force immediate flush of all pending writes + */ + void flush() { + std::unordered_map to_write; + + { + std::lock_guard lock(mutex_); + to_write.swap(pending_writes_); + } + + for (const auto& [key, data] : to_write) { + writer_(key, data); + } + } + + /** + * @brief Check if there are pending writes + */ + bool has_pending() const { + std::lock_guard lock(mutex_); + return !pending_writes_.empty(); + } + +private: + WriteFunc writer_; + std::chrono::milliseconds flush_interval_; + std::unordered_map pending_writes_; + mutable std::mutex mutex_; + std::thread flush_thread_; + std::atomic stop_; +}; + +} // namespace vt + +#endif // VT_THREAD_POOL_HH diff --git a/src/network/remote_link.hh b/src/network/remote_link.hh index 581a03b8..36ee2a70 100644 --- a/src/network/remote_link.hh +++ b/src/network/remote_link.hh @@ -28,7 +28,9 @@ #include #include -inline constexpr size_t QUEUE_SIZE = 2097152; +// Reduced from 2MB to 256KB for better performance on memory-constrained +// systems like Raspberry Pi CM5 with 2GB RAM +inline constexpr size_t QUEUE_SIZE = 262144; /**** Types ****/ class CharQueue diff --git a/src/network/socket.cc b/src/network/socket.cc index 86605bd5..e105ffd5 100644 --- a/src/network/socket.cc +++ b/src/network/socket.cc @@ -309,6 +309,7 @@ int Listen(int port, int nonblocking) { vt::cpp23::format_to_buffer(str, STRLENGTH, "setsockopt port {}", port); perror(str); + close(sockfd); return -1; } @@ -321,6 +322,7 @@ int Listen(int port, int nonblocking) { vt::cpp23::format_to_buffer(str, STRLENGTH, "bind port {}", port); perror(str); + close(sockfd); return -1; } @@ -328,6 +330,7 @@ int Listen(int port, int nonblocking) { vt::cpp23::format_to_buffer(str, STRLENGTH, "listen port {}", port); perror(str); + close(sockfd); return -1; } return sockfd; diff --git a/term/layer.cc b/term/layer.cc index 3d7bed10..73ecfe9d 100644 --- a/term/layer.cc +++ b/term/layer.cc @@ -686,24 +686,8 @@ int Layer::Rectangle(int rx, int ry, int rw, int rh, int image) if (r.w > 0 && r.h > 0) { - // Optimize: cache current tile and tile origin to avoid redundant X11 calls - static Pixmap current_tile = 0; - static int current_origin_x = -1; - static int current_origin_y = -1; - - Pixmap new_tile = GetTexture(image); - - if (new_tile != current_tile) { - XSetTile(dis, gfx, new_tile); - current_tile = new_tile; - } - - if (page_x != current_origin_x || page_y != current_origin_y) { - XSetTSOrigin(dis, gfx, page_x, page_y); - current_origin_x = page_x; - current_origin_y = page_y; - } - + XSetTSOrigin(dis, gfx, page_x, page_y); + XSetTile(dis, gfx, GetTexture(image)); XSetFillStyle(dis, gfx, FillTiled); XFillRectangle(dis, pix, gfx, page_x + r.x, page_y + r.y, r.w, r.h); XSetFillStyle(dis, gfx, FillSolid); diff --git a/term/term_view.cc b/term/term_view.cc index 711bcdf0..f5c8b6aa 100644 --- a/term/term_view.cc +++ b/term/term_view.cc @@ -670,7 +670,7 @@ int SocketNo = 0; Display *Dis = nullptr; GC Gfx = nullptr; Window MainWin; -std::array Texture; +std::array Texture{}; // Lazy-loaded texture cache (initialized to 0) Pixmap ShadowPix; int ScrDepth = 0; Visual *ScrVis = nullptr; @@ -759,6 +759,14 @@ void X11ResourceManager::cleanup() { Gfx = nullptr; } + // Clean up cached textures + for (size_t i = 0; i < IMAGE_COUNT; ++i) { + if (Texture[i] != 0 && Dis != nullptr) { + XFreePixmap(Dis, Texture[i]); + Texture[i] = 0; + } + } + // Clean up fonts for (size_t i = 0; i < FONT_SPACE; ++i) { if (FontInfo[i]) { @@ -3949,6 +3957,10 @@ int OpenTerm(const char* display, TouchScreen *ts, int is_term_local, int term_h MainLayer = l; ResetView(); + // Preload all textures to prevent button highlighting bugs. + // The static cache optimization in Layer::Rectangle() can cause + // incorrect texture rendering if textures are lazy-loaded. + PreloadAllTextures(); // Performance monitoring removed for production efficiency ReadScreenSaverPix(); @@ -4422,12 +4434,59 @@ Pixmap GetTexture(const int texture) noexcept { FnTrace("GetTexture()"); + // Validate texture index if (texture < 0 || texture >= IMAGE_COUNT) { // Return default texture (DARK_SAND) for invalid texture IDs - return LoadPixmap(ImageData[IMAGE_DARK_SAND]); + return GetTexture(IMAGE_DARK_SAND); + } + + // Check if already cached + if (Texture[texture] != 0) { + return Texture[texture]; + } + + // Load and cache the texture (lazy loading) + Texture[texture] = LoadPixmap(ImageData[texture]); + return Texture[texture]; +} + +void ClearTextureCache() noexcept +{ + FnTrace("ClearTextureCache()"); + + for (size_t i = 0; i < IMAGE_COUNT; ++i) { + if (Texture[i] != 0 && Dis != nullptr) { + XFreePixmap(Dis, Texture[i]); + Texture[i] = 0; + } } +} - return LoadPixmap(ImageData[texture]); +void PreloadAllTextures() noexcept +{ + FnTrace("PreloadAllTextures()"); + + // Preload all textures at startup to prevent rendering bugs. + // The static cache in Layer::Rectangle() can cause buttons to show + // incorrect textures (e.g., highlighted when they shouldn't be) when + // lazy loading is used. By preloading all textures, we ensure that + // GetTexture() always returns consistent Pixmap values. + for (int i = 0; i < IMAGE_COUNT; ++i) { + (void)GetTexture(i); // Load and cache each texture + } +} + +int GetCachedTextureCount() noexcept +{ + FnTrace("GetCachedTextureCount()"); + + int count = 0; + for (size_t i = 0; i < IMAGE_COUNT; ++i) { + if (Texture[i] != 0) { + ++count; + } + } + return count; } XftFont *GetXftFontInfo(const int font_id) noexcept diff --git a/term/term_view.hh b/term/term_view.hh index 7f1d5425..af670b54 100644 --- a/term/term_view.hh +++ b/term/term_view.hh @@ -349,6 +349,9 @@ extern XftFont *GetXftFontInfo(int font_id) noexcept; extern int GetFontBaseline(int font_id) noexcept; extern int GetFontHeight(int font_id) noexcept; extern Pixmap GetTexture(int texture) noexcept; +extern void PreloadAllTextures() noexcept; // Preload all textures to avoid rendering bugs +extern void ClearTextureCache() noexcept; // Clear cached textures to free memory +extern int GetCachedTextureCount() noexcept; // Get number of currently loaded textures // Image loading functions extern Pixmap LoadPixmap(const char** image_data); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 086e80ef..1bc7f81f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -14,6 +14,7 @@ add_executable(vt_tests unit/test_sales_tax_calculations.cc unit/test_time_operations.cc unit/test_error_handler.cc + unit/test_list_utility.cc mocks/mock_terminal.cc mocks/mock_settings.cc ) diff --git a/tests/unit/test_list_utility.cc b/tests/unit/test_list_utility.cc new file mode 100644 index 00000000..3150bc10 --- /dev/null +++ b/tests/unit/test_list_utility.cc @@ -0,0 +1,235 @@ +/* + * test_list_utility.cc - Unit tests for list_utility.hh + * Tests count caching and other list operations + */ + +#include +#include "src/core/list_utility.hh" + +// Test node for SList (single linked) +struct SNode { + int value; + SNode *next{nullptr}; + + explicit SNode(int v) : value(v) {} +}; + +// Test node for DList (double linked) +struct DNode { + int value; + DNode *fore{nullptr}; + DNode *next{nullptr}; + + explicit DNode(int v) : value(v) {} +}; + +TEST_CASE("SList count caching", "[list_utility][slist]") { + SList list; + + SECTION("Empty list has count 0") { + REQUIRE(list.Count() == 0); + REQUIRE(list.IsEmpty() == true); + } + + SECTION("AddToHead increments count") { + list.AddToHead(new SNode(1)); + REQUIRE(list.Count() == 1); + + list.AddToHead(new SNode(2)); + REQUIRE(list.Count() == 2); + + list.AddToHead(new SNode(3)); + REQUIRE(list.Count() == 3); + } + + SECTION("AddToTail increments count") { + list.AddToTail(new SNode(1)); + REQUIRE(list.Count() == 1); + + list.AddToTail(new SNode(2)); + REQUIRE(list.Count() == 2); + } + + SECTION("AddAfterNode increments count") { + auto* first = new SNode(1); + list.AddToHead(first); + + list.AddAfterNode(first, new SNode(2)); + REQUIRE(list.Count() == 2); + } + + SECTION("Remove decrements count") { + auto* n1 = new SNode(1); + auto* n2 = new SNode(2); + auto* n3 = new SNode(3); + + list.AddToTail(n1); + list.AddToTail(n2); + list.AddToTail(n3); + REQUIRE(list.Count() == 3); + + list.Remove(n2); + delete n2; + REQUIRE(list.Count() == 2); + + list.Remove(n1); + delete n1; + REQUIRE(list.Count() == 1); + + list.Remove(n3); + delete n3; + REQUIRE(list.Count() == 0); + } + + SECTION("RemoveAndDelete decrements count") { + list.AddToTail(new SNode(1)); + list.AddToTail(new SNode(2)); + REQUIRE(list.Count() == 2); + + list.RemoveAndDelete(list.Head()); + REQUIRE(list.Count() == 1); + } + + SECTION("Purge resets count to 0") { + list.AddToTail(new SNode(1)); + list.AddToTail(new SNode(2)); + list.AddToTail(new SNode(3)); + REQUIRE(list.Count() == 3); + + list.Purge(); + REQUIRE(list.Count() == 0); + REQUIRE(list.IsEmpty() == true); + } + + SECTION("Move constructor transfers count") { + list.AddToTail(new SNode(1)); + list.AddToTail(new SNode(2)); + REQUIRE(list.Count() == 2); + + SList moved(std::move(list)); + REQUIRE(moved.Count() == 2); + REQUIRE(list.Count() == 0); // NOLINT - intentionally checking moved-from state + } +} + +TEST_CASE("DList count caching", "[list_utility][dlist]") { + DList list; + + SECTION("Empty list has count 0") { + REQUIRE(list.Count() == 0); + REQUIRE(list.IsEmpty() == true); + } + + SECTION("AddToHead increments count") { + list.AddToHead(new DNode(1)); + REQUIRE(list.Count() == 1); + + list.AddToHead(new DNode(2)); + REQUIRE(list.Count() == 2); + } + + SECTION("AddToTail increments count") { + list.AddToTail(new DNode(1)); + REQUIRE(list.Count() == 1); + + list.AddToTail(new DNode(2)); + REQUIRE(list.Count() == 2); + } + + SECTION("AddAfterNode increments count") { + auto* first = new DNode(1); + list.AddToHead(first); + + list.AddAfterNode(first, new DNode(2)); + REQUIRE(list.Count() == 2); + } + + SECTION("AddBeforeNode increments count") { + auto* last = new DNode(2); + list.AddToTail(last); + + list.AddBeforeNode(last, new DNode(1)); + REQUIRE(list.Count() == 2); + } + + SECTION("Remove decrements count") { + auto* n1 = new DNode(1); + auto* n2 = new DNode(2); + auto* n3 = new DNode(3); + + list.AddToTail(n1); + list.AddToTail(n2); + list.AddToTail(n3); + REQUIRE(list.Count() == 3); + + list.Remove(n2); + delete n2; + REQUIRE(list.Count() == 2); + + // Verify list integrity + REQUIRE(n1->next == n3); + REQUIRE(n3->fore == n1); + } + + SECTION("RemoveAndDelete decrements count") { + list.AddToTail(new DNode(1)); + list.AddToTail(new DNode(2)); + REQUIRE(list.Count() == 2); + + list.RemoveAndDelete(list.Head()); + REQUIRE(list.Count() == 1); + } + + SECTION("Purge resets count to 0") { + list.AddToTail(new DNode(1)); + list.AddToTail(new DNode(2)); + list.AddToTail(new DNode(3)); + REQUIRE(list.Count() == 3); + + list.Purge(); + REQUIRE(list.Count() == 0); + REQUIRE(list.IsEmpty() == true); + } + + SECTION("Sort preserves count") { + list.AddToTail(new DNode(3)); + list.AddToTail(new DNode(1)); + list.AddToTail(new DNode(2)); + REQUIRE(list.Count() == 3); + + list.Sort([](DNode* a, DNode* b) { return a->value - b->value; }); + REQUIRE(list.Count() == 3); + + // Verify sorted order + REQUIRE(list.Head()->value == 1); + REQUIRE(list.Tail()->value == 3); + } + + SECTION("Move constructor transfers count") { + list.AddToTail(new DNode(1)); + list.AddToTail(new DNode(2)); + REQUIRE(list.Count() == 2); + + DList moved(std::move(list)); + REQUIRE(moved.Count() == 2); + REQUIRE(list.Count() == 0); // NOLINT - intentionally checking moved-from state + } +} + +TEST_CASE("Count caching performance benefit", "[list_utility][performance]") { + DList list; + + // Add 1000 items + for (int i = 0; i < 1000; ++i) { + list.AddToTail(new DNode(i)); + } + + // Count() should now be O(1) - calling it many times should be fast + int total = 0; + for (int i = 0; i < 10000; ++i) { + total += list.Count(); // Would be O(n*10000) without caching + } + + REQUIRE(total == 10000 * 1000); + REQUIRE(list.Count() == 1000); +} diff --git a/zone/cdu_zone.cc b/zone/cdu_zone.cc index b8dad5eb..0f112c6a 100644 --- a/zone/cdu_zone.cc +++ b/zone/cdu_zone.cc @@ -41,7 +41,6 @@ CDUZone::CDUZone() AddSubmit("Submit", 10); record_no = -1; - report = nullptr; page = 0; no_line = 1; lines_shown = 0; @@ -49,11 +48,7 @@ CDUZone::CDUZone() cdustring = nullptr; } -CDUZone::~CDUZone() -{ - if (report != nullptr) - delete report; -} +CDUZone::~CDUZone() = default; RenderResult CDUZone::Render(Terminal *term, int update_flag) { @@ -80,10 +75,8 @@ RenderResult CDUZone::Render(Terminal *term, int update_flag) if (update || update_flag || (report == nullptr)) { - if (report != nullptr) - delete report; - report = new Report; - ListReport(term, report); + report = std::make_unique(); + ListReport(term, report.get()); } if (show_item) report->selected_line = record_no; @@ -260,11 +253,7 @@ int CDUZone::UpdateForm(Terminal *term, int record) } if (changed) { - if (report != nullptr) - { - delete report; - report = nullptr; - } + report.reset(); update = 1; } return 0; @@ -353,9 +342,7 @@ int CDUZone::SaveRecord(Terminal *term, int record, int write_file) records = RecordCount(term); if (record_no >= records) record_no = records - 1; - if (report != nullptr) - delete report; - report = nullptr; + report.reset(); cdustring = nullptr; show_item = 0; update = 1; @@ -436,8 +423,7 @@ int CDUZone::Search(Terminal *term, int record, const genericChar* word) { record_no = -1; show_item = 0; - delete report; - report = nullptr; + report.reset(); } return 1; } diff --git a/zone/cdu_zone.hh b/zone/cdu_zone.hh index e746d92d..10327eb2 100644 --- a/zone/cdu_zone.hh +++ b/zone/cdu_zone.hh @@ -26,6 +26,8 @@ #include "form_zone.hh" #include "utility.hh" +#include + #define CDU_ZONE_COLUMNS 2 class CDUZone : public FormZone @@ -34,7 +36,7 @@ class CDUZone : public FormZone Flt list_footer; Flt list_spacing; int lines_shown; - Report *report; + std::unique_ptr report; int page; int show_item; CDUString *cdustring; diff --git a/zone/check_list_zone.cc b/zone/check_list_zone.cc index 9d1c6e15..9a80a4ea 100644 --- a/zone/check_list_zone.cc +++ b/zone/check_list_zone.cc @@ -485,9 +485,7 @@ RenderResult CheckEditZone::Render(Terminal *term, int update_flag) { check = term->check; LoadRecord(term, 0); - if (report != nullptr) - free(report); - report = nullptr; + report.reset(); my_update = 0; } diff --git a/zone/check_list_zone.hh b/zone/check_list_zone.hh index 62277a4f..769bf05a 100644 --- a/zone/check_list_zone.hh +++ b/zone/check_list_zone.hh @@ -24,11 +24,13 @@ #include "form_zone.hh" #include "layout_zone.hh" +#include /**** Types ****/ class Archive; class Check; class Employee; +class Report; class CheckListZone : public LayoutZone { @@ -66,7 +68,7 @@ class CheckEditZone : public FormZone int page; int my_update; Check *check; - Report *report; + std::unique_ptr report; int view; public: diff --git a/zone/creditcard_list_zone.cc b/zone/creditcard_list_zone.cc index 67931c54..b36bdf5c 100644 --- a/zone/creditcard_list_zone.cc +++ b/zone/creditcard_list_zone.cc @@ -97,10 +97,8 @@ RenderResult CreditCardListZone::Render(Terminal *term, int update_flag) if (update || update_flag || (report == nullptr)) { - if (report != nullptr) - delete report; - report = new Report; - ListReport(term, report); + report = std::make_unique(); + ListReport(term, report.get()); } if (report != nullptr) diff --git a/zone/creditcard_list_zone.hh b/zone/creditcard_list_zone.hh index fbe712b2..18088bd9 100644 --- a/zone/creditcard_list_zone.hh +++ b/zone/creditcard_list_zone.hh @@ -24,13 +24,15 @@ #include "form_zone.hh" #include "credit.hh" +#include + class CreditCardListZone : public ListFormZone { Flt list_header; Flt list_footer; Flt list_spacing; int lines_shown; - Report *report; + std::unique_ptr report; Credit *credit; CreditDB *creditdb; Archive *archive; diff --git a/zone/dialog_zone.cc b/zone/dialog_zone.cc index 2e81ddd5..ff0757de 100644 --- a/zone/dialog_zone.cc +++ b/zone/dialog_zone.cc @@ -2025,16 +2025,18 @@ void CreditCardDialog::Init(Terminal *term, SubCheck *subch, const char* swipe_v term->credit->ParseSwipe(swipe_value); else ProcessSwipe(term, swipe_value); + // Cache settings pointer for efficiency + Settings *settings = term->GetSettings(); if (term->credit != nullptr && term->credit->IsValid() && term->auth_action == 0 && !term->credit->IsAuthed() && !term->credit->IsPreauthed() && - (term->GetSettings()->authorize_method == CCAUTH_CREDITCHEQ || - term->GetSettings()->auto_authorize > 0)) + (settings->authorize_method == CCAUTH_CREDITCHEQ || + settings->auto_authorize > 0)) { if ((term->credit->CardType() == CARD_TYPE_NONE) && - ((term->GetSettings()->card_types & CARD_TYPE_DEBIT) == 0) && - ((term->GetSettings()->card_types & CARD_TYPE_GIFT) == 0)) + ((settings->card_types & CARD_TYPE_DEBIT) == 0) && + ((settings->card_types & CARD_TYPE_GIFT) == 0)) { term->credit->SetCardType(CARD_TYPE_CREDIT); } diff --git a/zone/drawer_zone.cc b/zone/drawer_zone.cc index e1ef3984..ed64a3ce 100644 --- a/zone/drawer_zone.cc +++ b/zone/drawer_zone.cc @@ -367,11 +367,7 @@ DrawerManageZone::DrawerManageZone() drawer_zone_type = DRAWER_ZONE_BALANCE; } -DrawerManageZone::~DrawerManageZone() -{ - if (report) - delete report; -} +DrawerManageZone::~DrawerManageZone() = default; RenderResult DrawerManageZone::Render(Terminal *term, int update_flag) { @@ -413,8 +409,7 @@ RenderResult DrawerManageZone::Render(Terminal *term, int update_flag) if (report) { - delete report; - report = nullptr; + report.reset(); } CreateDrawers(term); group = 0; @@ -622,13 +617,9 @@ RenderResult DrawerManageZone::Render(Terminal *term, int update_flag) genericChar str[256]; if (d == nullptr) { - if (report) - { - delete report; - report = nullptr; - } + report.reset(); - report = new Report; + report = std::make_unique(); if (view == DRAWER_OPEN) report->TextC(term->Translate("There Are No Open Drawers For")); else @@ -660,8 +651,8 @@ RenderResult DrawerManageZone::Render(Terminal *term, int update_flag) default: if (report == nullptr) { - report = new Report; - d->MakeReport(term, check_list, report); + report = std::make_unique(); + d->MakeReport(term, check_list, report.get()); } if (report) { @@ -675,11 +666,7 @@ RenderResult DrawerManageZone::Render(Terminal *term, int update_flag) break; case DRAWER_PULLED: - if (report) - { - delete report; - report = nullptr; - } + report.reset(); int pcolor; int per_page = (int) ((size_y - 4) / 2.0); @@ -926,8 +913,7 @@ SignalResult DrawerManageZone::Signal(Terminal *term, const genericChar* message current = zoneobj; if (report) { - delete report; - report = nullptr; + report.reset(); } Draw(term, 0); return SIGNAL_OKAY; @@ -950,8 +936,7 @@ SignalResult DrawerManageZone::Signal(Terminal *term, const genericChar* message current = zoneobj; if (report) { - delete report; - report = nullptr; + report.reset(); } Draw(term, 0); return SIGNAL_OKAY; @@ -993,8 +978,7 @@ SignalResult DrawerManageZone::Touch(Terminal *term, int tx, int ty) } if (report) { - delete report; - report = nullptr; + report.reset(); } media = TENDER_CASH; Draw(term, 0); @@ -1089,8 +1073,7 @@ int DrawerManageZone::Update(Terminal *term, int update_message, const genericCh { if (report) { - delete report; - report = nullptr; + report.reset(); } return Draw(term, 0); } diff --git a/zone/drawer_zone.hh b/zone/drawer_zone.hh index 5f77fd10..5f5aeee8 100644 --- a/zone/drawer_zone.hh +++ b/zone/drawer_zone.hh @@ -25,6 +25,7 @@ #include "zone_object.hh" #include "expense.hh" +#include /**** Definitions ****/ #define MAX_BALANCES 64 @@ -63,7 +64,7 @@ class DrawerManageZone : public LayoutZone ZoneObject *current; int drawers_shown; int group; - Report *report; + std::unique_ptr report; int mode; int view; Drawer *drawer_list; diff --git a/zone/expense_zone.cc b/zone/expense_zone.cc index e6a84026..a59bb78f 100644 --- a/zone/expense_zone.cc +++ b/zone/expense_zone.cc @@ -71,8 +71,6 @@ ExpenseZone::ExpenseZone() ExpenseZone::~ExpenseZone() { - if (report != nullptr) - delete report; if (saved_expense != nullptr) delete saved_expense; } @@ -116,10 +114,8 @@ RenderResult ExpenseZone::Render(Terminal *term, int update_flag) // generate and display the list of expenses if (update || update_flag || (report == nullptr)) { - if (report != nullptr) - delete report; - report = new Report; - ListReport(term, report); + report = std::make_unique(); + ListReport(term, report.get()); } if (show_expense) report->selected_line = record_no; @@ -336,11 +332,7 @@ int ExpenseZone::UpdateForm(Terminal *term, int record) if (changed) { - if (report != nullptr) - { - delete report; - report = nullptr; - } + report.reset(); update = 1; } return 0; @@ -554,9 +546,7 @@ int ExpenseZone::SaveRecord(Terminal *term, int record, int write_file) record_no = records - 1; // Update the drawer balance entry term->system_data->expense_db.AddDrawerPayments(dlist); - if (report != nullptr) - delete report; - report = nullptr; + report.reset(); expense = nullptr; show_expense = 0; update = 1; @@ -657,8 +647,7 @@ int ExpenseZone::Search(Terminal *term, int record, const genericChar* word) { record_no = -1; show_expense = 0; - delete report; - report = nullptr; + report.reset(); } return 1; } diff --git a/zone/expense_zone.hh b/zone/expense_zone.hh index b1eef695..a1fa5a78 100644 --- a/zone/expense_zone.hh +++ b/zone/expense_zone.hh @@ -24,6 +24,8 @@ #include "form_zone.hh" #include "expense.hh" +#include + #define EXPENSE_ZONE_COLUMNS 5 class ExpenseZone : public FormZone @@ -35,7 +37,7 @@ class ExpenseZone : public FormZone Flt list_footer; Flt list_spacing; int lines_shown; - Report *report; + std::unique_ptr report; int page; int allow_balanced; public: diff --git a/zone/labor_zone.cc b/zone/labor_zone.cc index a562c3a9..beb2ff3b 100644 --- a/zone/labor_zone.cc +++ b/zone/labor_zone.cc @@ -66,11 +66,7 @@ LaborZone::LaborZone() } // Destructor -LaborZone::~LaborZone() -{ - if (report) - delete report; -} +LaborZone::~LaborZone() = default; // Member Functions RenderResult LaborZone::Render(Terminal *term, int update_flag) @@ -82,8 +78,7 @@ RenderResult LaborZone::Render(Terminal *term, int update_flag) { if (report) { - delete report; - report = nullptr; + report.reset(); } if (update_flag == RENDER_NEW || period == nullptr) @@ -120,9 +115,9 @@ RenderResult LaborZone::Render(Terminal *term, int update_flag) if (report == nullptr && period) { - report = new Report; + report = std::make_unique(); report->SetTitle(GlobalTranslate("Time Clock Summary")); - period->WorkReport(term, term->server, start, end, report); + period->WorkReport(term, term->server, start, end, report.get()); } FormZone::Render(term, update_flag); @@ -391,7 +386,7 @@ SignalResult LaborZone::Touch(Terminal *term, int tx, int ty) int LaborZone::Update(Terminal *term, int update_message, const genericChar* value) { FnTrace("LaborZone::Update()"); - Report *r = report; + Report *r = report.get(); if (r == nullptr || r->update_flag & update_message) return Draw(term, 1); @@ -520,8 +515,7 @@ int LaborZone::UpdateForm(Terminal *term, int record) work->end = we; if (report) { - delete report; - report = nullptr; + report.reset(); } } return 0; diff --git a/zone/labor_zone.hh b/zone/labor_zone.hh index 3d69903f..580587d8 100644 --- a/zone/labor_zone.hh +++ b/zone/labor_zone.hh @@ -23,6 +23,7 @@ #include "form_zone.hh" +#include /**** Types ****/ class Employee; @@ -33,7 +34,7 @@ class WorkEntry; // Employee timeclock viewing/editing class LaborZone : public FormZone { - Report *report; + std::unique_ptr report; LaborPeriod *period; WorkEntry *work; int page; diff --git a/zone/payout_zone.cc b/zone/payout_zone.cc index 0751cdd3..08333e7f 100644 --- a/zone/payout_zone.cc +++ b/zone/payout_zone.cc @@ -58,15 +58,10 @@ PayoutZone::PayoutZone() page = 0; archive = nullptr; tip_db = nullptr; - report = nullptr; } // Destructor -PayoutZone::~PayoutZone() -{ - if (report) - delete report; -} +PayoutZone::~PayoutZone() = default; // Member Functions RenderResult PayoutZone::Render(Terminal *term, int update_flag) @@ -80,11 +75,7 @@ RenderResult PayoutZone::Render(Terminal *term, int update_flag) sys->tip_db.Update(sys); archive = nullptr; } - if (report) - { - delete report; - report = nullptr; - } + report.reset(); if (archive) tip_db = &(archive->tip_db); else @@ -98,8 +89,8 @@ RenderResult PayoutZone::Render(Terminal *term, int update_flag) if (report == nullptr) { - report = new Report; - tip_db->ListReport(term, term->user, report); + report = std::make_unique(); + tip_db->ListReport(term, term->user, report.get()); } genericChar str[256], price[64]; @@ -345,7 +336,9 @@ RenderResult EndDayZone::Render(Terminal *term, int update_flag) Settings *s = &(sys->settings); Archive *a = sys->ArchiveListEnd(); genericChar buffer[STRLENGTH]; - int min_day_secs = term->GetSettings()->min_day_length; + // Cache settings pointer for efficiency + Settings *settings = term->GetSettings(); + int min_day_secs = settings->min_day_length; int min_day_hrs = min_day_secs / 60 / 60; int line = 0; diff --git a/zone/payout_zone.hh b/zone/payout_zone.hh index bebc6c4f..7fa9088a 100644 --- a/zone/payout_zone.hh +++ b/zone/payout_zone.hh @@ -23,6 +23,7 @@ #include "layout_zone.hh" +#include /**** Types ****/ class Archive; @@ -40,7 +41,7 @@ class PayoutZone : public LayoutZone Archive *archive; TipDB *tip_db; Flt spacing; - Report *report; + std::unique_ptr report; public: // Constructor diff --git a/zone/report_zone.cc b/zone/report_zone.cc index 5882af2f..f74d4452 100644 --- a/zone/report_zone.cc +++ b/zone/report_zone.cc @@ -66,13 +66,7 @@ ReportZone::ReportZone() } // Destructor -ReportZone::~ReportZone() -{ - if (report) - delete report; - if (temp_report) - delete temp_report; -} +ReportZone::~ReportZone() = default; // Member Functions RenderResult ReportZone::Render(Terminal *term, int update_flag) @@ -81,7 +75,7 @@ RenderResult ReportZone::Render(Terminal *term, int update_flag) Employee *e = term->user; System *sys = term->system_data; Settings *s = &(sys->settings); - Report *r = temp_report; + Report *r = temp_report.get(); // allow no user signin for kitchen display if (e == nullptr && report_type != REPORT_CHECK) @@ -92,19 +86,14 @@ RenderResult ReportZone::Render(Terminal *term, int update_flag) update_flag = 0; if (r->is_complete) { - if (report) - delete report; - report = r; - temp_report = nullptr; + report = std::move(temp_report); if (printing_to_printer) { Print(term, printer_dest); printing_to_printer = 0; - if (report != nullptr) - delete report; - report = nullptr; - temp_report = new Report(); - sys->RoyaltyReport(term, day_start, day_end, term->archive, temp_report, this); + report.reset(); + temp_report = std::make_unique(); + sys->RoyaltyReport(term, day_start, day_end, term->archive, temp_report.get(), this); return RENDER_OKAY; } } @@ -116,8 +105,7 @@ RenderResult ReportZone::Render(Terminal *term, int update_flag) { if (report) { - delete report; - report = nullptr; + report.reset(); // page = 0; // <--- REMOVE THIS LINE! } @@ -229,68 +217,68 @@ RenderResult ReportZone::Render(Terminal *term, int update_flag) d = term->FindDrawer(); } - temp_report = new Report; + temp_report = std::make_unique(); switch (report_type) { case REPORT_DRAWER: if (term->server == nullptr) - sys->DrawerSummaryReport(term, drawer_list, check_list, temp_report); + sys->DrawerSummaryReport(term, drawer_list, check_list, temp_report.get()); else if (d) - d->MakeReport(term, check_list, temp_report); + d->MakeReport(term, check_list, temp_report.get()); break; case REPORT_CLOSEDCHECK: - sys->ClosedCheckReport(term, day_start, day_end, term->server, temp_report); + sys->ClosedCheckReport(term, day_start, day_end, term->server, temp_report.get()); break; case REPORT_SERVERLABOR: - sys->labor_db.ServerLaborReport(term, e, day_start, day_end, temp_report); + sys->labor_db.ServerLaborReport(term, e, day_start, day_end, temp_report.get()); break; case REPORT_CHECK: - DisplayCheckReport(term, temp_report); + DisplayCheckReport(term, temp_report.get()); break; case REPORT_SERVER: - sys->ServerReport(term, day_start, day_end, term->server, temp_report); + sys->ServerReport(term, day_start, day_end, term->server, temp_report.get()); break; case REPORT_SALES: - sys->SalesMixReport(term, day_start, day_end, term->server, temp_report); + sys->SalesMixReport(term, day_start, day_end, term->server, temp_report.get()); break; case REPORT_BALANCE: if (period_view != SP_DAY) - sys->BalanceReport(term, day_start, day_end, temp_report); + sys->BalanceReport(term, day_start, day_end, temp_report.get()); else - sys->ShiftBalanceReport(term, ref, temp_report); + sys->ShiftBalanceReport(term, ref, temp_report.get()); break; case REPORT_DEPOSIT: if (period_view != SP_NONE) - sys->DepositReport(term, day_start, day_end, nullptr, temp_report); + sys->DepositReport(term, day_start, day_end, nullptr, temp_report.get()); else - sys->DepositReport(term, day_start, day_end, term->archive, temp_report); + sys->DepositReport(term, day_start, day_end, term->archive, temp_report.get()); break; case REPORT_COMPEXCEPTION: - sys->ItemExceptionReport(term, day_start, day_end, 1, term->server, temp_report); + sys->ItemExceptionReport(term, day_start, day_end, 1, term->server, temp_report.get()); break; case REPORT_VOIDEXCEPTION: - sys->ItemExceptionReport(term, day_start, day_end, 2, term->server, temp_report); + sys->ItemExceptionReport(term, day_start, day_end, 2, term->server, temp_report.get()); break; case REPORT_TABLEEXCEPTION: - sys->TableExceptionReport(term, day_start, day_end, term->server, temp_report); + sys->TableExceptionReport(term, day_start, day_end, term->server, temp_report.get()); break; case REPORT_REBUILDEXCEPTION: - sys->RebuildExceptionReport(term, day_start, day_end, term->server, temp_report); + sys->RebuildExceptionReport(term, day_start, day_end, term->server, temp_report.get()); break; case REPORT_CUSTOMERDETAIL: - sys->CustomerDetailReport(term, e, temp_report); + sys->CustomerDetailReport(term, e, temp_report.get()); break; case REPORT_EXPENSES: - sys->ExpenseReport(term, day_start, day_end, nullptr, temp_report, this); + sys->ExpenseReport(term, day_start, day_end, nullptr, temp_report.get(), this); break; case REPORT_ROYALTY: - sys->RoyaltyReport(term, day_start, day_end, term->archive, temp_report, this); + sys->RoyaltyReport(term, day_start, day_end, term->archive, temp_report.get(), this); break; case REPORT_AUDITING: - sys->AuditingReport(term, day_start, day_end, term->archive, temp_report, this); + sys->AuditingReport(term, day_start, day_end, term->archive, temp_report.get(), this); break; case REPORT_CREDITCARD: - sys->CreditCardReport(term, day_start, day_end, term->archive, temp_report, this); + sys->CreditCardReport(term, day_start, day_end, term->archive, temp_report.get(), this); break; } @@ -298,8 +286,7 @@ RenderResult ReportZone::Render(Terminal *term, int update_flag) if (temp_report && temp_report->is_complete) { - report = temp_report; - temp_report = nullptr; + report = std::move(temp_report); } } @@ -1338,7 +1325,7 @@ int ReportZone::Update(Terminal *t, int update_message, const genericChar* value return 0; } - Report *r = report; + Report *r = report.get(); if (r == nullptr) return 0; @@ -1411,20 +1398,11 @@ int ReportZone::Print(Terminal *t, int print_mode) { System *sys = t->system_data; printing_to_printer = 1; - if (report != nullptr) - { - delete report; - report = nullptr; - } - if (temp_report != nullptr) - { - delete temp_report; - temp_report = nullptr; - } - temp_report = new Report; + report.reset(); + temp_report = std::make_unique(); temp_report->max_width = p->MaxWidth(); temp_report->destination = RP_DEST_PRINTER; - sys->RoyaltyReport(t, day_start, day_end, t->archive, temp_report, this); + sys->RoyaltyReport(t, day_start, day_end, t->archive, temp_report.get(), this); return 0; } diff --git a/zone/report_zone.hh b/zone/report_zone.hh index b2ae10a1..4747fe4c 100644 --- a/zone/report_zone.hh +++ b/zone/report_zone.hh @@ -24,6 +24,7 @@ #include "layout_zone.hh" #include "report.hh" +#include /**** Types ****/ class Employee; @@ -31,8 +32,8 @@ class Employee; // General report viewing zone class ReportZone : public LayoutZone { - Report *report; - Report *temp_report; + std::unique_ptr report; + std::unique_ptr temp_report; int lines_shown; int page; int header;