Support key up/down to move through query history.

We follow the behavior of linux shell:
(1) Whenever there is a new query, the iterator is reset to the new
    query.
(2) Prev of the iterator moves till the first query and stick there.
(3) Next of the iterator can move beyond last query and get an empty
    query.


Bug: b:119504796
Change-Id: Iee1247359df78b8e2189fa86c3bd1b8b6ebfe612
Reviewed-on: https://chromium-review.googlesource.com/c/1334271
Commit-Queue: Yilei Li <yileili@google.com>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609888}

ash/BUILD.gn

@@ -184,6 +184,8 @@ component("ash") {
     "assistant/model/assistant_interaction_model_observer.h",
     "assistant/model/assistant_query.cc",
     "assistant/model/assistant_query.h",
+    "assistant/model/assistant_query_history.cc",
+    "assistant/model/assistant_query_history.h",
     "assistant/model/assistant_response.cc",
     "assistant/model/assistant_response.h",
     "assistant/model/assistant_screen_context_model.cc",
@@ -1625,6 +1627,7 @@ test("ash_unittests") {
     "ash_service_unittest.cc",
     "assistant/assistant_controller_unittest.cc",
     "assistant/assistant_screen_context_controller_unittest.cc",
+    "assistant/model/assistant_query_history_unittest.cc",
     "assistant/ui/assistant_container_view_unittest.cc",
     "assistant/util/deep_link_util_unittest.cc",
     "autoclick/autoclick_drag_event_rewriter_unittest.cc",

ash/assistant/assistant_interaction_controller.cc

@@ -203,6 +203,28 @@ void AssistantInteractionController::OnHighlighterSelectionRecognized(
   StartMetalayerInteraction(/*region=*/rect);
 }
 
+void AssistantInteractionController::OnCommittedQueryChanged(
+    const AssistantQuery& assistant_query) {
+  std::string query;
+  switch (assistant_query.type()) {
+    case AssistantQueryType::kText: {
+      const auto* assistant_text_query =
+          static_cast<const AssistantTextQuery*>(&assistant_query);
+      query = assistant_text_query->text();
+      break;
+    }
+    case AssistantQueryType::kVoice: {
+      const auto* assistant_voice_query =
+          static_cast<const AssistantVoiceQuery*>(&assistant_query);
+      query = assistant_voice_query->high_confidence_speech();
+      break;
+    }
+    case AssistantQueryType::kNull:
+      break;
+  }
+  model_.query_history().Add(query);
+}
+
 void AssistantInteractionController::OnInteractionStateChanged(
     InteractionState interaction_state) {
   if (interaction_state != InteractionState::kActive)

ash/assistant/assistant_interaction_controller.h

@@ -67,6 +67,7 @@ class AssistantInteractionController
   void OnInteractionStateChanged(InteractionState interaction_state) override;
   void OnInputModalityChanged(InputModality input_modality) override;
   void OnMicStateChanged(MicState mic_state) override;
+  void OnCommittedQueryChanged(const AssistantQuery& query) override;
   void OnResponseChanged(
       const std::shared_ptr<AssistantResponse>& response) override;
 

ash/assistant/model/assistant_interaction_model.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/assistant/model/assistant_query_history.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 
@@ -119,6 +120,12 @@ class AssistantInteractionModel {
   // Updates the speech level in dB.
   void SetSpeechLevel(float speech_level_db);
 
+  // Returns the reference to query history.
+  AssistantQueryHistory& query_history() { return query_history_; }
+
+  // Returns the const reference to query history.
+  const AssistantQueryHistory& query_history() const { return query_history_; }
+
  private:
   void NotifyInteractionStateChanged();
   void NotifyInputModalityChanged();
@@ -134,6 +141,7 @@ class AssistantInteractionModel {
   InteractionState interaction_state_ = InteractionState::kInactive;
   InputModality input_modality_ = InputModality::kKeyboard;
   MicState mic_state_ = MicState::kClosed;
+  AssistantQueryHistory query_history_;
   std::unique_ptr<AssistantQuery> committed_query_;
   std::unique_ptr<AssistantQuery> pending_query_;
   std::unique_ptr<AssistantResponse> pending_response_;

ash/assistant/model/assistant_query_history.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/assistant/model/assistant_query_history.h"
+
+namespace ash {
+
+AssistantQueryHistory::AssistantQueryHistory(int capacity)
+    : capacity_(capacity) {
+  queries_.reserve(capacity);
+}
+
+AssistantQueryHistory::~AssistantQueryHistory() = default;
+
+std::unique_ptr<AssistantQueryHistory::Iterator>
+AssistantQueryHistory::GetIterator() const {
+  return std::make_unique<AssistantQueryHistory::Iterator>(queries_);
+}
+
+void AssistantQueryHistory::Add(const std::string& query) {
+  if (query.empty())
+    return;
+
+  if (static_cast<int>(queries_.size()) == capacity_)
+    queries_.pop_front();
+  queries_.push_back(query);
+}
+
+AssistantQueryHistory::Iterator::Iterator(
+    const base::circular_deque<std::string>& queries)
+    : queries_(queries), cur_pos_(queries_.size()) {}
+
+AssistantQueryHistory::Iterator::~Iterator() = default;
+
+base::Optional<std::string> AssistantQueryHistory::Iterator::Next() {
+  // queries_.size() is of type unsigned int and queries_.size() -1 will
+  // overflow if it is 0.
+  if (cur_pos_ + 1 >= queries_.size()) {
+    cur_pos_ = queries_.size();
+    return base::nullopt;
+  }
+  cur_pos_++;
+  return base::make_optional<std::string>(queries_[cur_pos_]);
+}
+
+base::Optional<std::string> AssistantQueryHistory::Iterator::Prev() {
+  if (queries_.size() == 0)
+    return base::nullopt;
+
+  if (cur_pos_ != 0)
+    cur_pos_--;
+
+  return base::make_optional<std::string>(queries_[cur_pos_]);
+}
+
+void AssistantQueryHistory::Iterator::ResetToLast() {
+  cur_pos_ = queries_.size();
+}
+
+}  // namespace ash

ash/assistant/model/assistant_query_history.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_ASSISTANT_MODEL_ASSISTANT_QUERY_HISTORY_H_
+#define ASH_ASSISTANT_MODEL_ASSISTANT_QUERY_HISTORY_H_
+
+#include <memory>
+#include <string>
+
+#include "base/containers/circular_deque.h"
+#include "base/macros.h"
+#include "base/optional.h"
+
+namespace ash {
+
+// Caches user query history.
+class AssistantQueryHistory {
+ public:
+  class Iterator {
+   public:
+    Iterator(const base::circular_deque<std::string>& queries);
+    ~Iterator();
+
+    // Fetches the next query. If current is already the last query, or there is
+    // no query in history, returns nullopt.
+    base::Optional<std::string> Next();
+
+    // Fetches the previous query. If current is already the first query, return
+    // the first query. If there is no query in history, returns nullopt.
+    base::Optional<std::string> Prev();
+
+    // Resets to the last query. It also makes current iterator valid again if
+    // new queries are added to the underlying AssistantQueryHistory.
+    void ResetToLast();
+
+   private:
+    const base::circular_deque<std::string>& queries_;
+    size_t cur_pos_;
+
+    DISALLOW_COPY_AND_ASSIGN(Iterator);
+  };
+
+  AssistantQueryHistory(int capacity = 100);
+  ~AssistantQueryHistory();
+
+  // Gets the iterator of query history.
+  std::unique_ptr<Iterator> GetIterator() const;
+
+  // Adds a query to history. If it is empty, ignore it.
+  void Add(const std::string& query);
+
+ private:
+  const int capacity_;
+  base::circular_deque<std::string> queries_;
+
+  DISALLOW_COPY_AND_ASSIGN(AssistantQueryHistory);
+};
+
+}  // namespace ash
+
+#endif  // ASH_ASSISTANT_MODEL_ASSISTANT_QUERY_HISTORY_H_

ash/assistant/model/assistant_query_history_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/assistant/model/assistant_query_history.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ash {
+
+// Assert that iterator Prev() or Next() does not crash on an empty history.
+TEST(AssistantQueryHistory, Empty) {
+  AssistantQueryHistory history(10);
+  auto it = history.GetIterator();
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Prev());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Prev());
+}
+
+TEST(AssistantQueryHistory, Full) {
+  int size = 3;
+  AssistantQueryHistory history(size);
+  // Make more queries than history limit.
+  for (int i = 0; i <= size; i++)
+    history.Add(std::to_string(i));
+  auto it = history.GetIterator();
+  // Assert history only contains last 3 queries.
+  for (int i = size; i > 0; i--)
+    EXPECT_EQ(std::to_string(i), it->Prev().value());
+  EXPECT_EQ(std::to_string(1), it->Prev().value());
+  // Assert that iterate does not pass first query.
+  EXPECT_EQ(std::to_string(1), it->Prev().value());
+
+  // Make more queries than history limit again.
+  for (int i = 0; i <= size; i++)
+    history.Add(std::to_string(i + 4));
+  it->ResetToLast();
+  // Assert that history only contains last 3 queries.
+  for (int i = size; i > 0; i--)
+    EXPECT_EQ(std::to_string(i + 4), it->Prev());
+  EXPECT_EQ(std::to_string(5), it->Prev().value());
+  // Assert that iterate does not pass first query.
+  EXPECT_EQ(std::to_string(5), it->Prev().value());
+}
+
+TEST(AssistantQueryHistory, Add) {
+  AssistantQueryHistory history(10);
+  auto it = history.GetIterator();
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+  history.Add("Query01");
+  history.Add("Query02");
+  it = history.GetIterator();
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ("Query02", it->Prev().value());
+  EXPECT_EQ("Query01", it->Prev().value());
+  EXPECT_EQ("Query01", it->Prev().value());
+  EXPECT_EQ("Query02", it->Next().value());
+  EXPECT_EQ(base::nullopt, it->Next());
+  EXPECT_EQ(base::nullopt, it->Next());
+}
+
+}  // namespace ash

ash/assistant/ui/dialog_plate/dialog_plate.cc

@@ -69,7 +69,11 @@ DialogPlate::DialogPlate(AssistantController* assistant_controller)
               base::Unretained(this)),
           /*end_animation_callback=*/base::BindRepeating(
               &DialogPlate::OnAnimationEnded,
-              base::Unretained(this)))) {
+              base::Unretained(this)))),
+      query_history_iterator_(assistant_controller_->interaction_controller()
+                                  ->model()
+                                  ->query_history()
+                                  .GetIterator()) {
   InitLayout();
 
   // The Assistant controller indirectly owns the view hierarchy to which
@@ -109,30 +113,43 @@ void DialogPlate::ButtonPressed(views::Button* sender, const ui::Event& event) {
 
 bool DialogPlate::HandleKeyEvent(views::Textfield* textfield,
                                  const ui::KeyEvent& key_event) {
-  if (key_event.key_code() != ui::KeyboardCode::VKEY_RETURN)
-    return false;
-
   if (key_event.type() != ui::EventType::ET_KEY_PRESSED)
     return false;
 
+  switch (key_event.key_code()) {
+    case ui::KeyboardCode::VKEY_RETURN: {
       // In tablet mode the virtual keyboard should not be sticky, so we hide it
       // when committing a query.
       if (IsTabletMode())
         textfield_->GetFocusManager()->ClearFocus();
 
-  const base::StringPiece16& trimmed_text =
-      base::TrimWhitespace(textfield_->text(), base::TrimPositions::TRIM_ALL);
+      const base::StringPiece16& trimmed_text = base::TrimWhitespace(
+          textfield_->text(), base::TrimPositions::TRIM_ALL);
 
-  // Only non-empty trimmed text is consider a valid contents commit. Anything
-  // else will simply result in the DialogPlate being cleared.
+      // Only non-empty trimmed text is consider a valid contents commit.
+      // Anything else will simply result in the DialogPlate being cleared.
       if (!trimmed_text.empty()) {
         for (DialogPlateObserver& observer : observers_)
-      observer.OnDialogPlateContentsCommitted(base::UTF16ToUTF8(trimmed_text));
+          observer.OnDialogPlateContentsCommitted(
+              base::UTF16ToUTF8(trimmed_text));
       }
 
       textfield_->SetText(base::string16());
 
       return true;
+    }
+    case ui::KeyboardCode::VKEY_UP:
+    case ui::KeyboardCode::VKEY_DOWN: {
+      DCHECK(query_history_iterator_);
+      auto opt_query = key_event.key_code() == ui::KeyboardCode::VKEY_UP
+                           ? query_history_iterator_->Prev()
+                           : query_history_iterator_->Next();
+      textfield_->SetText(base::UTF8ToUTF16(opt_query.value_or("")));
+      return true;
+    }
+    default:
+      return false;
+  }
 }
 
 void DialogPlate::OnInputModalityChanged(InputModality input_modality) {
@@ -219,6 +236,12 @@ void DialogPlate::OnInputModalityChanged(InputModality input_modality) {
   }
 }
 
+void DialogPlate::OnCommittedQueryChanged(
+    const AssistantQuery& committed_query) {
+  DCHECK(query_history_iterator_);
+  query_history_iterator_->ResetToLast();
+}
+
 void DialogPlate::OnUiVisibilityChanged(AssistantVisibility new_visibility,
                                         AssistantVisibility old_visibility,
                                         AssistantSource source) {

ash/assistant/ui/dialog_plate/dialog_plate.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "ash/assistant/model/assistant_interaction_model_observer.h"
+#include "ash/assistant/model/assistant_query_history.h"
 #include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/assistant/ui/dialog_plate/action_view.h"
 #include "base/macros.h"
@@ -87,6 +88,7 @@ class DialogPlate : public views::View,
 
   // AssistantInteractionModelObserver:
   void OnInputModalityChanged(InputModality input_modality) override;
+  void OnCommittedQueryChanged(const AssistantQuery& committed_query) override;
 
   // AssistantUiModelObserver:
   void OnUiVisibilityChanged(AssistantVisibility new_visibility,
@@ -120,6 +122,7 @@ class DialogPlate : public views::View,
   views::Textfield* textfield_;                      // Owned by view hierarchy.
 
   std::unique_ptr<ui::CallbackLayerAnimationObserver> animation_observer_;
+  std::unique_ptr<AssistantQueryHistory::Iterator> query_history_iterator_;
 
   base::ObserverList<DialogPlateObserver>::Unchecked observers_;