assistant: fix history not working with first key press

Test: unittests and local test
Bug: b:139438276
Change-Id: Ia17fe0126a7f400553e0954ffc471629b4884fa6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1935153
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719437}

ash/app_list/app_list_presenter_delegate_impl.cc

@@ -313,6 +313,10 @@ void AppListPresenterDelegateImpl::OnKeyEvent(ui::KeyEvent* event) {
   if (view_->search_box_view()->is_search_box_active())
     return;
 
+  // Don't absorb the first event when showing Assistant.
+  if (view_->IsShowingEmbeddedAssistantUI())
+    return;
+
   // Arrow keys or Tab will engage the traversal mode.
   if ((IsUnhandledArrowKeyEvent(*event) || event->key_code() == ui::VKEY_TAB)) {
     // Handle the first arrow key event to just show the focus rings.

ash/app_list/views/app_list_view.cc

@@ -5,6 +5,7 @@
 #include "ash/app_list/views/app_list_view.h"
 
 #include <algorithm>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -839,6 +840,10 @@ SkColor AppListView::GetAppListBackgroundShieldColorForTest() {
   return app_list_background_shield_->GetColorForTest();
 }
 
+bool AppListView::IsShowingEmbeddedAssistantUI() const {
+  return app_list_main_view()->contents_view()->IsShowingEmbeddedAssistantUI();
+}
+
 void AppListView::UpdateAppListConfig(aura::Window* parent_window) {
   // For side shelf, extra horizontal margin is needed to ensure the apps grid
   // does not overlap with shelf.
@@ -2247,10 +2252,6 @@ bool AppListView::ShouldIgnoreScrollEvents() {
          GetRootAppsGridView()->pagination_model()->has_transition();
 }
 
-bool AppListView::IsShowingEmbeddedAssistantUI() const {
-  return app_list_main_view()->contents_view()->IsShowingEmbeddedAssistantUI();
-}
-
 int AppListView::GetPreferredWidgetYForState(
     ash::AppListViewState state) const {
   // Note that app list container fills the screen, so we can treat the

ash/app_list/views/app_list_view.h

@@ -6,6 +6,7 @@
 #define ASH_APP_LIST_VIEWS_APP_LIST_VIEW_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "ash/app_list/app_list_export.h"
@@ -55,14 +56,10 @@ FORWARD_DECLARE_TEST(AppListControllerImplTest,
 FORWARD_DECLARE_TEST(AppListControllerImplMetricsTest,
                      PresentationTimeRecordedForDragInTabletMode);
 
-namespace {
-
 // The fraction of app list height that the app list must be released at in
 // order to transition to the next state.
 constexpr int kAppListThresholdDenominator = 3;
 
-}  // namespace
-
 // AppListView is the top-level view and controller of app list UI. It creates
 // and hosts a AppsGridView and passes AppListModel to it for display.
 // TODO(newcomer|weidongg): Organize the cc file to match the order of
@@ -367,6 +364,9 @@ class APP_LIST_EXPORT AppListView : public views::WidgetDelegateView,
 
   SkColor GetAppListBackgroundShieldColorForTest();
 
+  // Returns true if the Embedded Assistant UI is currently being shown.
+  bool IsShowingEmbeddedAssistantUI() const;
+
  private:
   FRIEND_TEST_ALL_PREFIXES(ash::AppListControllerImplTest,
                            CheckAppListViewBoundsWhenVKeyboardEnabled);
@@ -471,9 +471,6 @@ class APP_LIST_EXPORT AppListView : public views::WidgetDelegateView,
   // Returns true if scroll events should be ignored.
   bool ShouldIgnoreScrollEvents();
 
-  // Returns true if the Embedded Assistant UI is currently being shown.
-  bool IsShowingEmbeddedAssistantUI() const;
-
   // Returns preferred y of fullscreen widget bounds in parent window for the
   // specified state.
   int GetPreferredWidgetYForState(ash::AppListViewState state) const;

ash/app_list/views/assistant/assistant_page_view_unittest.cc

@@ -258,6 +258,33 @@ TEST_F(AssistantPageViewTest,
   EXPECT_HAS_FOCUS(input_text_field());
 }
 
+TEST_F(AssistantPageViewTest, RememberAndShowHistory) {
+  ShowAssistantUiInTextMode();
+  EXPECT_HAS_FOCUS(input_text_field());
+
+  MockAssistantInteractionWithQueryAndResponse("query 1", "response 1");
+  MockAssistantInteractionWithQueryAndResponse("query 2", "response 2");
+
+  EXPECT_HAS_FOCUS(input_text_field());
+
+  EXPECT_TRUE(input_text_field()->GetText().empty());
+
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_UP, /*flags=*/0);
+  EXPECT_EQ(input_text_field()->GetText(), base::UTF8ToUTF16("query 2"));
+
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_UP, /*flags=*/0);
+  EXPECT_EQ(input_text_field()->GetText(), base::UTF8ToUTF16("query 1"));
+
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_UP, /*flags=*/0);
+  EXPECT_EQ(input_text_field()->GetText(), base::UTF8ToUTF16("query 1"));
+
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_DOWN, /*flags=*/0);
+  EXPECT_EQ(input_text_field()->GetText(), base::UTF8ToUTF16("query 2"));
+
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_DOWN, /*flags=*/0);
+  EXPECT_TRUE(input_text_field()->GetText().empty());
+}
+
 // Tests the |AssistantPageView| with tablet mode enabled.
 class AssistantPageViewTabletModeTest : public AssistantAshTestBase {
  public:

ash/assistant/test/assistant_ash_test_base.cc

@@ -4,6 +4,9 @@
 
 #include "ash/assistant/test/assistant_ash_test_base.h"
 
+#include <string>
+#include <utility>
+
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/app_list/views/assistant/assistant_main_view.h"
 #include "ash/app_list/views/assistant/assistant_page_view.h"
@@ -133,14 +136,18 @@ views::View* AssistantAshTestBase::page_view() {
 
 void AssistantAshTestBase::MockAssistantInteractionWithResponse(
     const std::string& response_text) {
-  const std::string query = std::string("input text");
+  MockAssistantInteractionWithQueryAndResponse(/*query=*/"input text",
+                                               response_text);
+}
 
+void AssistantAshTestBase::MockAssistantInteractionWithQueryAndResponse(
+    const std::string& query,
+    const std::string& response_text) {
   SendQueryThroughTextField(query);
-  assistant_service()->SetInteractionResponse(
-      InteractionResponse()
-          .AddTextResponse(response_text)
-          .AddResolution(InteractionResponse::Resolution::kNormal)
-          .Clone());
+  auto response = std::make_unique<InteractionResponse>();
+  response->AddTextResponse(response_text)
+      ->AddResolution(InteractionResponse::Resolution::kNormal);
+  assistant_service()->SetInteractionResponse(std::move(response));
 
   base::RunLoop().RunUntilIdle();
 }

ash/assistant/test/assistant_ash_test_base.h

@@ -6,6 +6,7 @@
 #define ASH_ASSISTANT_TEST_ASSISTANT_ASH_TEST_BASE_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "ash/assistant/model/assistant_ui_model.h"
@@ -64,6 +65,10 @@ class AssistantAshTestBase : public AshTestBase {
   // and receiving |response_text| as a response to display.
   void MockAssistantInteractionWithResponse(const std::string& response_text);
 
+  void MockAssistantInteractionWithQueryAndResponse(
+      const std::string& query,
+      const std::string& response_text);
+
   // Simulate the user entering a query followed by <return>.
   void SendQueryThroughTextField(const std::string& query);
 

ash/assistant/test/test_assistant_service.cc

@@ -4,7 +4,9 @@
 
 #include "ash/assistant/test/test_assistant_service.h"
 
+#include <memory>
 #include <utility>
+#include <vector>
 
 #include "ash/assistant/assistant_interaction_controller.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
@@ -170,21 +172,15 @@ class InteractionResponse::Response {
   Response() = default;
   virtual ~Response() = default;
 
-  virtual std::unique_ptr<Response> Clone() const = 0;
-
   virtual void SendTo(
       chromeos::assistant::mojom::AssistantInteractionSubscriber* receiver) = 0;
 };
 
 class TextResponse : public InteractionResponse::Response {
  public:
-  TextResponse(const std::string& text) : text_(text) {}
+  explicit TextResponse(const std::string& text) : text_(text) {}
   ~TextResponse() override = default;
 
-  std::unique_ptr<Response> Clone() const override {
-    return std::make_unique<TextResponse>(text_);
-  }
-
   void SendTo(chromeos::assistant::mojom::AssistantInteractionSubscriber*
                   receiver) override {
     receiver->OnTextResponse(text_);
@@ -200,13 +196,10 @@ class ResolutionResponse : public InteractionResponse::Response {
  public:
   using Resolution = InteractionResponse::Resolution;
 
-  ResolutionResponse(Resolution resolution) : resolution_(resolution) {}
+  explicit ResolutionResponse(Resolution resolution)
+      : resolution_(resolution) {}
   ~ResolutionResponse() override = default;
 
-  std::unique_ptr<Response> Clone() const override {
-    return std::make_unique<ResolutionResponse>(resolution_);
-  }
-
   void SendTo(chromeos::assistant::mojom::AssistantInteractionSubscriber*
                   receiver) override {
     receiver->OnInteractionFinished(resolution_);
@@ -236,7 +229,7 @@ TestAssistantService::CreateRemoteAndBind() {
 }
 
 void TestAssistantService::SetInteractionResponse(
-    InteractionResponse&& response) {
+    std::unique_ptr<InteractionResponse> response) {
   interaction_response_ = std::move(response);
 }
 
@@ -263,12 +256,14 @@ void TestAssistantService ::StartTextInteraction(
     chromeos::assistant::mojom::AssistantQuerySource source,
     bool allow_tts) {
   StartInteraction(AssistantInteractionType::kText, source, query);
-  SendInteractionResponse();
+  if (interaction_response_)
+    SendInteractionResponse();
 }
 
 void TestAssistantService ::StartVoiceInteraction() {
   StartInteraction(AssistantInteractionType::kVoice);
-  SendInteractionResponse();
+  if (interaction_response_)
+    SendInteractionResponse();
 }
 
 void TestAssistantService ::StartWarmerWelcomeInteraction(
@@ -278,6 +273,10 @@ void TestAssistantService ::StartWarmerWelcomeInteraction(
 }
 
 void TestAssistantService ::StopActiveInteraction(bool cancel_conversation) {
+  if (!running_active_interaction_)
+    return;
+
+  running_active_interaction_ = false;
   for (auto& subscriber : interaction_subscribers_) {
     subscriber->OnInteractionFinished(
         AssistantInteractionResolution::kInterruption);
@@ -328,44 +327,36 @@ void TestAssistantService::StartInteraction(
     chromeos::assistant::mojom::AssistantInteractionType type,
     chromeos::assistant::mojom::AssistantQuerySource source,
     const std::string& query) {
+  DCHECK(!running_active_interaction_);
   for (auto& subscriber : interaction_subscribers_) {
     subscriber->OnInteractionStarted(
         AssistantInteractionMetadata::New(type, source, query));
   }
+  running_active_interaction_ = true;
 }
 
 void TestAssistantService::SendInteractionResponse() {
-  InteractionResponse response = PopInteractionResponse();
+  DCHECK(interaction_response_);
+  DCHECK(running_active_interaction_);
   for (auto& subscriber : interaction_subscribers_)
-    response.SendTo(subscriber.get());
-}
-
-InteractionResponse TestAssistantService::PopInteractionResponse() {
-  return std::move(interaction_response_);
+    interaction_response_->SendTo(subscriber.get());
+  DCHECK(!current_interaction());
+  interaction_response_.reset();
+  running_active_interaction_ = false;
 }
 
 InteractionResponse::InteractionResponse() = default;
-InteractionResponse::InteractionResponse(InteractionResponse&& other) = default;
-InteractionResponse& InteractionResponse::operator=(
-    InteractionResponse&& other) = default;
 InteractionResponse::~InteractionResponse() = default;
 
-InteractionResponse InteractionResponse::Clone() const {
-  InteractionResponse result{};
-  for (const auto& response : responses_)
-    result.AddResponse(response->Clone());
-  return result;
-}
-
-InteractionResponse& InteractionResponse::AddTextResponse(
+InteractionResponse* InteractionResponse::AddTextResponse(
     const std::string& text) {
   AddResponse(std::make_unique<TextResponse>(text));
-  return *this;
+  return this;
 }
 
-InteractionResponse& InteractionResponse::AddResolution(Resolution resolution) {
+InteractionResponse* InteractionResponse::AddResolution(Resolution resolution) {
   AddResponse(std::make_unique<ResolutionResponse>(resolution));
-  return *this;
+  return this;
 }
 
 void InteractionResponse::AddResponse(std::unique_ptr<Response> response) {

ash/assistant/test/test_assistant_service.h

@@ -5,7 +5,9 @@
 #ifndef ASH_ASSISTANT_TEST_TEST_ASSISTANT_SERVICE_H_
 #define ASH_ASSISTANT_TEST_TEST_ASSISTANT_SERVICE_H_
 
+#include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/timer/timer.h"
@@ -24,28 +26,23 @@ class SanityCheckSubscriber;
 // chain calls to the provided |AddXYZ| methods.
 //
 // Example usage:
-//     assistant_service()->SetInteractionResponse(
-//         InteractionResponse()
-//             .AddTextResponse("The response text")
-//             .AddResolution(InteractionResponse::Resolution::kNormal)
-//             .Clone());
+//     auto response = std::make_unique<InteractionResponse>();
+//     response->AddTextResponse("The response text")
+//             ->AddResolution(InteractionResponse::Resolution::kNormal);
+//     assistant_service()->SetInteractionResponse(std::move(response));
 class InteractionResponse {
  public:
   using Resolution = chromeos::assistant::mojom::AssistantInteractionResolution;
   class Response;
 
   InteractionResponse();
-  InteractionResponse(InteractionResponse&& other);
-  InteractionResponse& operator=(InteractionResponse&& other);
   ~InteractionResponse();
 
-  InteractionResponse Clone() const;
-
   // A simple textual response.
-  InteractionResponse& AddTextResponse(const std::string& text);
+  InteractionResponse* AddTextResponse(const std::string& text);
   // If used this will cause us to finish the interaction by passing the given
   // |resolution| to |AssistantInteractionSubscriber::OnInteractionFinished|.
-  InteractionResponse& AddResolution(Resolution resolution);
+  InteractionResponse* AddResolution(Resolution resolution);
 
   void SendTo(
       chromeos::assistant::mojom::AssistantInteractionSubscriber* receiver);
@@ -78,7 +75,7 @@ class TestAssistantService : public chromeos::assistant::mojom::Assistant {
   CreateRemoteAndBind();
 
   // Set the response that will be invoked when the next interaction starts.
-  void SetInteractionResponse(InteractionResponse&& response);
+  void SetInteractionResponse(std::unique_ptr<InteractionResponse> response);
 
   // Returns the current interaction.
   base::Optional<chromeos::assistant::mojom::AssistantInteractionMetadata>
@@ -120,14 +117,15 @@ class TestAssistantService : public chromeos::assistant::mojom::Assistant {
           chromeos::assistant::mojom::AssistantQuerySource::kUnspecified,
       const std::string& query = std::string());
   void SendInteractionResponse();
-  InteractionResponse PopInteractionResponse();
 
   mojo::Receiver<chromeos::assistant::mojom::Assistant> receiver_{this};
   mojo::RemoteSet<chromeos::assistant::mojom::AssistantInteractionSubscriber>
       interaction_subscribers_;
   std::unique_ptr<SanityCheckSubscriber> sanity_check_subscriber_;
   std::unique_ptr<CurrentInteractionSubscriber> current_interaction_subscriber_;
-  InteractionResponse interaction_response_;
+  std::unique_ptr<InteractionResponse> interaction_response_;
+
+  bool running_active_interaction_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestAssistantService);
 };