Fix/be respectful of special characters like exclamation marks in highlight phrases since they are also word boundaries (#1890)

* Add missing includes

We would normally have these included in another file already, or even
the precompiled headers, but having the files included here too makes
testing single parts easier.

* Modify the regex building of highlight phrases for non-regex phrases

For phrases like !test, the word boundary checking we did before was not
enough, so we now check for either a word boundary, a whitespace
character, or the line start/end.

* Add tests for ensuring I haven't fully broken the highlight system

* Add changelog entry

CHANGELOG.md

@@ -16,6 +16,7 @@
 - Minor: Removed "Online Logs" functionality as services are shut down (#1640)
 - Minor: CTRL+F now selects the Find text input field in the Settings Dialog (#1806 #1811)
 - Minor: CTRL+F now selects the search text input field in the Search Popup (#1812)
+- Minor: Modify our word boundary logic in highlight phrase searching to accomodate non-regex phrases with "word-boundary-creating" characters like ! (#1885, #1890)
 - Bugfix: Fixed not being able to open links in incognito with Microsoft Edge (Chromium) (#1875)
 - Bugfix: Fix the incorrect `Open stream in browser` labelling in the whisper split (#1860)
 - Bugfix: Fix preview on hover not working when Animated emotes options was disabled (#1546)

CMakeLists.txt

@@ -6,11 +6,11 @@ include_directories(src)
 
 set(chatterino_SOURCES
     src/common/UsernameSet.cpp
+    src/controllers/highlights/HighlightPhrase.cpp
     )
 
-find_package(Qt5Widgets CONFIG REQUIRED)
 find_package(Qt5 5.9.0 REQUIRED COMPONENTS
-    Core
+    Core Widgets
     )
 
 # set(CMAKE_AUTOMOC ON)
@@ -25,12 +25,18 @@ if (BUILD_TESTS)
 
         tests/src/main.cpp
         tests/src/UsernameSet.cpp
+        tests/src/HighlightPhrase.cpp
         )
 
-    target_link_libraries(chatterino-test Qt5::Core)
+    target_link_libraries(chatterino-test Qt5::Core Qt5::Widgets)
 
     target_link_libraries(chatterino-test gtest gtest_main)
 
+    set(BUILD_TESTS OFF)
+    add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/lib/serialize PajladaSerialize)
+
+    target_link_libraries(chatterino-test PajladaSerialize)
+
     gtest_discover_tests(chatterino-test)
 else()
     message(FATAL_ERROR "This cmake file is only intended for tests right now. Use qmake to build chatterino2")

src/controllers/highlights/HighlightPhrase.cpp

@@ -2,6 +2,13 @@
 
 namespace chatterino {
 
+namespace {
+
+    const QString REGEX_START_BOUNDARY("(\\b|\\s|^)");
+    const QString REGEX_END_BOUNDARY("(\\b|\\s|$)");
+
+}  // namespace
+
 QColor HighlightPhrase::FALLBACK_HIGHLIGHT_COLOR = QColor(127, 63, 73, 127);
 QColor HighlightPhrase::FALLBACK_REDEEMED_HIGHLIGHT_COLOR =
     QColor(28, 126, 141, 90);
@@ -27,8 +34,10 @@ HighlightPhrase::HighlightPhrase(const QString &pattern, bool hasAlert,
     , isRegex_(isRegex)
     , isCaseSensitive_(isCaseSensitive)
     , soundUrl_(soundUrl)
-    , regex_(isRegex_ ? pattern
-                      : "\\b" + QRegularExpression::escape(pattern) + "\\b",
+    , regex_(isRegex_
+                 ? pattern
+                 : REGEX_START_BOUNDARY + QRegularExpression::escape(pattern) +
+                       REGEX_END_BOUNDARY,
              QRegularExpression::UseUnicodePropertiesOption |
                  (isCaseSensitive_ ? QRegularExpression::NoPatternOption
                                    : QRegularExpression::CaseInsensitiveOption))
@@ -47,8 +56,10 @@ HighlightPhrase::HighlightPhrase(const QString &pattern, bool hasAlert,
     , isCaseSensitive_(isCaseSensitive)
     , soundUrl_(soundUrl)
     , color_(color)
-    , regex_(isRegex_ ? pattern
-                      : "\\b" + QRegularExpression::escape(pattern) + "\\b",
+    , regex_(isRegex_
+                 ? pattern
+                 : REGEX_START_BOUNDARY + QRegularExpression::escape(pattern) +
+                       REGEX_END_BOUNDARY,
              QRegularExpression::UseUnicodePropertiesOption |
                  (isCaseSensitive_ ? QRegularExpression::NoPatternOption
                                    : QRegularExpression::CaseInsensitiveOption))

src/controllers/highlights/HighlightPhrase.hpp

@@ -4,10 +4,14 @@
 #include "util/RapidJsonSerializeQString.hpp"
 #include "util/RapidjsonHelpers.hpp"
 
+#include <QColor>
 #include <QRegularExpression>
 #include <QString>
+#include <QUrl>
 #include <pajlada/serialize.hpp>
 
+#include <memory>
+
 namespace chatterino {
 
 class HighlightPhrase

src/providers/colors/ColorProvider.hpp

@@ -1,5 +1,10 @@
 #pragma once
 
+#include <memory>
+#include <unordered_map>
+
+#include <QColor>
+
 namespace chatterino {
 
 enum class ColorType {

tests/.clang-format

@@ -0,0 +1,34 @@
+Language: Cpp
+
+AccessModifierOffset: -4
+AlignEscapedNewlinesLeft: true
+AllowShortFunctionsOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: false
+AllowShortLoopsOnASingleLine: false
+AlwaysBreakAfterDefinitionReturnType: false
+AlwaysBreakBeforeMultilineStrings: false
+BasedOnStyle: Google
+BraceWrapping: {
+    AfterClass: 'true'
+    AfterControlStatement: 'true'
+    AfterFunction: 'true'
+    AfterNamespace: 'false'
+    BeforeCatch: 'true'
+    BeforeElse: 'true'
+}
+BreakBeforeBraces: Custom
+BreakConstructorInitializersBeforeComma: true
+ColumnLimit: 80
+ConstructorInitializerAllOnOneLineOrOnePerLine: false
+DerivePointerBinding: false
+FixNamespaceComments: true
+IndentCaseLabels: true
+IndentWidth: 4
+IndentWrappedFunctionNames: true
+IndentPPDirectives: AfterHash
+IncludeBlocks: Preserve
+NamespaceIndentation: Inner
+PointerBindsToType: false
+SpacesBeforeTrailingComments: 2
+Standard: Auto
+ReflowComments: false

tests/src/HighlightPhrase.cpp

@@ -0,0 +1,154 @@
+#include "controllers/highlights/HighlightPhrase.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace chatterino;
+
+namespace {
+
+HighlightPhrase buildHighlightPhrase(const QString &phrase, bool isRegex,
+                                     bool isCaseSensitive)
+{
+    return HighlightPhrase(phrase, false, false, isRegex, isCaseSensitive, "",
+                           QColor());
+}
+
+}  // namespace
+
+TEST(HighlightPhrase, Normal)
+{
+    constexpr bool isCaseSensitive = false;
+    constexpr bool isRegex = false;
+
+    auto p = buildHighlightPhrase("test", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("test"));
+    EXPECT_TRUE(p.isMatch("TEst"));
+    EXPECT_TRUE(p.isMatch("foo tEst"));
+    EXPECT_TRUE(p.isMatch("foo teSt bar"));
+    EXPECT_TRUE(p.isMatch("test bar"));
+
+    EXPECT_TRUE(p.isMatch("!teSt"));
+    EXPECT_TRUE(p.isMatch("test!"));
+
+    EXPECT_FALSE(p.isMatch("testbar"));
+    EXPECT_FALSE(p.isMatch("footest"));
+    EXPECT_FALSE(p.isMatch("footestbar"));
+
+    p = buildHighlightPhrase("!test", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("!test"));
+    EXPECT_TRUE(p.isMatch("foo !test"));
+    EXPECT_TRUE(p.isMatch("foo !test bar"));
+    EXPECT_TRUE(p.isMatch("!test bar"));
+
+    EXPECT_TRUE(p.isMatch("!test"));
+    EXPECT_FALSE(p.isMatch("test!"));
+
+    EXPECT_FALSE(p.isMatch("!testbar"));
+    EXPECT_TRUE(p.isMatch(
+        "foo!test"));  // Consequence of matching on ! is that before the ! there will be a word boundary, assuming text is smashed right before it EXPECT_FALSE(p.isMatch("foo!testbar"));
+    EXPECT_FALSE(p.isMatch("footest!bar"));
+
+    p = buildHighlightPhrase("test!", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("test!"));
+    EXPECT_TRUE(p.isMatch("foo test!"));
+    EXPECT_TRUE(p.isMatch("foo test! bar"));
+    EXPECT_TRUE(p.isMatch("test! bar"));
+
+    EXPECT_TRUE(p.isMatch("test!"));
+    EXPECT_FALSE(p.isMatch("test"));
+
+    EXPECT_TRUE(p.isMatch(
+        "test!bar"));  // Consequence of matching on ! is that before the ! there will be a word boundary, assuming text is smashed right before it
+    EXPECT_FALSE(p.isMatch("footest!"));
+    EXPECT_FALSE(p.isMatch("footest!bar"));
+}
+
+TEST(HighlightPhrase, CaseSensitive)
+{
+    constexpr bool isCaseSensitive = true;
+    constexpr bool isRegex = false;
+
+    using namespace chatterino;
+
+    auto p = buildHighlightPhrase("test", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("test"));
+    EXPECT_TRUE(p.isMatch("test"));
+    EXPECT_TRUE(p.isMatch("foo test"));
+    EXPECT_TRUE(p.isMatch("foo test bar"));
+    EXPECT_FALSE(p.isMatch("TEst"));
+    EXPECT_FALSE(p.isMatch("foo tEst"));
+    EXPECT_FALSE(p.isMatch("foo teSt bar"));
+    EXPECT_TRUE(p.isMatch("test bar"));
+
+    EXPECT_TRUE(p.isMatch("!test"));
+    EXPECT_FALSE(p.isMatch("!teSt"));
+    EXPECT_TRUE(p.isMatch("test!"));
+
+    EXPECT_FALSE(p.isMatch("testbar"));
+    EXPECT_FALSE(p.isMatch("footest"));
+    EXPECT_FALSE(p.isMatch("footestbar"));
+
+    p = buildHighlightPhrase("!test", isRegex, isCaseSensitive);
+
+    EXPECT_FALSE(p.isMatch("!teSt"));
+    EXPECT_TRUE(p.isMatch("!test"));
+    EXPECT_TRUE(p.isMatch("foo !test"));
+    EXPECT_TRUE(p.isMatch("foo !test bar"));
+    EXPECT_TRUE(p.isMatch("!test bar"));
+
+    EXPECT_TRUE(p.isMatch("!test"));
+    EXPECT_FALSE(p.isMatch("test!"));
+
+    EXPECT_FALSE(p.isMatch("!testbar"));
+    EXPECT_TRUE(p.isMatch(
+        "foo!test"));  // Consequence of matching on ! is that before the ! there will be a word boundary, assuming text is smashed right before it EXPECT_FALSE(p.isMatch("foo!testbar"));
+    EXPECT_FALSE(p.isMatch("footest!bar"));
+
+    p = buildHighlightPhrase("test!", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("test!"));
+    EXPECT_FALSE(p.isMatch("teSt!"));
+    EXPECT_TRUE(p.isMatch("foo test!"));
+    EXPECT_TRUE(p.isMatch("foo test! bar"));
+    EXPECT_TRUE(p.isMatch("test! bar"));
+
+    EXPECT_TRUE(p.isMatch("test!"));
+    EXPECT_FALSE(p.isMatch("test"));
+
+    EXPECT_TRUE(p.isMatch(
+        "test!bar"));  // Consequence of matching on ! is that before the ! there will be a word boundary, assuming text is smashed right before it
+    EXPECT_FALSE(p.isMatch("footest!"));
+    EXPECT_FALSE(p.isMatch("footest!bar"));
+}
+
+TEST(HighlightPhrase, Regex)
+{
+    constexpr bool isCaseSensitive = false;
+    constexpr bool isRegex = true;
+
+    using namespace chatterino;
+
+    auto p = buildHighlightPhrase("[a-z]+", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("foo"));
+    EXPECT_TRUE(p.isMatch("foo bar"));
+    EXPECT_FALSE(p.isMatch("!"));
+
+    p = buildHighlightPhrase("^[a-z]+$", isRegex, isCaseSensitive);
+
+    EXPECT_TRUE(p.isMatch("foo"));
+    EXPECT_FALSE(p.isMatch("foo bar"));
+    EXPECT_FALSE(p.isMatch("!"));
+
+    p = buildHighlightPhrase("^[a-z]+ [a-z]+", isRegex, isCaseSensitive);
+
+    EXPECT_FALSE(p.isMatch("foo"));
+    EXPECT_TRUE(p.isMatch("foo bar"));
+    EXPECT_TRUE(p.isMatch("foo bar baz"));
+    EXPECT_FALSE(p.isMatch("!foo bar"));
+    EXPECT_FALSE(p.isMatch("!"));
+}