From 0cbddf7e9bad60da4af73a762f59f3cd05f04724 Mon Sep 17 00:00:00 2001 From: pajlada Date: Sat, 22 Aug 2020 09:37:03 -0400 Subject: [PATCH] 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 | 1 + CMakeLists.txt | 12 +- .../highlights/HighlightPhrase.cpp | 19 ++- .../highlights/HighlightPhrase.hpp | 4 + src/providers/colors/ColorProvider.hpp | 5 + tests/.clang-format | 34 ++++ tests/src/HighlightPhrase.cpp | 154 ++++++++++++++++++ 7 files changed, 222 insertions(+), 7 deletions(-) create mode 100644 tests/.clang-format create mode 100644 tests/src/HighlightPhrase.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 15a72a8ef..071a69903 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4bcd983a4..29094f702 100644 --- a/CMakeLists.txt +++ b/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") diff --git a/src/controllers/highlights/HighlightPhrase.cpp b/src/controllers/highlights/HighlightPhrase.cpp index 5ffd5b360..72ae38192 100644 --- a/src/controllers/highlights/HighlightPhrase.cpp +++ b/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)) diff --git a/src/controllers/highlights/HighlightPhrase.hpp b/src/controllers/highlights/HighlightPhrase.hpp index 268ae354c..205cfb9a0 100644 --- a/src/controllers/highlights/HighlightPhrase.hpp +++ b/src/controllers/highlights/HighlightPhrase.hpp @@ -4,10 +4,14 @@ #include "util/RapidJsonSerializeQString.hpp" #include "util/RapidjsonHelpers.hpp" +#include #include #include +#include #include +#include + namespace chatterino { class HighlightPhrase diff --git a/src/providers/colors/ColorProvider.hpp b/src/providers/colors/ColorProvider.hpp index 1595c53d6..5d2d50128 100644 --- a/src/providers/colors/ColorProvider.hpp +++ b/src/providers/colors/ColorProvider.hpp @@ -1,5 +1,10 @@ #pragma once +#include +#include + +#include + namespace chatterino { enum class ColorType { diff --git a/tests/.clang-format b/tests/.clang-format new file mode 100644 index 000000000..0ef59b1fe --- /dev/null +++ b/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 diff --git a/tests/src/HighlightPhrase.cpp b/tests/src/HighlightPhrase.cpp new file mode 100644 index 000000000..731e926d1 --- /dev/null +++ b/tests/src/HighlightPhrase.cpp @@ -0,0 +1,154 @@ +#include "controllers/highlights/HighlightPhrase.hpp" + +#include + +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("!")); +}