From 497ce2d2f27991136be08856fc79d9eacf8fbaec Mon Sep 17 00:00:00 2001
From: Leon Richardt <leon.richardt@gmail.com>
Date: Mon, 27 Jan 2020 00:16:09 +0100
Subject: [PATCH] Better Highlights: Fix Unintentional Color Update (#1522)

* HighlightPhrase: Fix wrong documentation

* Use right constructor for new HighlightPhrases

* Fix preset highlights changing unintentionally

Prior to this commit, the callback for reacting to user input on the
highlight table (namely, `HighlightingPage::tableCellClicked`) only
checked for the row number in order to determine whether preset
highlights (self highlights, whispers, and subscriptions) need to be
updated. Hence, changing rows 0 through 2 in the "User Highlights" tab
would also update the preset highlights.

This commit adds a check to determine whether the callback was triggered
by the "Messages" highlight tab, or not.
---
 .../highlights/HighlightPhrase.cpp            | 10 -----
 .../highlights/HighlightPhrase.hpp            | 10 +++++
 .../settingspages/HighlightingPage.cpp        | 39 ++++++++++++-------
 3 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/src/controllers/highlights/HighlightPhrase.cpp b/src/controllers/highlights/HighlightPhrase.cpp
index d310a921d..40942df3d 100644
--- a/src/controllers/highlights/HighlightPhrase.cpp
+++ b/src/controllers/highlights/HighlightPhrase.cpp
@@ -12,11 +12,6 @@ bool HighlightPhrase::operator==(const HighlightPhrase &other) const
                                               other.soundUrl_, other.color_);
 }
 
-/**
- * @brief Create a new HighlightPhrase.
- *
- * Use this constructor when updating an existing HighlightPhrase's color.
- */
 HighlightPhrase::HighlightPhrase(const QString &pattern, bool hasAlert,
                                  bool hasSound, bool isRegex,
                                  bool isCaseSensitive, const QString &soundUrl,
@@ -36,11 +31,6 @@ HighlightPhrase::HighlightPhrase(const QString &pattern, bool hasAlert,
     this->color_ = std::make_shared<QColor>(color);
 }
 
-/**
- * @brief Create a new HighlightPhrase.
- *
- * Use this constructor when creating a new HighlightPhrase.
- */
 HighlightPhrase::HighlightPhrase(const QString &pattern, bool hasAlert,
                                  bool hasSound, bool isRegex,
                                  bool isCaseSensitive, const QString &soundUrl,
diff --git a/src/controllers/highlights/HighlightPhrase.hpp b/src/controllers/highlights/HighlightPhrase.hpp
index 6121fc0fb..72849bb66 100644
--- a/src/controllers/highlights/HighlightPhrase.hpp
+++ b/src/controllers/highlights/HighlightPhrase.hpp
@@ -15,10 +15,20 @@ class HighlightPhrase
 public:
     bool operator==(const HighlightPhrase &other) const;
 
+    /**
+     * @brief Create a new HighlightPhrase.
+     *
+     * Use this constructor when creating a new HighlightPhrase.
+     */
     HighlightPhrase(const QString &pattern, bool hasAlert, bool hasSound,
                     bool isRegex, bool isCaseSensitive, const QString &soundUrl,
                     QColor color);
 
+    /**
+     * @brief Create a new HighlightPhrase.
+     *
+     * Use this constructor when updating an existing HighlightPhrase's color.
+     */
     HighlightPhrase(const QString &pattern, bool hasAlert, bool hasSound,
                     bool isRegex, bool isCaseSensitive, const QString &soundUrl,
                     std::shared_ptr<QColor> color);
diff --git a/src/widgets/settingspages/HighlightingPage.cpp b/src/widgets/settingspages/HighlightingPage.cpp
index e1c018aea..46cdba3b6 100644
--- a/src/widgets/settingspages/HighlightingPage.cpp
+++ b/src/widgets/settingspages/HighlightingPage.cpp
@@ -121,7 +121,7 @@ HighlightingPage::HighlightingPage()
                     getApp()->highlights->highlightedUsers.appendItem(
                         HighlightPhrase{"highlighted user", true, false, false,
                                         false, "",
-                                        ColorProvider::instance().color(
+                                        *ColorProvider::instance().color(
                                             ColorType::SelfHighlight)});
                 });
 
@@ -243,20 +243,31 @@ void HighlightingPage::tableCellClicked(const QModelIndex &clicked,
                 view->getModel()->setData(clicked, selected,
                                           Qt::DecorationRole);
 
-                // For special highlights we need to manually update the color map
-                auto instance = ColorProvider::instance();
-                switch (clicked.row())
+                // Hacky (?) way to figure out what tab the cell was clicked in
+                const bool fromMessages = (dynamic_cast<HighlightModel *>(
+                                               view->getModel()) != nullptr);
+
+                if (fromMessages)
                 {
-                    case 0:
-                        instance.updateColor(ColorType::SelfHighlight,
-                                             selected);
-                        break;
-                    case 1:
-                        instance.updateColor(ColorType::Whisper, selected);
-                        break;
-                    case 2:
-                        instance.updateColor(ColorType::Subscription, selected);
-                        break;
+                    /*
+                     * For preset highlights in the "Messages" tab, we need to
+                     * manually update the color map.
+                     */
+                    auto instance = ColorProvider::instance();
+                    switch (clicked.row())
+                    {
+                        case 0:
+                            instance.updateColor(ColorType::SelfHighlight,
+                                                 selected);
+                            break;
+                        case 1:
+                            instance.updateColor(ColorType::Whisper, selected);
+                            break;
+                        case 2:
+                            instance.updateColor(ColorType::Subscription,
+                                                 selected);
+                            break;
+                    }
                 }
             }
         });