Metalayer mode should not be sticky.

We need to disable it:
- when Assistant UI is dismissed.
- when an Assistant interaction is started (e.g. via hotword).

Bug: b:110961300
Change-Id: I97024e9f2681d171c40f54ac5b47c5ed549085b6
Reviewed-on: https://chromium-review.googlesource.com/1119530
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573520}

ash/assistant/assistant_interaction_controller.cc

@@ -92,12 +92,27 @@ void AssistantInteractionController::OnHighlighterEnabledChanged(
       break;
     case HighlighterEnabledState::kDisabledByUser:
       FALLTHROUGH;
-    case HighlighterEnabledState::kDisabledBySessionEnd:
+    case HighlighterEnabledState::kDisabledBySessionComplete:
       assistant_interaction_model_.SetInputModality(InputModality::kKeyboard);
       break;
+    case HighlighterEnabledState::kDisabledBySessionAbort:
+      // When metalayer mode has been aborted, no action necessary. Abort occurs
+      // as a result of an interaction starting, most likely due to hotword
+      // detection. Setting the input modality in these cases would have the
+      // unintended consequence of stopping the active interaction.
+      break;
   }
 }
 
+void AssistantInteractionController::OnInteractionStateChanged(
+    InteractionState interaction_state) {
+  if (interaction_state != InteractionState::kActive)
+    return;
+
+  // Metalayer mode should not be sticky. Disable it on interaction start.
+  Shell::Get()->highlighter_controller()->AbortSession();
+}
+
 void AssistantInteractionController::OnInputModalityChanged(
     InputModality input_modality) {
   if (input_modality == InputModality::kVoice)

ash/assistant/assistant_interaction_controller.h

@@ -61,6 +61,7 @@ class AssistantInteractionController
   void OnSuggestionChipPressed(int id);
 
   // AssistantInteractionModelObserver:
+  void OnInteractionStateChanged(InteractionState interaction_state) override;
   void OnInputModalityChanged(InputModality input_modality) override;
   void OnCommittedQueryChanged(const AssistantQuery& committed_query) override;
 

ash/assistant/assistant_ui_controller.cc

@@ -41,6 +41,7 @@ void ShowToast(const std::string& id, int message_id) {
 AssistantUiController::AssistantUiController(
     AssistantController* assistant_controller)
     : assistant_controller_(assistant_controller) {
+  AddModelObserver(this);
   assistant_controller_->AddObserver(this);
   Shell::Get()->highlighter_controller()->AddObserver(this);
 }
@@ -48,6 +49,7 @@ AssistantUiController::AssistantUiController(
 AssistantUiController::~AssistantUiController() {
   Shell::Get()->highlighter_controller()->RemoveObserver(this);
   assistant_controller_->RemoveObserver(this);
+  RemoveModelObserver(this);
 
   if (container_view_)
     container_view_->GetWidget()->RemoveObserver(this);
@@ -158,7 +160,8 @@ void AssistantUiController::OnHighlighterEnabledChanged(
       if (assistant_ui_model_.visible())
         HideUi(AssistantSource::kStylus);
       break;
-    case HighlighterEnabledState::kDisabledBySessionEnd:
+    case HighlighterEnabledState::kDisabledBySessionComplete:
+    case HighlighterEnabledState::kDisabledBySessionAbort:
       // No action necessary.
       break;
   }
@@ -177,6 +180,15 @@ void AssistantUiController::OnUrlOpened(const GURL& url) {
   HideUi(AssistantSource::kUnspecified);
 }
 
+void AssistantUiController::OnUiVisibilityChanged(bool visible,
+                                                  AssistantSource source) {
+  if (visible)
+    return;
+
+  // Metalayer mode should not be sticky. Disable it when hiding UI.
+  Shell::Get()->highlighter_controller()->AbortSession();
+}
+
 void AssistantUiController::ShowUi(AssistantSource source) {
   if (!Shell::Get()->voice_interaction_controller()->setup_completed()) {
     assistant_setup_->StartAssistantOptInFlow();

ash/assistant/assistant_ui_controller.h

@@ -9,6 +9,7 @@
 #include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/model/assistant_interaction_model_observer.h"
 #include "ash/assistant/model/assistant_ui_model.h"
+#include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/assistant/ui/caption_bar.h"
 #include "ash/assistant/ui/dialog_plate/dialog_plate.h"
 #include "ash/highlighter/highlighter_controller.h"
@@ -41,6 +42,7 @@ class ASH_EXPORT AssistantUiController
     : public views::WidgetObserver,
       public AssistantControllerObserver,
       public AssistantInteractionModelObserver,
+      public AssistantUiModelObserver,
       public CaptionBarDelegate,
       public DialogPlateObserver,
       public HighlighterController::Observer {
@@ -89,6 +91,9 @@ class ASH_EXPORT AssistantUiController
   void OnDeepLinkReceived(const GURL& deep_link) override;
   void OnUrlOpened(const GURL& url) override;
 
+  // AssistantUiModelObserver:
+  void OnUiVisibilityChanged(bool visible, AssistantSource source) override;
+
   void ShowUi(AssistantSource source);
   void HideUi(AssistantSource source);
   void ToggleUi(AssistantSource source);

ash/highlighter/highlighter_controller.cc

@@ -91,6 +91,11 @@ void HighlighterController::UpdateEnabledState(
     observer.OnHighlighterEnabledChanged(enabled_state);
 }
 
+void HighlighterController::AbortSession() {
+  if (enabled_state_ == HighlighterEnabledState::kEnabled)
+    UpdateEnabledState(HighlighterEnabledState::kDisabledBySessionAbort);
+}
+
 void HighlighterController::BindRequest(
     mojom::HighlighterControllerRequest request) {
   binding_.Bind(std::move(request));

ash/highlighter/highlighter_controller.h

@@ -31,8 +31,11 @@ enum class HighlighterEnabledState {
   // Highlighter is disabled by user directly, for example disabling palette
   // tool by user actions on palette menu.
   kDisabledByUser,
-  // Highlighter is disabled on metalayer session aborted or complete.
-  kDisabledBySessionEnd,
+  // Highlighter is disabled on metalayer session complete.
+  kDisabledBySessionComplete,
+  // Highlighter is disabled on metalayer session abort. An abort may occur due
+  // to dismissal of Assistant UI or due to interruption by user via hotword.
+  kDisabledBySessionAbort,
 };
 
 // Controller for the highlighter functionality.
@@ -72,6 +75,10 @@ class ASH_EXPORT HighlighterController
   // Update highlighter enabled |state| and notify observers.
   void UpdateEnabledState(HighlighterEnabledState enabled_state);
 
+  // Aborts the current metalayer session. If no metalayer session exists,
+  // calling this method is a no-op.
+  void AbortSession();
+
   void BindRequest(mojom::HighlighterControllerRequest request);
 
   // mojom::HighlighterController:

ash/highlighter/highlighter_controller_unittest.cc

@@ -29,13 +29,19 @@ class TestHighlighterObserver : public HighlighterController::Observer {
 
   // HighlighterController::Observer:
   void OnHighlighterEnabledChanged(HighlighterEnabledState state) override {
-    if (state == HighlighterEnabledState::kEnabled) {
-      ++enabled_count_;
-    } else if (state == HighlighterEnabledState::kDisabledByUser) {
-      ++disabled_by_user_count_;
-    } else {
-      DCHECK_EQ(HighlighterEnabledState::kDisabledBySessionEnd, state);
-      ++disabled_by_session_end_;
+    switch (state) {
+      case HighlighterEnabledState::kEnabled:
+        ++enabled_count_;
+        break;
+      case HighlighterEnabledState::kDisabledByUser:
+        ++disabled_by_user_count_;
+        break;
+      case HighlighterEnabledState::kDisabledBySessionAbort:
+        ++disabled_by_session_abort_;
+        break;
+      case HighlighterEnabledState::kDisabledBySessionComplete:
+        ++disabled_by_session_complete_;
+        break;
     }
   }
 
@@ -45,7 +51,8 @@ class TestHighlighterObserver : public HighlighterController::Observer {
 
   int enabled_count_ = 0;
   int disabled_by_user_count_ = 0;
-  int disabled_by_session_end_ = 0;
+  int disabled_by_session_abort_ = 0;
+  int disabled_by_session_complete_ = 0;
   gfx::Rect last_recognized_rect_;
 
  private:
@@ -529,30 +536,121 @@ TEST_F(HighlighterControllerTest, UpdateEnabledState) {
   TestHighlighterObserver observer;
   controller_->AddObserver(&observer);
 
+  // Assert initial state.
   ASSERT_EQ(0, observer.enabled_count_);
   ASSERT_EQ(0, observer.disabled_by_user_count_);
-  ASSERT_EQ(0, observer.disabled_by_session_end_);
+  ASSERT_EQ(0, observer.disabled_by_session_abort_);
+  ASSERT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Test enabling.
   tool_->OnEnable();
   EXPECT_EQ(1, observer.enabled_count_);
   EXPECT_EQ(0, observer.disabled_by_user_count_);
-  EXPECT_EQ(0, observer.disabled_by_session_end_);
+  EXPECT_EQ(0, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
 
+  // Test disabling by user.
   tool_->OnDisable();
   EXPECT_EQ(1, observer.enabled_count_);
   EXPECT_EQ(1, observer.disabled_by_user_count_);
-  EXPECT_EQ(0, observer.disabled_by_session_end_);
+  EXPECT_EQ(0, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
 
+  // Test disabling by session abort.
   tool_->OnEnable();
   EXPECT_EQ(2, observer.enabled_count_);
   EXPECT_EQ(1, observer.disabled_by_user_count_);
-  EXPECT_EQ(0, observer.disabled_by_session_end_);
+  EXPECT_EQ(0, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
   controller_->UpdateEnabledState(
-      HighlighterEnabledState::kDisabledBySessionEnd);
+      HighlighterEnabledState::kDisabledBySessionAbort);
   EXPECT_EQ(2, observer.enabled_count_);
   EXPECT_EQ(1, observer.disabled_by_user_count_);
-  EXPECT_EQ(1, observer.disabled_by_session_end_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Test disabling by session complete.
+  tool_->OnEnable();
+  EXPECT_EQ(3, observer.enabled_count_);
+  EXPECT_EQ(1, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+  controller_->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledBySessionComplete);
+  EXPECT_EQ(3, observer.enabled_count_);
+  EXPECT_EQ(1, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
 
   controller_->RemoveObserver(&observer);
 }
 
+// Test aborting a metalayer session and notifying observers properly.
+TEST_F(HighlighterControllerTest, AbortSession) {
+  TestHighlighterObserver observer;
+  controller_->AddObserver(&observer);
+
+  // Assert initial state.
+  ASSERT_EQ(0, observer.enabled_count_);
+  ASSERT_EQ(0, observer.disabled_by_user_count_);
+  ASSERT_EQ(0, observer.disabled_by_session_abort_);
+  ASSERT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Start metalayer session.
+  tool_->OnEnable();
+  EXPECT_EQ(1, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(0, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Abort metalayer session.
+  controller_->AbortSession();
+  EXPECT_EQ(1, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Assert no-op when aborting an aborted session.
+  controller_->AbortSession();
+  EXPECT_EQ(1, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+
+  // Assert no-op when aborting a completed session.
+  tool_->OnEnable();
+  EXPECT_EQ(2, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(0, observer.disabled_by_session_complete_);
+  controller_->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledBySessionComplete);
+  EXPECT_EQ(2, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
+  controller_->AbortSession();
+  EXPECT_EQ(2, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
+
+  // Assert no-op when aborting a disabled session.
+  tool_->OnEnable();
+  EXPECT_EQ(3, observer.enabled_count_);
+  EXPECT_EQ(0, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
+  tool_->OnDisable();
+  EXPECT_EQ(3, observer.enabled_count_);
+  EXPECT_EQ(1, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
+  controller_->AbortSession();
+  EXPECT_EQ(3, observer.enabled_count_);
+  EXPECT_EQ(1, observer.disabled_by_user_count_);
+  EXPECT_EQ(1, observer.disabled_by_session_abort_);
+  EXPECT_EQ(1, observer.disabled_by_session_complete_);
+}
+
 }  // namespace ash

ash/system/palette/tools/metalayer_mode.cc

@@ -224,7 +224,7 @@ void MetalayerMode::UpdateView() {
 
 void MetalayerMode::OnMetalayerSessionComplete() {
   Shell::Get()->highlighter_controller()->UpdateEnabledState(
-      HighlighterEnabledState::kDisabledBySessionEnd);
+      HighlighterEnabledState::kDisabledBySessionComplete);
 }
 
 }  // namespace ash