Review fixes.beta-697

5 files changed, 43 insertions, 45 deletions

diff --git a/search/search_quality/assessment_tool/edits.cpp b/search/search_quality/assessment_tool/edits.cpp
index 8e57778acb..dab85f074a 100644
--- a/search/search_quality/assessment_tool/edits.cpp
+++ b/search/search_quality/assessment_tool/edits.cpp
@@ -4,63 +4,62 @@
 
 // Edits::RelevanceEditor --------------------------------------------------------------------------
 Edits::RelevanceEditor::RelevanceEditor(Edits & parent, size_t index)
-  : m_parent(&parent), m_index(index)
+  : m_parent(parent), m_index(index)
 {
 }
 
 bool Edits::RelevanceEditor::Set(Relevance relevance)
 {
-  CHECK(IsValid(), ());
-  return m_parent->UpdateRelevance(m_index, relevance);
+  return m_parent.UpdateRelevance(m_index, relevance);
 }
 
 Edits::Relevance Edits::RelevanceEditor::Get() const
 {
-  CHECK(IsValid(), ());
-  auto const & relevances = m_parent->GetRelevances();
+  auto const & relevances = m_parent.GetRelevances();
   CHECK_LESS(m_index, relevances.size(), ());
   return relevances[m_index];
 }
 
 // Edits -------------------------------------------------------------------------------------------
-Edits::Edits(Delegate & delegate) : m_delegate(delegate) {}
+Edits::Edits(Observer & observer) : m_observer(observer) {}
 
 void Edits::ResetRelevances(std::vector<Relevance> const & relevances)
 {
-  WithDelegate([this, &relevances]() {
+  WithObserver([this, &relevances]() {
     m_origRelevances = relevances;
     m_currRelevances = relevances;
-    m_relevanceEdits.clear();
+    m_numEdits = 0;
   });
 }
 
 bool Edits::UpdateRelevance(size_t index, Relevance relevance)
 {
-  return WithDelegate([this, index, relevance]() {
+  return WithObserver([this, index, relevance]() {
     CHECK_LESS(index, m_currRelevances.size(), ());
-    m_currRelevances[index] = relevance;
-
     CHECK_EQUAL(m_currRelevances.size(), m_origRelevances.size(), ());
-    if (m_currRelevances[index] != m_origRelevances[index])
+
+    if (m_currRelevances[index] != m_origRelevances[index] && relevance == m_origRelevances[index])
     {
-      m_relevanceEdits.insert(index);
-      return true;
+      --m_numEdits;
     }
-    else
+    else if (m_currRelevances[index] == m_origRelevances[index] &&
+             relevance != m_origRelevances[index])
     {
-      m_relevanceEdits.erase(index);
-      return false;
+      ++m_numEdits;
     }
+
+    m_currRelevances[index] = relevance;
+    return m_currRelevances[index] != m_origRelevances[index];
   });
 }
 
 void Edits::Clear()
 {
-  WithDelegate([this]() {
+  WithObserver([this]() {
     m_origRelevances.clear();
     m_currRelevances.clear();
-    m_relevanceEdits.clear();
+    m_numEdits = 0;
   });
 }
 
-bool Edits::HasChanges() const { return !m_relevanceEdits.empty(); }
+bool Edits::HasChanges() const { return m_numEdits != 0; }
diff --git a/search/search_quality/assessment_tool/edits.hpp b/search/search_quality/assessment_tool/edits.hpp
index 0944240fa2..9dbfb1c3e6 100644
--- a/search/search_quality/assessment_tool/edits.hpp
+++ b/search/search_quality/assessment_tool/edits.hpp
@@ -4,7 +4,7 @@
 
 #include "base/scope_guard.hpp"
 
-#include <set>
+#include <cstddef>
 #include <type_traits>
 #include <vector>
 
@@ -16,28 +16,25 @@ public:
   class RelevanceEditor
   {
   public:
-    RelevanceEditor() = default;
     RelevanceEditor(Edits & parent, size_t index);
 
-    bool IsValid() const { return m_parent != nullptr; }
-
     // Sets relevance to |relevance|. Returns true iff |relevance|
     // differs from the original one.
     bool Set(Relevance relevance);
     Relevance Get() const;
 
   private:
-    Edits * m_parent = nullptr;
+    Edits & m_parent;
     size_t m_index = 0;
   };
 
-  struct Delegate
+  struct Observer
   {
-    virtual ~Delegate() = default;
+    virtual ~Observer() = default;
     virtual void OnUpdate() = 0;
   };
 
-  Edits(Delegate & delegate);
+  explicit Edits(Observer & observer);
 
   void ResetRelevances(std::vector<Relevance> const & relevances);
 
@@ -52,16 +49,16 @@ public:
 
 private:
   template <typename Fn>
-  typename std::result_of<Fn()>::type WithDelegate(Fn && fn)
+  typename std::result_of<Fn()>::type WithObserver(Fn && fn)
   {
-    MY_SCOPE_GUARD(cleanup, [this]() { m_delegate.OnUpdate(); });
+    MY_SCOPE_GUARD(cleanup, [this]() { m_observer.OnUpdate(); });
     return fn();
   }
 
-  Delegate & m_delegate;
+  Observer & m_observer;
 
   std::vector<Relevance> m_origRelevances;
   std::vector<Relevance> m_currRelevances;
 
-  std::set<size_t> m_relevanceEdits;
+  size_t m_numEdits = 0;
 };
diff --git a/search/search_quality/assessment_tool/result_view.cpp b/search/search_quality/assessment_tool/result_view.cpp
index d66b777fca..faeb8e3da1 100644
--- a/search/search_quality/assessment_tool/result_view.cpp
+++ b/search/search_quality/assessment_tool/result_view.cpp
@@ -3,7 +3,10 @@
 #include "search/result.hpp"
 #include "search/search_quality/assessment_tool/helpers.hpp"
 
+#include "base/stl_add.hpp"
+
 #include <string>
+#include <utility>
 
 #include <QtWidgets/QHBoxLayout>
 #include <QtWidgets/QLabel>
@@ -40,18 +43,15 @@ ResultView::ResultView(search::Result const & result, QWidget & parent) : QWidge
   setObjectName("result");
 }
 
-void ResultView::EnableEditing(Edits::RelevanceEditor editor)
+void ResultView::EnableEditing(Edits::RelevanceEditor && editor)
 {
-  if (!editor.IsValid())
-    return;
-
-  m_editor = editor;
+  m_editor = my::make_unique<Edits::RelevanceEditor>(std::move(editor));
 
   m_irrelevant->setChecked(false);
   m_relevant->setChecked(false);
   m_vital->setChecked(false);
 
-  switch (m_editor.Get())
+  switch (m_editor->Get())
   {
   case Relevance::Irrelevant: m_irrelevant->setChecked(true); break;
   case Relevance::Relevant: m_relevant->setChecked(true); break;
@@ -113,7 +113,7 @@ QRadioButton * ResultView::CreateRatioButton(string const & text, QLayout & layo
 
 void ResultView::OnRelevanceChanged()
 {
-  if (!m_editor.IsValid())
+  if (!m_editor)
     return;
 
   auto relevance = Relevance::Irrelevant;
@@ -124,7 +124,7 @@ void ResultView::OnRelevanceChanged()
   else if (m_vital->isChecked())
     relevance = Relevance::Vital;
 
-  bool changed = m_editor.Set(relevance);
+  bool changed = m_editor->Set(relevance);
   if (changed)
     setStyleSheet("#result {background: rgba(255, 255, 200, 50%)}");
   else
diff --git a/search/search_quality/assessment_tool/result_view.hpp b/search/search_quality/assessment_tool/result_view.hpp
index 04312fb269..1201928b9b 100644
--- a/search/search_quality/assessment_tool/result_view.hpp
+++ b/search/search_quality/assessment_tool/result_view.hpp
@@ -3,6 +3,8 @@
 #include "search/search_quality/assessment_tool/edits.hpp"
 #include "search/search_quality/sample.hpp"
 
+#include <memory>
+
 #include <QtWidgets/QWidget>
 
 class QLabel;
@@ -20,7 +22,7 @@ public:
 
   ResultView(search::Result const & result, QWidget & parent);
 
-  void EnableEditing(Edits::RelevanceEditor editor);
+  void EnableEditing(Edits::RelevanceEditor && editor);
 
 private:
   void Init();
@@ -37,5 +39,5 @@ private:
   QRadioButton * m_relevant = nullptr;
   QRadioButton * m_vital = nullptr;
 
-  Edits::RelevanceEditor m_editor;
+  std::unique_ptr<Edits::RelevanceEditor> m_editor;
 };
diff --git a/search/search_quality/assessment_tool/sample_view.hpp b/search/search_quality/assessment_tool/sample_view.hpp
index 0dc891fa4b..7030696354 100644
--- a/search/search_quality/assessment_tool/sample_view.hpp
+++ b/search/search_quality/assessment_tool/sample_view.hpp
@@ -10,7 +10,7 @@ class LanguagesList;
 class QLineEdit;
 class ResultsView;
 
-class SampleView : public QWidget, public Edits::Delegate
+class SampleView : public QWidget, public Edits::Observer
 {
   Q_OBJECT
 
@@ -23,8 +23,8 @@ public:
   void ShowResults(search::Results::Iter begin, search::Results::Iter end);
   void SetResultRelevances(std::vector<Relevance> const & relevances);
 
-  // Edits::Delegate overrides:
-  void OnUpdate();
+  // Edits::Observer overrides:
+  void OnUpdate() override;
 
 signals:
   void EditStateUpdated(bool hasEdits);