From 74226102118400b228618f7373137a4a01e7d85f Mon Sep 17 00:00:00 2001 From: Shane Synan Date: Sat, 1 Sep 2018 16:02:00 -0500 Subject: [PATCH] common: Port HighlightRule to ExpressionMatch Port HighlightRule objects to ExpressionMatch class, providing easy caching and simplifying expression handling. Migrate HighlightRule struct into a full-blown class for easier management and greater assurance over automatic internal cache management. Port HighlightRuleManager to ExpressionMatch for nickname matching, providing easy caching and simplifying expression handling. (Noticing a theme?) Add tons of documentation comments, too, and fix up line lengths. --- src/common/highlightrulemanager.cpp | 173 ++++++---- src/common/highlightrulemanager.h | 298 ++++++++++++++++-- .../corehighlightsettingspage.cpp | 53 ++-- 3 files changed, 414 insertions(+), 110 deletions(-) diff --git a/src/common/highlightrulemanager.cpp b/src/common/highlightrulemanager.cpp index ff60df62..c9c4cfcd 100644 --- a/src/common/highlightrulemanager.cpp +++ b/src/common/highlightrulemanager.cpp @@ -22,6 +22,7 @@ #include +#include "expressionmatch.h" #include "util.h" INIT_SYNCABLE_OBJECT(HighlightRuleManager) @@ -42,7 +43,7 @@ HighlightRuleManager &HighlightRuleManager::operator=(const HighlightRuleManager int HighlightRuleManager::indexOf(int id) const { for (int i = 0; i < _highlightRuleList.count(); i++) { - if (_highlightRuleList[i].id == id) + if (_highlightRuleList[i].id() == id) return i; } return -1; @@ -53,7 +54,7 @@ int HighlightRuleManager::nextId() { int max = 0; for (int i = 0; i < _highlightRuleList.count(); i++) { - int id = _highlightRuleList[i].id; + int id = _highlightRuleList[i].id(); if (id > max) { max = id; } @@ -75,14 +76,14 @@ QVariantMap HighlightRuleManager::initHighlightRuleList() const QStringList channel; for (int i = 0; i < _highlightRuleList.count(); i++) { - id << _highlightRuleList[i].id; - name << _highlightRuleList[i].name; - isRegEx << _highlightRuleList[i].isRegEx; - isCaseSensitive << _highlightRuleList[i].isCaseSensitive; - isActive << _highlightRuleList[i].isEnabled; - isInverse << _highlightRuleList[i].isInverse; - sender << _highlightRuleList[i].sender; - channel << _highlightRuleList[i].chanName; + id << _highlightRuleList[i].id(); + name << _highlightRuleList[i].contents(); + isRegEx << _highlightRuleList[i].isRegEx(); + isCaseSensitive << _highlightRuleList[i].isCaseSensitive(); + isActive << _highlightRuleList[i].isEnabled(); + isInverse << _highlightRuleList[i].isInverse(); + sender << _highlightRuleList[i].sender(); + channel << _highlightRuleList[i].chanName(); } highlightRuleListMap["id"] = id; @@ -155,47 +156,34 @@ bool HighlightRuleManager::match(const QString &msgContents, bool matches = false; for (int i = 0; i < _highlightRuleList.count(); i++) { - const HighlightRule &rule = _highlightRuleList.at(i); - if (!rule.isEnabled) + auto &rule = _highlightRuleList.at(i); + if (!rule.isEnabled()) continue; - if (!rule.chanName.isEmpty() - && !scopeMatch(bufferName, rule.chanName, rule.isRegEx, rule.isCaseSensitive)) { + // Skip if channel name doesn't match and channel rule is not empty + // + // Match succeeds if... + // Channel name matches a defined rule + // Defined rule is empty + // And take the inverse of the above + if (!rule.chanNameMatcher().match(bufferName, true)) { // A channel name rule is specified and does NOT match the current buffer name, skip // this rule continue; } - bool nameMatch = false; - if (rule.name.isEmpty()) { - // Empty rule, matches any message - nameMatch = true; - } else { - // Check according to specified rule - QRegExp rx; - if (rule.isRegEx) { - rx = QRegExp(rule.name, - rule.isCaseSensitive ? Qt::CaseSensitive : Qt::CaseInsensitive); - } else { - rx = QRegExp("(^|\\W)" + QRegExp::escape(rule.name) + "(\\W|$)", - rule.isCaseSensitive ? Qt::CaseSensitive : Qt::CaseInsensitive); - } - nameMatch = (rx.indexIn(stripFormatCodes(msgContents)) >= 0); - } + // Check message according to specified rule, allowing empty rules to match + bool contentsMatch = rule.contentsMatcher().match(stripFormatCodes(msgContents), true); - bool senderMatch; - if (rule.sender.isEmpty()) { - senderMatch = true; - } else { - // A sender name rule is specified, match according to scope rules. - senderMatch = scopeMatch(msgSender, rule.sender, rule.isRegEx, rule.isCaseSensitive); - } + // Check sender according to specified rule, allowing empty rules to match + bool senderMatch = rule.senderMatcher().match(msgSender, true); - if (nameMatch && senderMatch) { + if (contentsMatch && senderMatch) { // If an inverse rule matches, then we know that we never want to return a highlight. - if (rule.isInverse) { + if (rule.isInverse()) { return false; - } else { + } + else { matches = true; } } @@ -204,22 +192,16 @@ bool HighlightRuleManager::match(const QString &msgContents, if (matches) return true; - if (!currentNick.isEmpty()) { - QStringList nickList; - if (_highlightNick == CurrentNick) { - nickList << currentNick; - } - else if (_highlightNick == AllNicks) { - nickList = identityNicks; - if (!nickList.contains(currentNick)) - nickList.prepend(currentNick); - } + // Check nicknames + if (_highlightNick != HighlightNickType::NoNick && !currentNick.isEmpty()) { + // Update cache if needed + determineNickExpressions(currentNick, identityNicks); - for(const QString &nickname : nickList) { - QRegExp nickRegExp("(^|\\W)" + QRegExp::escape(nickname) + "(\\W|$)", _nicksCaseSensitive ? Qt::CaseSensitive : Qt::CaseInsensitive); - if (nickRegExp.indexIn(stripFormatCodes(msgContents)) >= 0) { - return true; - } + // Check for a match + if (_cachedNickMatcher.isValid() + && _cachedNickMatcher.match(stripFormatCodes(msgContents))) { + // Nick matcher is valid and match found + return true; } } @@ -239,7 +221,7 @@ void HighlightRuleManager::toggleHighlightRule(int highlightRule) int idx = indexOf(highlightRule); if (idx == -1) return; - _highlightRuleList[idx].isEnabled = !_highlightRuleList[idx].isEnabled; + _highlightRuleList[idx].setIsEnabled(!_highlightRuleList[idx].isEnabled()); SYNC(ARG(highlightRule)) } @@ -248,3 +230,82 @@ bool HighlightRuleManager::match(const Message &msg, const QString ¤tNick, { return match(msg.contents(), msg.sender(), msg.type(), msg.flags(), msg.bufferInfo().bufferName(), currentNick, identityNicks); } + + +void HighlightRuleManager::determineNickExpressions(const QString ¤tNick, + const QStringList identityNicks) const +{ + // Don't do anything for no nicknames + if (_highlightNick == HighlightNickType::NoNick) { + return; + } + + // Only update if needed (check nickname config, current nick, identity nicks for change) + if (!_cacheNickConfigInvalid + && _cachedNickCurrent == currentNick + && _cachedIdentityNicks == identityNicks) { + return; + } + + // Add all nicknames + QStringList nickList; + if (_highlightNick == CurrentNick) { + nickList << currentNick; + } + else if (_highlightNick == AllNicks) { + nickList = identityNicks; + if (!nickList.contains(currentNick)) + nickList.prepend(currentNick); + } + + // Set up phrase matcher, joining with newlines + _cachedNickMatcher = ExpressionMatch(nickList.join("\n"), + ExpressionMatch::MatchMode::MatchMultiPhrase, + _nicksCaseSensitive); + + _cacheNickConfigInvalid = false; + _cachedNickCurrent = currentNick; + _cachedIdentityNicks = identityNicks; +} + + +/************************************************************************** + * HighlightRule + *************************************************************************/ +bool HighlightRuleManager::HighlightRule::operator!=(const HighlightRule &other) const +{ + return (_id != other._id || + _contents != other._contents || + _isRegEx != other._isRegEx || + _isCaseSensitive != other._isCaseSensitive || + _isEnabled != other._isEnabled || + _isInverse != other._isInverse || + _sender != other._sender || + _chanName != other._chanName); + // Don't compare ExpressionMatch objects as they are created as needed from the above +} + + +void HighlightRuleManager::HighlightRule::determineExpressions() const +{ + // Don't update if not needed + if (!_cacheInvalid) { + return; + } + + // Set up matching rules + // Message is either phrase or regex + ExpressionMatch::MatchMode contentsMode = + _isRegEx ? ExpressionMatch::MatchMode::MatchRegEx : + ExpressionMatch::MatchMode::MatchPhrase; + // Sender and channel are either multiple wildcard entries or regex + ExpressionMatch::MatchMode scopeMode = + _isRegEx ? ExpressionMatch::MatchMode::MatchRegEx : + ExpressionMatch::MatchMode::MatchMultiWildcard; + + _contentsMatch = ExpressionMatch(_contents, contentsMode, _isCaseSensitive); + _senderMatch = ExpressionMatch(_sender, scopeMode, _isCaseSensitive); + _chanNameMatch = ExpressionMatch(_chanName, scopeMode, _isCaseSensitive); + + _cacheInvalid = false; +} diff --git a/src/common/highlightrulemanager.h b/src/common/highlightrulemanager.h index 3d7407f2..cabf0d25 100644 --- a/src/common/highlightrulemanager.h +++ b/src/common/highlightrulemanager.h @@ -22,12 +22,12 @@ #include -#include #include #include #include #include +#include "expressionmatch.h" #include "message.h" #include "syncableobject.h" @@ -49,34 +49,250 @@ public: inline HighlightRuleManager(QObject *parent = nullptr) : SyncableObject(parent) { setAllowClientUpdates(true); } HighlightRuleManager &operator=(const HighlightRuleManager &other); - struct HighlightRule + /** + * Individual highlight rule + */ + class HighlightRule { - int id; - QString name; - bool isRegEx = false; - bool isCaseSensitive = false; - bool isEnabled = true; - bool isInverse = false; - QString sender; - QString chanName; + public: + /** + * Construct an empty highlight rule + */ HighlightRule() {} - HighlightRule(int id_, QString name_, bool isRegEx_, bool isCaseSensitive_, bool isEnabled_, bool isInverse_, - QString sender_, QString chanName_) - : id(id_), name(std::move(name_)), isRegEx(isRegEx_), isCaseSensitive(isCaseSensitive_), - isEnabled(isEnabled_), isInverse(isInverse_), sender(std::move(sender_)), chanName(std::move(chanName_)) - {} - bool operator!=(const HighlightRule &other) const + /** + * Construct a highlight rule with the given parameters + * + * @param id Integer ID of the rule + * @param contents String representing a message contents expression to match + * @param isRegEx True if regular expression, otherwise false + * @param isCaseSensitive True if case sensitive, otherwise false + * @param isEnabled True if enabled, otherwise false + * @param isInverse True if rule is treated as highlight ignore, otherwise false + * @param sender String representing a message sender expression to match + * @param chanName String representing a channel name expression to match + */ + HighlightRule(int id, QString contents, bool isRegEx, bool isCaseSensitive, bool isEnabled, + bool isInverse, QString sender, QString chanName) + : _id(id), _contents(contents), _isRegEx(isRegEx), _isCaseSensitive(isCaseSensitive), + _isEnabled(isEnabled), _isInverse(isInverse), _sender(sender), _chanName(chanName) { - return (id != other.id || - name != other.name || - isRegEx != other.isRegEx || - isCaseSensitive != other.isCaseSensitive || - isEnabled != other.isEnabled || - isInverse != other.isInverse || - sender != other.sender || - chanName != other.chanName); + _cacheInvalid = true; + // Cache expression matches on construction + // + // This provides immediate feedback on errors when loading the rule. If profiling shows + // this as a performance bottleneck, this can be removed in deference to caching on + // first use. + // + // Inversely, if needed for validity checks, caching can be done on every update below + // instead of on first use. + determineExpressions(); } + + /** + * Gets the unique ID of this rule + * + * @return Integer ID of the rule + */ + inline int id() const { + return _id; + } + /** + * Sets the ID of this rule + * + * CAUTION: IDs should be kept unique! + * + * @param id Integer ID of the rule + */ + inline void setId(int id) { + _id = id; + } + + /** + * Gets the message contents this rule matches + * + * NOTE: Use HighlightRule::contentsMatcher() for performing matches + * + * @return String representing a phrase or expression to match + */ + inline QString contents() const { + return _contents; + } + /** + * Sets the message contents this rule matches + * + * @param contents String representing a phrase or expression to match + */ + inline void setContents(const QString &contents) { + _contents = contents; + _cacheInvalid = true; + } + + /** + * Gets if this is a regular expression rule + * + * @return True if regular expression, otherwise false + */ + inline bool isRegEx() const { + return _isRegEx; + } + /** + * Sets if this rule is a regular expression rule + * + * @param isRegEx True if regular expression, otherwise false + */ + inline void setIsRegEx(bool isRegEx) { + _isRegEx = isRegEx; + _cacheInvalid = true; + } + + /** + * Gets if this rule is case sensitive + * + * @return True if case sensitive, otherwise false + */ + inline bool isCaseSensitive() const { + return _isCaseSensitive; + } + /** + * Sets if this rule is case sensitive + * + * @param isCaseSensitive True if case sensitive, otherwise false + */ + inline void setIsCaseSensitive(bool isCaseSensitive) { + _isCaseSensitive = isCaseSensitive; + _cacheInvalid = true; + } + + /** + * Gets if this rule is enabled and active + * + * @return True if enabled, otherwise false + */ + inline bool isEnabled() const { + return _isEnabled; + } + /** + * Sets if this rule is enabled and active + * + * @param isEnabled True if enabled, otherwise false + */ + inline void setIsEnabled(bool isEnabled) { + _isEnabled = isEnabled; + } + + /** + * Gets if this rule is a highlight ignore rule + * + * @return True if rule is treated as highlight ignore, otherwise false + */ + inline bool isInverse() const { + return _isInverse; + } + /** + * Sets if this rule is a highlight ignore rule + * + * @param isInverse True if rule is treated as highlight ignore, otherwise false + */ + inline void setIsInverse(bool isInverse) { + _isInverse = isInverse; + } + + /** + * Gets the message sender this rule matches + * + * NOTE: Use HighlightRule::senderMatcher() for performing matches + * + * @return String representing a phrase or expression to match + */ + inline QString sender() const { return _sender; } + /** + * Sets the message sender this rule matches + * + * @param sender String representing a phrase or expression to match + */ + inline void setSender(const QString &sender) { + _sender = sender; + _cacheInvalid = true; + } + + /** + * Gets the channel name this rule matches + * + * NOTE: Use HighlightRule::chanNameMatcher() for performing matches + * + * @return String representing a phrase or expression to match + */ + inline QString chanName() const { return _chanName; } + /** + * Sets the channel name this rule matches + * + * @param chanName String representing a phrase or expression to match + */ + inline void setChanName(const QString &chanName) { + _chanName = chanName; + _cacheInvalid = true; + } + + /** + * Gets the expression matcher for the message contents, caching if needed + * + * @return Expression matcher to compare with message contents + */ + inline ExpressionMatch contentsMatcher() const { + if (_cacheInvalid) { + determineExpressions(); + } + return _contentsMatch; + } + + /** + * Gets the expression matcher for the message sender, caching if needed + * + * @return Expression matcher to compare with message sender + */ + inline ExpressionMatch senderMatcher() const { + if (_cacheInvalid) { + determineExpressions(); + } + return _senderMatch; + } + + /** + * Gets the expression matcher for the channel name, caching if needed + * + * @return Expression matcher to compare with channel name + */ + inline ExpressionMatch chanNameMatcher() const { + if (_cacheInvalid) { + determineExpressions(); + } + return _chanNameMatch; + } + + bool operator!=(const HighlightRule &other) const; + + private: + /** + * Update internal cache of expression matching if needed + */ + void determineExpressions() const; + + int _id = -1; + QString _contents = {}; + bool _isRegEx = false; + bool _isCaseSensitive = false; + bool _isEnabled = true; + bool _isInverse = false; + QString _sender = {}; + QString _chanName = {}; + + // These represent internal cache and should be safe to mutate in 'const' functions + // See https://stackoverflow.com/questions/3141087/what-is-meant-with-const-at-end-of-function-declaration + mutable bool _cacheInvalid = true; ///< If true, match cache needs redone + mutable ExpressionMatch _contentsMatch = {}; ///< Expression match cache for message content + mutable ExpressionMatch _senderMatch = {}; ///< Expression match cache for sender + mutable ExpressionMatch _chanNameMatch = {}; ///< Expression match cache for channel name }; using HighlightRuleList = QList; @@ -146,14 +362,20 @@ public slots: REQUEST(ARG(highlightNick)) } - inline void setHighlightNick(int highlightNick) { _highlightNick = static_cast(highlightNick); } + inline void setHighlightNick(int highlightNick) { + _highlightNick = static_cast(highlightNick); + _cacheNickConfigInvalid = true; + } virtual inline void requestSetNicksCaseSensitive(bool nicksCaseSensitive) { REQUEST(ARG(nicksCaseSensitive)) } - inline void setNicksCaseSensitive(bool nicksCaseSensitive) { _nicksCaseSensitive = nicksCaseSensitive; } + inline void setNicksCaseSensitive(bool nicksCaseSensitive) { + _nicksCaseSensitive = nicksCaseSensitive; + _cacheNickConfigInvalid = true; + } protected: void setHighlightRuleList(const QList &HighlightRuleList) { _highlightRuleList = HighlightRuleList; } @@ -170,7 +392,31 @@ signals: void ruleAdded(QString name, bool isRegEx, bool isCaseSensitive, bool isEnabled, bool isInverse, QString sender, QString chanName); private: + /** + * Update internal cache of expression matching if needed + */ + void determineNickExpressions(const QString ¤tNick, + const QStringList identityNicks) const; + + /** + * Check if nickname matching cache is invalid + * @param currentNick + * @param identityNicks + * @return + */ + bool cacheNickInvalid(const QString ¤tNick, const QStringList identityNicks) const { + if (_cacheNickConfigInvalid) return true; + if (_cachedNickCurrent != currentNick) return true; + if (_cachedIdentityNicks != identityNicks) return true; + } + HighlightRuleList _highlightRuleList; HighlightNickType _highlightNick = HighlightNickType::CurrentNick; bool _nicksCaseSensitive = false; + + // These represent internal cache and should be safe to mutate in 'const' functions + mutable bool _cacheNickConfigInvalid = true; ///< If true, nick match cache needs redone + mutable QString _cachedNickCurrent = {}; ///< Last cached current nick + mutable QStringList _cachedIdentityNicks = {}; ///< Last cached identity nicks + mutable ExpressionMatch _cachedNickMatcher = {}; ///< Expression match cache for nicks }; diff --git a/src/qtui/settingspages/corehighlightsettingspage.cpp b/src/qtui/settingspages/corehighlightsettingspage.cpp index a11d36d9..3de8d766 100644 --- a/src/qtui/settingspages/corehighlightsettingspage.cpp +++ b/src/qtui/settingspages/corehighlightsettingspage.cpp @@ -547,26 +547,26 @@ void CoreHighlightSettingsPage::highlightTableChanged(QTableWidgetItem *item) switch (item->column()) { case CoreHighlightSettingsPage::EnableColumn: - highlightRule.isEnabled = (item->checkState() == Qt::Checked); + highlightRule.setIsEnabled(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::NameColumn: - highlightRule.name = item->text(); + highlightRule.setContents(item->text()); break; case CoreHighlightSettingsPage::RegExColumn: - highlightRule.isRegEx = (item->checkState() == Qt::Checked); + highlightRule.setIsRegEx(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::CsColumn: - highlightRule.isCaseSensitive = (item->checkState() == Qt::Checked); + highlightRule.setIsCaseSensitive(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::SenderColumn: if (!item->text().isEmpty() && item->text().trimmed().isEmpty()) item->setText(""); - highlightRule.sender = item->text(); + highlightRule.setSender(item->text()); break; case CoreHighlightSettingsPage::ChanColumn: if (!item->text().isEmpty() && item->text().trimmed().isEmpty()) item->setText(""); - highlightRule.chanName = item->text(); + highlightRule.setChanName(item->text()); break; } highlightList[item->row()] = highlightRule; @@ -584,26 +584,26 @@ void CoreHighlightSettingsPage::ignoredTableChanged(QTableWidgetItem *item) switch (item->column()) { case CoreHighlightSettingsPage::EnableColumn: - ignoredRule.isEnabled = (item->checkState() == Qt::Checked); + ignoredRule.setIsEnabled(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::NameColumn: - ignoredRule.name = item->text(); + ignoredRule.setContents(item->text()); break; case CoreHighlightSettingsPage::RegExColumn: - ignoredRule.isRegEx = (item->checkState() == Qt::Checked); + ignoredRule.setIsRegEx(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::CsColumn: - ignoredRule.isCaseSensitive = (item->checkState() == Qt::Checked); + ignoredRule.setIsCaseSensitive(item->checkState() == Qt::Checked); break; case CoreHighlightSettingsPage::SenderColumn: if (!item->text().isEmpty() && item->text().trimmed().isEmpty()) item->setText(""); - ignoredRule.sender = item->text(); + ignoredRule.setSender(item->text()); break; case CoreHighlightSettingsPage::ChanColumn: if (!item->text().isEmpty() && item->text().trimmed().isEmpty()) item->setText(""); - ignoredRule.chanName = item->text(); + ignoredRule.setChanName(item->text()); break; } ignoredList[item->row()] = ignoredRule; @@ -619,18 +619,13 @@ void CoreHighlightSettingsPage::load() auto ruleManager = Client::highlightRuleManager(); if (ruleManager) { for (auto &rule : ruleManager->highlightRuleList()) { - if (rule.isInverse) { - addNewIgnoredRow(rule.isEnabled, - rule.id, - rule.name, - rule.isRegEx, - rule.isCaseSensitive, - rule.sender, - rule.chanName); + if (rule.isInverse()) { + addNewIgnoredRow(rule.isEnabled(), rule.id(), rule.contents(), rule.isRegEx(), + rule.isCaseSensitive(), rule.sender(), rule.chanName()); } else { - addNewHighlightRow(rule.isEnabled, rule.id, rule.name, rule.isRegEx, rule.isCaseSensitive, rule.sender, - rule.chanName); + addNewHighlightRow(rule.isEnabled(), rule.id(), rule.contents(), rule.isRegEx(), + rule.isCaseSensitive(), rule.sender(), rule.chanName()); } } @@ -665,13 +660,15 @@ void CoreHighlightSettingsPage::save() clonedManager.clear(); for (auto &rule : highlightList) { - clonedManager.addHighlightRule(rule.id, rule.name, rule.isRegEx, rule.isCaseSensitive, rule.isEnabled, false, - rule.sender, rule.chanName); + clonedManager.addHighlightRule(rule.id(), rule.contents(), rule.isRegEx(), + rule.isCaseSensitive(), rule.isEnabled(), false, + rule.sender(), rule.chanName()); } for (auto &rule : ignoredList) { - clonedManager.addHighlightRule(rule.id, rule.name, rule.isRegEx, rule.isCaseSensitive, rule.isEnabled, true, - rule.sender, rule.chanName); + clonedManager.addHighlightRule(rule.id(), rule.contents(), rule.isRegEx(), + rule.isCaseSensitive (), rule.isEnabled(), true, + rule.sender(), rule.chanName()); } auto highlightNickType = ui.highlightNicksComboBox->itemData(ui.highlightNicksComboBox->currentIndex()).value(); @@ -689,13 +686,13 @@ int CoreHighlightSettingsPage::nextId() { int max = 0; for (int i = 0; i < highlightList.count(); i++) { - int id = highlightList[i].id; + int id = highlightList[i].id(); if (id > max) { max = id; } } for (int i = 0; i < ignoredList.count(); i++) { - int id = ignoredList[i].id; + int id = ignoredList[i].id(); if (id > max) { max = id; } -- 2.20.1