From 81caf1aae01fcd7697b76fd4b1dbb4b16a14b5be Mon Sep 17 00:00:00 2001 From: Daniel Sage <24928223+dnsge@users.noreply.github.com> Date: Sat, 18 Jun 2022 06:44:48 -0400 Subject: [PATCH] Rewrite & optimize LimitedQueue (#3798) * Use circular buffer for LimitedQueue * Reduce copying of snapshot * Small optimizations * Remove unneeded lock statements * Add LimitedQueue tests * Fix includes for limited queue benchmark * Update CHANGELOG.md * Use correct boost version iterators * Use a shared_mutex to clarify reads and writes * Update `find`/`rfind` to return the result as a boost::optional * Use `[[nodiscard]]` where applicable * Update comments * Add a couple more doc comments * Replace size with get get is a safe (locked & checked) version of at * Use std::vector in LimitedQueueSnapshot * Update LimitedQueue benchmarks * Add mutex guard to buffer accessors We do not know whether T is an atomic type or not so we can't safely say that we can copy the value at a certain address of the buffer. See https://stackoverflow.com/a/2252478 * Update doc comments, add first/last getters * Make limit_ const * Omit `else` if the if-case always returns * Title case category comments * Remove `at` * Fix `get` comment * Privatize/comment/lock property accessors - `limit` is now private - `space` is now private - `full` has been removed - `empty` now locks * Remove `front` function * Remove `back` method * Add comment to `first` * Add comment to `last` Co-authored-by: Rasmus Karlsson --- CHANGELOG.md | 1 + benchmarks/CMakeLists.txt | 1 + benchmarks/src/LimitedQueue.cpp | 116 ++++++ src/common/Channel.cpp | 21 +- src/messages/LimitedQueue.hpp | 521 +++++++++++++------------- src/messages/LimitedQueueSnapshot.hpp | 62 +-- src/widgets/Scrollbar.cpp | 7 +- src/widgets/Scrollbar.hpp | 2 +- src/widgets/helper/ChannelView.cpp | 51 +-- src/widgets/helper/ChannelView.hpp | 2 +- tests/CMakeLists.txt | 1 + tests/src/LimitedQueue.cpp | 132 +++++++ 12 files changed, 576 insertions(+), 341 deletions(-) create mode 100644 benchmarks/src/LimitedQueue.cpp create mode 100644 tests/src/LimitedQueue.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index e7710dbf..88eac3ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ - Bugfix: Fixed automod queue pubsub topic persisting after user change. (#3718) - Bugfix: Fixed viewer list not closing after pressing escape key. (#3734) - Bugfix: Fixed links with no thumbnail having previous link's thumbnail. (#3720) +- Dev: Rewrite LimitedQueue (#3798) - Dev: Overhaul highlight system by moving all checks into a Controller allowing for easier tests. (#3399, #3801) - Dev: Use Game Name returned by Get Streams instead of querying it from the Get Games API. (#3662) - Dev: Batch checking live status for all channels after startup. (#3757, #3762, #3767) diff --git a/benchmarks/CMakeLists.txt b/benchmarks/CMakeLists.txt index af78a0f9..1bc975fd 100644 --- a/benchmarks/CMakeLists.txt +++ b/benchmarks/CMakeLists.txt @@ -6,6 +6,7 @@ set(benchmark_SOURCES ${CMAKE_CURRENT_LIST_DIR}/src/Highlights.cpp ${CMAKE_CURRENT_LIST_DIR}/src/FormatTime.cpp ${CMAKE_CURRENT_LIST_DIR}/src/Helpers.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/LimitedQueue.cpp # Add your new file above this line! ) diff --git a/benchmarks/src/LimitedQueue.cpp b/benchmarks/src/LimitedQueue.cpp new file mode 100644 index 00000000..00b3507e --- /dev/null +++ b/benchmarks/src/LimitedQueue.cpp @@ -0,0 +1,116 @@ +#include "messages/LimitedQueue.hpp" + +#include + +#include +#include +#include + +using namespace chatterino; + +void BM_LimitedQueue_PushBack(benchmark::State &state) +{ + LimitedQueue queue(1000); + for (auto _ : state) + { + queue.pushBack(1); + } +} + +void BM_LimitedQueue_PushFront_One(benchmark::State &state) +{ + std::vector items = {1}; + LimitedQueue queue(2); + + for (auto _ : state) + { + state.PauseTiming(); + queue.clear(); + state.ResumeTiming(); + queue.pushFront(items); + } +} + +void BM_LimitedQueue_PushFront_Many(benchmark::State &state) +{ + std::vector items; + items.resize(10000); + std::iota(items.begin(), items.end(), 0); + + for (auto _ : state) + { + state.PauseTiming(); + LimitedQueue queue(1000); + state.ResumeTiming(); + queue.pushFront(items); + } +} + +void BM_LimitedQueue_Replace(benchmark::State &state) +{ + LimitedQueue queue(1000); + for (int i = 0; i < 1000; ++i) + { + queue.pushBack(i); + } + + for (auto _ : state) + { + queue.replaceItem(500, 500); + } +} + +void BM_LimitedQueue_Snapshot(benchmark::State &state) +{ + LimitedQueue queue(1000); + for (int i = 0; i < 1000; ++i) + { + queue.pushBack(i); + } + + for (auto _ : state) + { + auto snapshot = queue.getSnapshot(); + benchmark::DoNotOptimize(snapshot); + } +} + +void BM_LimitedQueue_Snapshot_ExpensiveCopy(benchmark::State &state) +{ + LimitedQueue> queue(1000); + for (int i = 0; i < 1000; ++i) + { + queue.pushBack(std::make_shared(i)); + } + + for (auto _ : state) + { + auto snapshot = queue.getSnapshot(); + benchmark::DoNotOptimize(snapshot); + } +} + +void BM_LimitedQueue_Find(benchmark::State &state) +{ + LimitedQueue queue(1000); + for (int i = 0; i < 10000; ++i) + { + queue.pushBack(i); + } + + for (auto _ : state) + { + auto res = queue.find([](const auto &val) { + return val == 500; + }); + benchmark::DoNotOptimize(res); + } +} + +BENCHMARK(BM_LimitedQueue_PushBack); +BENCHMARK(BM_LimitedQueue_PushFront_One); +BENCHMARK(BM_LimitedQueue_PushFront_Many); +BENCHMARK(BM_LimitedQueue_Replace); +BENCHMARK(BM_LimitedQueue_Snapshot); +BENCHMARK(BM_LimitedQueue_Snapshot_ExpensiveCopy); +BENCHMARK(BM_LimitedQueue_Find); diff --git a/src/common/Channel.cpp b/src/common/Channel.cpp index 63c5ba72..b11e2cb3 100644 --- a/src/common/Channel.cpp +++ b/src/common/Channel.cpp @@ -246,23 +246,20 @@ void Channel::deleteMessage(QString messageID) msg->flags.set(MessageFlag::Disabled); } } + MessagePtr Channel::findMessage(QString messageID) { - LimitedQueueSnapshot snapshot = this->getMessageSnapshot(); - int snapshotLength = snapshot.size(); + MessagePtr res; - int end = std::max(0, snapshotLength - 200); - - for (int i = snapshotLength - 1; i >= end; --i) + if (auto msg = this->messages_.rfind([&messageID](const MessagePtr &msg) { + return msg->id == messageID; + }); + msg) { - auto &s = snapshot[i]; - - if (s->id == messageID) - { - return s; - } + res = *msg; } - return nullptr; + + return res; } bool Channel::canSendMessage() const diff --git a/src/messages/LimitedQueue.hpp b/src/messages/LimitedQueue.hpp index f206704f..1f3b696d 100644 --- a/src/messages/LimitedQueue.hpp +++ b/src/messages/LimitedQueue.hpp @@ -2,296 +2,295 @@ #include "messages/LimitedQueueSnapshot.hpp" -#include +#include +#include -#include +#include #include +#include #include namespace chatterino { -// -// Warning: -// - this class is so overengineered it's not even funny anymore -// -// Explanation: -// - messages can be appended until 'limit' is reached -// - when the limit is reached for every message added one will be removed at -// the start -// - messages can only be added to the start when there is space for them, -// trying to add messages to the start when it's full will not add them -// - you are able to get a "Snapshot" which captures the state of this object -// - adding items to this class does not change the "items" of the snapshot -// - template class LimitedQueue { -protected: - using Chunk = std::vector; - using ChunkVector = std::vector>; - public: LimitedQueue(size_t limit = 1000) : limit_(limit) + , buffer_(limit) { - this->clear(); - } - - void clear() - { - std::lock_guard lock(this->mutex_); - - this->chunks_ = std::make_shared(); - auto chunk = std::make_shared(); - chunk->resize(this->chunkSize_); - this->chunks_->push_back(chunk); - this->firstChunkOffset_ = 0; - this->lastChunkEnd_ = 0; - } - - // return true if an item was deleted - // deleted will be set if the item was deleted - bool pushBack(const T &item, T &deleted) - { - std::lock_guard lock(this->mutex_); - - auto lastChunk = this->chunks_->back(); - - if (lastChunk->size() <= this->lastChunkEnd_) - { - // Last chunk is full, create a new one and rebuild our chunk vector - auto newVector = std::make_shared(); - - // copy chunks - for (auto &chunk : *this->chunks_) - { - newVector->push_back(chunk); - } - - // push back new chunk - auto newChunk = std::make_shared(); - newChunk->resize(this->chunkSize_); - newVector->push_back(newChunk); - - // replace current chunk vector - this->chunks_ = newVector; - this->lastChunkEnd_ = 0; - lastChunk = this->chunks_->back(); - } - - lastChunk->at(this->lastChunkEnd_++) = item; - - return this->deleteFirstItem(deleted); - } - - // returns a vector with all the accepted items - std::vector pushFront(const std::vector &items) - { - std::vector acceptedItems; - - if (this->space() > 0) - { - std::lock_guard lock(this->mutex_); - - // create new vector to clone chunks into - auto newChunks = std::make_shared(); - - newChunks->resize(this->chunks_->size()); - - // copy chunks except for first one - for (size_t i = 1; i < this->chunks_->size(); i++) - { - newChunks->at(i) = this->chunks_->at(i); - } - - // create new chunk for the first one - size_t offset = - std::min(this->space(), static_cast(items.size())); - auto newFirstChunk = std::make_shared(); - newFirstChunk->resize(this->chunks_->front()->size() + offset); - - for (size_t i = 0; i < offset; i++) - { - newFirstChunk->at(i) = items[items.size() - offset + i]; - acceptedItems.push_back(items[items.size() - offset + i]); - } - - for (size_t i = 0; i < this->chunks_->at(0)->size(); i++) - { - newFirstChunk->at(i + offset) = this->chunks_->at(0)->at(i); - } - - newChunks->at(0) = newFirstChunk; - - this->chunks_ = newChunks; - - if (this->chunks_->size() == 1) - { - this->lastChunkEnd_ += offset; - } - } - - return acceptedItems; - } - - // replace an single item, return index if successful, -1 if unsuccessful - int replaceItem(const T &item, const T &replacement) - { - std::lock_guard lock(this->mutex_); - - int x = 0; - - for (size_t i = 0; i < this->chunks_->size(); i++) - { - auto &chunk = this->chunks_->at(i); - - size_t start = i == 0 ? this->firstChunkOffset_ : 0; - size_t end = - i == chunk->size() - 1 ? this->lastChunkEnd_ : chunk->size(); - - for (size_t j = start; j < end; j++) - { - if (chunk->at(j) == item) - { - auto newChunk = std::make_shared(); - newChunk->resize(chunk->size()); - - for (size_t k = 0; k < chunk->size(); k++) - { - newChunk->at(k) = chunk->at(k); - } - - newChunk->at(j) = replacement; - this->chunks_->at(i) = newChunk; - - return x; - } - x++; - } - } - - return -1; - } - - // replace an item at index, return true if worked - bool replaceItem(size_t index, const T &replacement) - { - std::lock_guard lock(this->mutex_); - - size_t x = 0; - - for (size_t i = 0; i < this->chunks_->size(); i++) - { - auto &chunk = this->chunks_->at(i); - - size_t start = i == 0 ? this->firstChunkOffset_ : 0; - size_t end = - i == chunk->size() - 1 ? this->lastChunkEnd_ : chunk->size(); - - for (size_t j = start; j < end; j++) - { - if (x == index) - { - auto newChunk = std::make_shared(); - newChunk->resize(chunk->size()); - - for (size_t k = 0; k < chunk->size(); k++) - { - newChunk->at(k) = chunk->at(k); - } - - newChunk->at(j) = replacement; - this->chunks_->at(i) = newChunk; - - return true; - } - x++; - } - } - return false; - } - - // void insertAfter(const std::vector &items, const T &index) - - LimitedQueueSnapshot getSnapshot() - { - std::lock_guard lock(this->mutex_); - - return LimitedQueueSnapshot( - this->chunks_, this->limit_ - this->space(), - this->firstChunkOffset_, this->lastChunkEnd_); - } - - bool empty() const - { - return this->limit_ - this->space() == 0; } private: - qsizetype space() const + /// Property Accessors + /** + * @brief Return the limit of the internal buffer + */ + [[nodiscard]] size_t limit() const { - size_t totalSize = 0; - for (auto &chunk : *this->chunks_) - { - totalSize += chunk->size(); - } - - totalSize -= this->chunks_->back()->size() - this->lastChunkEnd_; - if (this->chunks_->size() != 1) - { - totalSize -= this->firstChunkOffset_; - } - - return this->limit_ - totalSize; + return this->limit_; } - bool deleteFirstItem(T &deleted) + /** + * @brief Return the amount of space left in the buffer + * + * This does not lock + */ + [[nodiscard]] size_t space() const { - // determine if the first chunk should be deleted - if (space() > 0) + return this->limit() - this->buffer_.size(); + } + +public: + /** + * @brief Return true if the buffer is empty + */ + [[nodiscard]] bool empty() const + { + std::shared_lock lock(this->mutex_); + + return this->buffer_.empty(); + } + + /// Value Accessors + // Copies of values are returned so that references aren't invalidated + + /** + * @brief Get the item at the given index safely + * + * @param[in] index the index of the item to fetch + * @return the item at the index if it's populated, or none if it's not + */ + [[nodiscard]] boost::optional get(size_t index) const + { + std::shared_lock lock(this->mutex_); + + if (index >= this->buffer_.size()) + { + return boost::none; + } + + return this->buffer_[index]; + } + + /** + * @brief Get the first item from the queue + * + * @return the item at the front of the queue if it's populated, or none the queue is empty + */ + [[nodiscard]] boost::optional first() const + { + std::shared_lock lock(this->mutex_); + + if (this->buffer_.empty()) + { + return boost::none; + } + + return this->buffer_.front(); + } + + /** + * @brief Get the last item from the queue + * + * @return the item at the back of the queue if it's populated, or none the queue is empty + */ + [[nodiscard]] boost::optional last() const + { + std::shared_lock lock(this->mutex_); + + if (this->buffer_.empty()) + { + return boost::none; + } + + return this->buffer_.back(); + } + + /// Modifiers + + // Clear the buffer + void clear() + { + std::unique_lock lock(this->mutex_); + + this->buffer_.clear(); + } + + /** + * @brief Push an item to the end of the queue + * + * @param item the item to push + * @param[out] deleted the item that was deleted + * @return true if an element was deleted to make room + */ + bool pushBack(const T &item, T &deleted) + { + std::unique_lock lock(this->mutex_); + + bool full = this->buffer_.full(); + if (full) + { + deleted = this->buffer_.front(); + } + this->buffer_.push_back(item); + return full; + } + + /** + * @brief Push an item to the end of the queue + * + * @param item the item to push + * @return true if an element was deleted to make room + */ + bool pushBack(const T &item) + { + std::unique_lock lock(this->mutex_); + + bool full = this->buffer_.full(); + this->buffer_.push_back(item); + return full; + } + + /** + * @brief Push items into beginning of queue + * + * Items are inserted in reverse order. + * Items will only be inserted if they fit, + * meaning no elements can be deleted from using this function. + * + * @param items the vector of items to push + * @return vector of elements that were pushed + */ + std::vector pushFront(const std::vector &items) + { + std::unique_lock lock(this->mutex_); + + size_t numToPush = std::min(items.size(), this->space()); + std::vector pushed; + pushed.reserve(numToPush); + + size_t f = items.size() - numToPush; + size_t b = items.size() - 1; + for (; f < items.size(); ++f, --b) + { + this->buffer_.push_front(items[b]); + pushed.push_back(items[f]); + } + + return pushed; + } + + /** + * @brief Replace the needle with the given item + * + * @param[in] needle the item to search for + * @param[in] replacement the item to replace needle with + * @return the index of the replaced item, or -1 if no replacement took place + */ + int replaceItem(const T &needle, const T &replacement) + { + std::unique_lock lock(this->mutex_); + + for (int i = 0; i < this->buffer_.size(); ++i) + { + if (this->buffer_[i] == needle) + { + this->buffer_[i] = replacement; + return i; + } + } + return -1; + } + + /** + * @brief Replace the item at index with the given item + * + * @param[in] index the index of the item to replace + * @param[in] replacement the item to put in place of the item at index + * @return true if a replacement took place + */ + bool replaceItem(size_t index, const T &replacement) + { + std::unique_lock lock(this->mutex_); + + if (index >= this->buffer_.size()) { return false; } - deleted = this->chunks_->front()->at(this->firstChunkOffset_); - - // need to delete the first chunk - if (this->firstChunkOffset_ == this->chunks_->front()->size() - 1) - { - // copy the chunk vector - auto newVector = std::make_shared(); - - // delete first chunk - bool first = true; - for (auto &chunk : *this->chunks_) - { - if (!first) - { - newVector->push_back(chunk); - } - first = false; - } - - this->chunks_ = newVector; - this->firstChunkOffset_ = 0; - } - else - { - this->firstChunkOffset_++; - } - + this->buffer_[index] = replacement; return true; } - std::shared_ptr chunks_; - std::mutex mutex_; + [[nodiscard]] LimitedQueueSnapshot getSnapshot() const + { + std::shared_lock lock(this->mutex_); + return LimitedQueueSnapshot(this->buffer_); + } + + // Actions + + /** + * @brief Returns the first item matching a predicate + * + * The contents of the LimitedQueue are iterated over from front to back + * until the first element that satisfies `pred(item)`. If no item + * satisfies the predicate, or if the queue is empty, then boost::none + * is returned. + * + * @param[in] pred predicate that will be applied to items + * @return the first item found or boost::none + */ + template + [[nodiscard]] boost::optional find(Predicate pred) const + { + std::shared_lock lock(this->mutex_); + + for (const auto &item : this->buffer_) + { + if (pred(item)) + { + return item; + } + } + + return boost::none; + } + + /** + * @brief Returns the first item matching a predicate, checking in reverse + * + * The contents of the LimitedQueue are iterated over from back to front + * until the first element that satisfies `pred(item)`. If no item + * satisfies the predicate, or if the queue is empty, then boost::none + * is returned. + * + * @param[in] pred predicate that will be applied to items + * @return the first item found or boost::none + */ + template + [[nodiscard]] boost::optional rfind(Predicate pred) const + { + std::shared_lock lock(this->mutex_); + + for (auto it = this->buffer_.rbegin(); it != this->buffer_.rend(); ++it) + { + if (pred(*it)) + { + return *it; + } + } + + return boost::none; + } + +private: + mutable std::shared_mutex mutex_; - size_t firstChunkOffset_; - size_t lastChunkEnd_; const size_t limit_; - - const size_t chunkSize_ = 100; + boost::circular_buffer buffer_; }; } // namespace chatterino diff --git a/src/messages/LimitedQueueSnapshot.hpp b/src/messages/LimitedQueueSnapshot.hpp index e7d62ae1..9e8da8b7 100644 --- a/src/messages/LimitedQueueSnapshot.hpp +++ b/src/messages/LimitedQueueSnapshot.hpp @@ -1,60 +1,62 @@ #pragma once +#include + #include #include #include namespace chatterino { +template +class LimitedQueue; + template class LimitedQueueSnapshot { +private: + friend class LimitedQueue; + + LimitedQueueSnapshot(const boost::circular_buffer &buf) + : buffer_(buf.begin(), buf.end()) + { + } + public: LimitedQueueSnapshot() = default; - LimitedQueueSnapshot( - std::shared_ptr>>> chunks, - size_t length, size_t firstChunkOffset, size_t lastChunkEnd) - : chunks_(chunks) - , length_(length) - , firstChunkOffset_(firstChunkOffset) - , lastChunkEnd_(lastChunkEnd) + size_t size() const { + return this->buffer_.size(); } - std::size_t size() const + const T &operator[](size_t index) const { - return this->length_; + return this->buffer_[index]; } - T const &operator[](std::size_t index) const + auto begin() const { - index += this->firstChunkOffset_; + return this->buffer_.begin(); + } - size_t x = 0; + auto end() const + { + return this->buffer_.end(); + } - for (size_t i = 0; i < this->chunks_->size(); i++) - { - auto &chunk = this->chunks_->at(i); + auto rbegin() const + { + return this->buffer_.rbegin(); + } - if (x <= index && x + chunk->size() > index) - { - return chunk->at(index - x); - } - x += chunk->size(); - } - - assert(false && "out of range"); - - return this->chunks_->at(0)->at(0); + auto rend() const + { + return this->buffer_.rend(); } private: - std::shared_ptr>>> chunks_; - - size_t length_ = 0; - size_t firstChunkOffset_ = 0; - size_t lastChunkEnd_ = 0; + std::vector buffer_; }; } // namespace chatterino diff --git a/src/widgets/Scrollbar.cpp b/src/widgets/Scrollbar.cpp index 69a99c66..67e2176b 100644 --- a/src/widgets/Scrollbar.cpp +++ b/src/widgets/Scrollbar.cpp @@ -32,8 +32,7 @@ Scrollbar::Scrollbar(ChannelView *parent) void Scrollbar::addHighlight(ScrollbarHighlight highlight) { - ScrollbarHighlight deleted; - this->highlights_.pushBack(highlight, deleted); + this->highlights_.pushBack(highlight); } void Scrollbar::addHighlightsAtStart( @@ -62,7 +61,7 @@ void Scrollbar::clearHighlights() this->highlights_.clear(); } -LimitedQueueSnapshot Scrollbar::getHighlightSnapshot() +LimitedQueueSnapshot &Scrollbar::getHighlightSnapshot() { if (!this->highlightsPaused_) { @@ -275,7 +274,7 @@ void Scrollbar::paintEvent(QPaintEvent *) } // draw highlights - auto snapshot = this->getHighlightSnapshot(); + auto &snapshot = this->getHighlightSnapshot(); size_t snapshotLength = snapshot.size(); if (snapshotLength == 0) diff --git a/src/widgets/Scrollbar.hpp b/src/widgets/Scrollbar.hpp index ff6cbc76..140a7992 100644 --- a/src/widgets/Scrollbar.hpp +++ b/src/widgets/Scrollbar.hpp @@ -67,7 +67,7 @@ protected: private: Q_PROPERTY(qreal currentValue_ READ getCurrentValue WRITE setCurrentValue) - LimitedQueueSnapshot getHighlightSnapshot(); + LimitedQueueSnapshot &getHighlightSnapshot(); void updateScroll(); QMutex mutex_; diff --git a/src/widgets/helper/ChannelView.cpp b/src/widgets/helper/ChannelView.cpp index 1b9c37f0..29e85d26 100644 --- a/src/widgets/helper/ChannelView.cpp +++ b/src/widgets/helper/ChannelView.cpp @@ -381,7 +381,7 @@ void ChannelView::performLayout(bool causedByScrollbar) // BenchmarkGuard benchmark("layout"); /// Get messages and check if there are at least 1 - auto messages = this->getMessagesSnapshot(); + auto &messages = this->getMessagesSnapshot(); this->showingLatestMessages_ = this->scrollBar_->isAtBottom() || !this->scrollBar_->isVisible(); @@ -502,7 +502,7 @@ QString ChannelView::getSelectedText() { QString result = ""; - LimitedQueueSnapshot messagesSnapshot = + LimitedQueueSnapshot &messagesSnapshot = this->getMessagesSnapshot(); Selection _selection = this->selection_; @@ -561,7 +561,7 @@ const boost::optional &ChannelView::getOverrideFlags() return this->overrideFlags_; } -LimitedQueueSnapshot ChannelView::getMessagesSnapshot() +LimitedQueueSnapshot &ChannelView::getMessagesSnapshot() { if (!this->paused() /*|| this->scrollBar_->isVisible()*/) { @@ -682,11 +682,9 @@ void ChannelView::setChannel(ChannelPtr underlyingChannel) auto snapshot = underlyingChannel->getMessageSnapshot(); - for (size_t i = 0; i < snapshot.size(); i++) + for (const auto &msg : snapshot) { - MessageLayoutPtr deleted; - - auto messageLayout = new MessageLayout(snapshot[i]); + auto messageLayout = new MessageLayout(msg); if (this->lastMessageHasAlternateBackground_) { @@ -700,11 +698,10 @@ void ChannelView::setChannel(ChannelPtr underlyingChannel) messageLayout->flags.set(MessageLayoutFlag::IgnoreHighlights); } - this->messages_.pushBack(MessageLayoutPtr(messageLayout), deleted); + this->messages_.pushBack(MessageLayoutPtr(messageLayout)); if (this->showScrollbarHighlights()) { - this->scrollBar_->addHighlight( - snapshot[i]->getScrollBarHighlight()); + this->scrollBar_->addHighlight(msg->getScrollBarHighlight()); } } @@ -776,8 +773,6 @@ bool ChannelView::hasSourceChannel() const void ChannelView::messageAppended(MessagePtr &message, boost::optional overridingFlags) { - MessageLayoutPtr deleted; - auto *messageFlags = &message->flags; if (overridingFlags) { @@ -809,7 +804,7 @@ void ChannelView::messageAppended(MessagePtr &message, loop.exec(); } - if (this->messages_.pushBack(MessageLayoutPtr(messageRef), deleted)) + if (this->messages_.pushBack(MessageLayoutPtr(messageRef))) { if (this->paused()) { @@ -915,22 +910,16 @@ void ChannelView::messageRemoveFromStart(MessagePtr &message) void ChannelView::messageReplaced(size_t index, MessagePtr &replacement) { - if (index >= this->messages_.getSnapshot().size()) + auto oMessage = this->messages_.get(index); + if (!oMessage) { return; } + auto message = *oMessage; + MessageLayoutPtr newItem(new MessageLayout(replacement)); - auto snapshot = this->messages_.getSnapshot(); - if (index >= snapshot.size()) - { - qCDebug(chatterinoWidget) - << "Tried to replace out of bounds message. Index:" << index - << ". Length:" << snapshot.size(); - return; - } - const auto &message = snapshot[index]; if (message->flags.has(MessageLayoutFlag::AlternateBackground)) { newItem->flags.set(MessageLayoutFlag::AlternateBackground); @@ -945,11 +934,9 @@ void ChannelView::messageReplaced(size_t index, MessagePtr &replacement) void ChannelView::updateLastReadMessage() { - auto _snapshot = this->getMessagesSnapshot(); - - if (_snapshot.size() > 0) + if (auto lastMessage = this->messages_.last()) { - this->lastReadMessage_ = _snapshot[_snapshot.size() - 1]; + this->lastReadMessage_ = *lastMessage; } this->update(); @@ -1055,7 +1042,7 @@ void ChannelView::paintEvent(QPaintEvent * /*event*/) // such as the grey overlay when a message is disabled void ChannelView::drawMessages(QPainter &painter) { - auto messagesSnapshot = this->getMessagesSnapshot(); + auto &messagesSnapshot = this->getMessagesSnapshot(); size_t start = size_t(this->scrollBar_->getCurrentValue()); @@ -1122,7 +1109,7 @@ void ChannelView::drawMessages(QPainter &painter) // add all messages on screen to the map for (size_t i = start; i < messagesSnapshot.size(); ++i) { - std::shared_ptr layout = messagesSnapshot[i]; + const std::shared_ptr &layout = messagesSnapshot[i]; this->messagesOnScreen_.insert(layout); @@ -1153,7 +1140,7 @@ void ChannelView::wheelEvent(QWheelEvent *event) qreal desired = this->scrollBar_->getDesiredValue(); qreal delta = event->angleDelta().y() * qreal(1.5) * mouseMultiplier; - auto snapshot = this->getMessagesSnapshot(); + auto &snapshot = this->getMessagesSnapshot(); int snapshotLength = int(snapshot.size()); int i = std::min(int(desired), snapshotLength); @@ -1553,7 +1540,7 @@ void ChannelView::mousePressEvent(QMouseEvent *event) if (!tryGetMessageAt(event->pos(), layout, relativePos, messageIndex)) { setCursor(Qt::ArrowCursor); - auto messagesSnapshot = this->getMessagesSnapshot(); + auto &messagesSnapshot = this->getMessagesSnapshot(); if (messagesSnapshot.size() == 0) { return; @@ -2387,7 +2374,7 @@ bool ChannelView::tryGetMessageAt(QPoint p, std::shared_ptr &_message, QPoint &relativePos, int &index) { - auto messagesSnapshot = this->getMessagesSnapshot(); + auto &messagesSnapshot = this->getMessagesSnapshot(); size_t start = this->scrollBar_->getCurrentValue(); diff --git a/src/widgets/helper/ChannelView.hpp b/src/widgets/helper/ChannelView.hpp index 6588fc6c..1aabbf62 100644 --- a/src/widgets/helper/ChannelView.hpp +++ b/src/widgets/helper/ChannelView.hpp @@ -94,7 +94,7 @@ public: void setSourceChannel(ChannelPtr sourceChannel); bool hasSourceChannel() const; - LimitedQueueSnapshot getMessagesSnapshot(); + LimitedQueueSnapshot &getMessagesSnapshot(); void queueLayout(); void clearMessages(); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 72a3a6dc..827fe67c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -19,6 +19,7 @@ set(test_SOURCES ${CMAKE_CURRENT_LIST_DIR}/src/TwitchMessageBuilder.cpp ${CMAKE_CURRENT_LIST_DIR}/src/HighlightController.cpp ${CMAKE_CURRENT_LIST_DIR}/src/FormatTime.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/LimitedQueue.cpp # Add your new file above this line! ) diff --git a/tests/src/LimitedQueue.cpp b/tests/src/LimitedQueue.cpp new file mode 100644 index 00000000..9ecf6203 --- /dev/null +++ b/tests/src/LimitedQueue.cpp @@ -0,0 +1,132 @@ +#include "messages/LimitedQueue.hpp" + +#include +#include + +using namespace chatterino; + +namespace chatterino { + +template +std::ostream &operator<<(std::ostream &os, + const LimitedQueueSnapshot &snapshot) +{ + os << "[ "; + for (size_t i = 0; i < snapshot.size(); ++i) + { + os << snapshot[i] << ' '; + } + os << "]"; + + return os; +} + +} // namespace chatterino + +namespace std { +template +std::ostream &operator<<(std::ostream &os, const vector &vec) +{ + os << "[ "; + for (const auto &item : vec) + { + os << item << ' '; + } + os << "]"; + + return os; +} +} // namespace std + +template +inline void SNAPSHOT_EQUALS(const LimitedQueueSnapshot &snapshot, + const std::vector &values, + const std::string &msg) +{ + SCOPED_TRACE(msg); + ASSERT_EQ(snapshot.size(), values.size()) + << "snapshot = " << snapshot << " values = " << values; + + if (snapshot.size() != values.size()) + return; + + for (size_t i = 0; i < snapshot.size(); ++i) + { + EXPECT_EQ(snapshot[i], values[i]) << "i = " << i; + } +} + +TEST(LimitedQueue, PushBack) +{ + LimitedQueue queue(5); + int d = 0; + bool flag; + + EXPECT_TRUE(queue.empty()); + flag = queue.pushBack(1, d); + EXPECT_FALSE(flag); + flag = queue.pushBack(2, d); + EXPECT_FALSE(flag); + + EXPECT_FALSE(queue.empty()); + + auto snapshot1 = queue.getSnapshot(); + SNAPSHOT_EQUALS(snapshot1, {1, 2}, "first snapshot"); + + flag = queue.pushBack(3, d); + EXPECT_FALSE(flag); + flag = queue.pushBack(4, d); + EXPECT_FALSE(flag); + + // snapshot should be the same + SNAPSHOT_EQUALS(snapshot1, {1, 2}, "first snapshot same 1"); + + flag = queue.pushBack(5, d); + EXPECT_FALSE(flag); + flag = queue.pushBack(6, d); + EXPECT_TRUE(flag); + EXPECT_EQ(d, 1); + + SNAPSHOT_EQUALS(snapshot1, {1, 2}, "first snapshot same 2"); + + auto snapshot2 = queue.getSnapshot(); + SNAPSHOT_EQUALS(snapshot2, {2, 3, 4, 5, 6}, "second snapshot"); + SNAPSHOT_EQUALS(snapshot1, {1, 2}, "first snapshot same 3"); +} + +TEST(LimitedQueue, PushFront) +{ + LimitedQueue queue(5); + queue.pushBack(1); + queue.pushBack(2); + queue.pushBack(3); + + std::vector expectedPush = {7, 8}; + auto pushed = queue.pushFront({4, 5, 6, 7, 8}); + auto snapshot = queue.getSnapshot(); + SNAPSHOT_EQUALS(snapshot, {7, 8, 1, 2, 3}, "first snapshot"); + EXPECT_EQ(pushed, expectedPush); + + auto pushed2 = queue.pushFront({9, 10, 11}); + EXPECT_EQ(pushed2.size(), 0); +} + +TEST(LimitedQueue, ReplaceItem) +{ + LimitedQueue queue(5); + queue.pushBack(1); + queue.pushBack(2); + queue.pushBack(3); + + int idex = queue.replaceItem(2, 10); + EXPECT_EQ(idex, 1); + idex = queue.replaceItem(5, 11); + EXPECT_EQ(idex, -1); + + bool res = queue.replaceItem(std::size_t(0), 9); + EXPECT_TRUE(res); + res = queue.replaceItem(std::size_t(5), 4); + EXPECT_FALSE(res); + + SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3}, "first snapshot"); +}