Improve/simplify stop interaction behavior.

Changes include:
- Stopping interaction on widget close (fixes ESC key bug).
- Stopping interaction on input modality change (if not voice).

Bug: b:79441527
Change-Id: I99e3de5ac8967e79a81bfbe1733da99df52ed03f
Reviewed-on: https://chromium-review.googlesource.com/1052430
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558779}

ash/BUILD.gn

@@ -1850,6 +1850,7 @@ test("ash_unittests") {
     "//chromeos:power_manager_proto",
     "//chromeos:test_support",
     "//chromeos:test_support_without_gmock",
+    "//chromeos/services/assistant:test_support",
     "//chromeos/services/multidevice_setup/public/mojom",
     "//components/account_id",
     "//components/password_manager/core/browser:hash_password_manager",

ash/DEPS

@@ -79,6 +79,7 @@ include_rules = [
   # TODO(jamescook): Eliminate this. http://crbug.com/644355
   "+chromeos/network",
   "+chromeos/services/assistant/public" ,
+  "+chromeos/services/assistant/test_support",
   "+chromeos/services/multidevice_setup/public/mojom",
   # TODO(jamescook): Eliminate this. http://crbug.com/644361
   "+chromeos/settings/timezone_settings.h",

ash/assistant/assistant_controller.cc

@@ -93,19 +93,11 @@ void AssistantController::RemoveInteractionModelObserver(
 }
 
 void AssistantController::StartInteraction() {
-  // TODO(dmblack): Instruct underlying service to start listening if current
-  // input modality is VOICE.
-  if (assistant_interaction_model_.interaction_state() ==
-      InteractionState::kInactive) {
-    OnInteractionStarted();
-  }
+  OnInteractionStarted();
 }
 
 void AssistantController::StopInteraction() {
-  if (assistant_interaction_model_.interaction_state() !=
-      InteractionState::kInactive) {
-    OnInteractionDismissed();
-  }
+  assistant_interaction_model_.SetInteractionState(InteractionState::kInactive);
 }
 
 void AssistantController::ToggleInteraction() {
@@ -119,13 +111,16 @@ void AssistantController::ToggleInteraction() {
 
 void AssistantController::OnInteractionStateChanged(
     InteractionState interaction_state) {
-  if (interaction_state == InteractionState::kInactive) {
-    assistant_interaction_model_.ClearInteraction();
+  if (interaction_state == InteractionState::kActive)
+    return;
 
-    // TODO(dmblack): Input modality should default back to the user's
-    // preferred input modality.
-    assistant_interaction_model_.SetInputModality(InputModality::kVoice);
-  }
+  // When the user-facing interaction is dismissed, we instruct the service to
+  // terminate any listening, speaking, or query in flight.
+  DCHECK(assistant_);
+  assistant_->StopActiveInteraction();
+
+  assistant_interaction_model_.ClearInteraction();
+  assistant_interaction_model_.SetInputModality(InputModality::kVoice);
 }
 
 void AssistantController::OnHighlighterEnabledChanged(
@@ -139,21 +134,26 @@ void AssistantController::OnHighlighterEnabledChanged(
   }
 }
 
+void AssistantController::OnInputModalityChanged(InputModality input_modality) {
+  if (input_modality == InputModality::kVoice)
+    return;
+
+  // When switching to a non-voice input modality we instruct the underlying
+  // service to terminate any listening, speaking, or in flight query. We do not
+  // do this when switching to voice input modality because initiation of a
+  // voice interaction will automatically interrupt any pre-existing activity.
+  // Stopping the active interaction here for voice input modality would
+  // actually have the undesired effect of stopping the voice interaction.
+  DCHECK(assistant_);
+  assistant_->StopActiveInteraction();
+}
+
 void AssistantController::OnInteractionStarted() {
   assistant_interaction_model_.SetInteractionState(InteractionState::kActive);
 }
 
 void AssistantController::OnInteractionFinished(
-    AssistantInteractionResolution resolution) {}
-
-void AssistantController::OnInteractionDismissed() {
-  assistant_interaction_model_.SetInteractionState(InteractionState::kInactive);
-
-  // When the user-facing interaction is dismissed, we instruct the service to
-  // terminate any listening, speaking, or query in flight.
-  DCHECK(assistant_);
-  assistant_->StopActiveInteraction();
-}
+    chromeos::assistant::mojom::AssistantInteractionResolution resolution) {}
 
 void AssistantController::OnDialogPlateContentsChanged(
     const std::string& text) {
@@ -162,16 +162,6 @@ void AssistantController::OnDialogPlateContentsChanged(
     // voice so that we will show the mic icon in the UI.
     assistant_interaction_model_.SetInputModality(InputModality::kVoice);
   } else {
-    // If switching to keyboard modality from voice, we instruct the service to
-    // terminate any listening, speaking, or query in flight.
-    // TODO(dmblack): We probably want this same logic when switching to any
-    // modality from voice. Handle this instead in OnInputModalityChanged, but
-    // we will need to add a previous modality parameter to that API.
-    if (assistant_interaction_model_.input_modality() ==
-        InputModality::kVoice) {
-      DCHECK(assistant_);
-      assistant_->StopActiveInteraction();
-    }
     assistant_interaction_model_.SetInputModality(InputModality::kKeyboard);
     assistant_interaction_model_.SetMicState(MicState::kClosed);
   }

ash/assistant/assistant_controller.h

@@ -86,6 +86,7 @@ class AssistantController
   void OnSuggestionChipPressed(int id);
 
   // AssistantInteractionModelObserver:
+  void OnInputModalityChanged(InputModality input_modality) override;
   void OnInteractionStateChanged(InteractionState interaction_state) override;
 
   // HighlighterController::Observer:
@@ -115,8 +116,6 @@ class AssistantController
       mojom::AssistantCardRendererPtr assistant_card_renderer) override;
 
  private:
-  void OnInteractionDismissed();
-
   mojo::Binding<mojom::AssistantController> assistant_controller_binding_;
   mojo::Binding<chromeos::assistant::mojom::AssistantEventSubscriber>
       assistant_event_subscriber_binding_;

ash/assistant/assistant_controller_unittest.cc

@@ -4,12 +4,16 @@
 
 #include "ash/assistant/assistant_controller.h"
 
+#include <memory>
+
 #include "ash/highlighter/highlighter_controller.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/services/assistant/test_support/mock_assistant.h"
+#include "mojo/public/cpp/bindings/binding.h"
 
 namespace ash {
 
@@ -25,6 +29,15 @@ class AssistantControllerTest : public AshTestBase {
     AshTestBase::SetUp();
     controller_ = Shell::Get()->assistant_controller();
     DCHECK(controller_);
+
+    // Mock Assistant.
+    assistant_ = std::make_unique<chromeos::assistant::MockAssistant>();
+    assistant_binding_ =
+        std::make_unique<mojo::Binding<chromeos::assistant::mojom::Assistant>>(
+            assistant_.get());
+    chromeos::assistant::mojom::AssistantPtr assistant;
+    assistant_binding_->Bind(mojo::MakeRequest(&assistant));
+    controller_->SetAssistant(std::move(assistant));
   }
 
   const AssistantInteractionModel* interaction_model() {
@@ -35,6 +48,10 @@ class AssistantControllerTest : public AshTestBase {
   base::test::ScopedFeatureList scoped_feature_list_;
   AssistantController* controller_ = nullptr;
 
+  std::unique_ptr<chromeos::assistant::MockAssistant> assistant_;
+  std::unique_ptr<mojo::Binding<chromeos::assistant::mojom::Assistant>>
+      assistant_binding_;
+
   DISALLOW_COPY_AND_ASSIGN(AssistantControllerTest);
 };
 

ash/assistant/model/assistant_interaction_model.cc

@@ -11,10 +11,7 @@
 namespace ash {
 
 AssistantInteractionModel::AssistantInteractionModel()
-    : query_(std::make_unique<AssistantEmptyQuery>()) {
-  // TODO(dmblack): Default input modality should be read from user preferences.
-  input_modality_ = InputModality::kVoice;
-}
+    : query_(std::make_unique<AssistantEmptyQuery>()) {}
 
 AssistantInteractionModel::~AssistantInteractionModel() = default;
 

ash/assistant/model/assistant_interaction_model.h

@@ -117,7 +117,7 @@ class AssistantInteractionModel {
   void NotifySuggestionsCleared();
 
   InteractionState interaction_state_ = InteractionState::kInactive;
-  InputModality input_modality_;
+  InputModality input_modality_ = InputModality::kVoice;
   MicState mic_state_ = MicState::kClosed;
   std::unique_ptr<AssistantQuery> query_;
   std::vector<AssistantSuggestionPtr> suggestions_;

ash/assistant/ui/assistant_bubble.cc

@@ -127,6 +127,12 @@ void AssistantBubble::OnWidgetActivationChanged(views::Widget* widget,
 }
 
 void AssistantBubble::OnWidgetDestroying(views::Widget* widget) {
+  // We need to be sure that the Assistant interaction is stopped when the
+  // widget is closed. Special cases, such as closing the widget via the |ESC|
+  // key might otherwise go unhandled, causing inconsistencies between the
+  // widget visibility state and the underlying interaction model state.
+  assistant_controller_->StopInteraction();
+
   container_view_->GetWidget()->RemoveObserver(this);
   container_view_ = nullptr;
 }

chromeos/services/assistant/BUILD.gn

@@ -120,3 +120,16 @@ catalog("tests_catalog") {
   testonly = true
   embedded_services = [ ":unittest_manifest" ]
 }
+
+static_library("test_support") {
+  testonly = true
+  sources = [
+    "test_support/mock_assistant.cc",
+    "test_support/mock_assistant.h",
+  ]
+  deps = [
+    "//base",
+    "//chromeos/services/assistant/public/mojom",
+    "//testing/gmock",
+  ]
+}

chromeos/services/assistant/test_support/mock_assistant.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 "chromeos/services/assistant/test_support/mock_assistant.h"
+
+namespace chromeos {
+namespace assistant {
+
+MockAssistant::MockAssistant() = default;
+
+MockAssistant::~MockAssistant() = default;
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/test_support/mock_assistant.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 CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_MOCK_ASSISTANT_H_
+#define CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_MOCK_ASSISTANT_H_
+
+#include "base/macros.h"
+#include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace chromeos {
+namespace assistant {
+
+class MockAssistant : public mojom::Assistant {
+ public:
+  MockAssistant();
+  ~MockAssistant() override;
+
+  MOCK_METHOD0(StartVoiceInteraction, void());
+
+  MOCK_METHOD0(StopActiveInteraction, void());
+
+  MOCK_METHOD1(SendTextQuery, void(const std::string&));
+
+  MOCK_METHOD1(AddAssistantEventSubscriber,
+               void(chromeos::assistant::mojom::AssistantEventSubscriberPtr));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockAssistant);
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_TEST_SUPPORT_MOCK_ASSISTANT_H_