croissant: introduce HighlighterController::UpdateEnabledState API

Changes:
This CL mainly addressed two issues:
- On stylus selection complete, do not dismiss assistant bubble.
- provide AssistantController api to exit metalayer mode.

Introduce HighlighterEnabledState, which contains
- kEnabled
- kDisabledByUser
- kDisabledBySessionEnd

When AssistantController calls this api, it could specify
kDisabledByMetalayerSessionEnd state to indicate metalayer session
aborted so that (1) it doesn't dismiss assistant bubble, (2) metalayer
mode is exited. (1)(2) are done by observers.

Bug: b/78193960
Test: manual test and test coverage
Change-Id: Iba81d6827aa7f45bb2195a865e6684148319ac7b
Reviewed-on: https://chromium-review.googlesource.com/1053213
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558525}

ash/assistant/assistant_controller.cc

@@ -128,12 +128,15 @@ void AssistantController::OnInteractionStateChanged(
   }
 }
 
-void AssistantController::OnHighlighterEnabledChanged(bool enabled) {
-  // TODO(warx): add a reason enum to distinguish the case of deselecting the
-  // tool and done with a stylus selection.
+void AssistantController::OnHighlighterEnabledChanged(
+    HighlighterEnabledState state) {
   assistant_interaction_model_.SetInputModality(InputModality::kStylus);
-  assistant_interaction_model_.SetInteractionState(
-      enabled ? InteractionState::kActive : InteractionState::kInactive);
+  if (state == HighlighterEnabledState::kEnabled) {
+    assistant_interaction_model_.SetInteractionState(InteractionState::kActive);
+  } else if (state == HighlighterEnabledState::kDisabledByUser) {
+    assistant_interaction_model_.SetInteractionState(
+        InteractionState::kInactive);
+  }
 }
 
 void AssistantController::OnInteractionStarted() {

ash/assistant/assistant_controller.h

@@ -89,7 +89,7 @@ class AssistantController
   void OnInteractionStateChanged(InteractionState interaction_state) override;
 
   // HighlighterController::Observer:
-  void OnHighlighterEnabledChanged(bool enabled) override;
+  void OnHighlighterEnabledChanged(HighlighterEnabledState state) override;
 
   // chromeos::assistant::mojom::AssistantEventSubscriber:
   void OnInteractionStarted() override;

ash/assistant/assistant_controller_unittest.cc

@@ -41,12 +41,20 @@ class AssistantControllerTest : public AshTestBase {
 TEST_F(AssistantControllerTest, HighlighterEnabledStatus) {
   HighlighterController* highlighter_controller =
       Shell::Get()->highlighter_controller();
-  highlighter_controller->SetEnabled(true);
+  highlighter_controller->UpdateEnabledState(HighlighterEnabledState::kEnabled);
   EXPECT_EQ(InputModality::kStylus, interaction_model()->input_modality());
   EXPECT_EQ(InteractionState::kActive,
             interaction_model()->interaction_state());
 
-  highlighter_controller->SetEnabled(false);
+  // Metalayer mode session end should keep interaction state active.
+  highlighter_controller->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledBySessionEnd);
+  EXPECT_EQ(InteractionState::kActive,
+            interaction_model()->interaction_state());
+
+  // Disabling directly by user action should make interaction state inactive.
+  highlighter_controller->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledByUser);
   EXPECT_EQ(InteractionState::kInactive,
             interaction_model()->interaction_state());
 }

ash/highlighter/highlighter_controller.cc

@@ -80,6 +80,30 @@ void HighlighterController::SetExitCallback(base::OnceClosure exit_callback,
   require_success_ = require_success;
 }
 
+void HighlighterController::UpdateEnabledState(
+    HighlighterEnabledState enabled_state) {
+  if (enabled_state_ == enabled_state)
+    return;
+  enabled_state_ = enabled_state;
+
+  SetEnabled(enabled_state == HighlighterEnabledState::kEnabled);
+  for (auto& observer : observers_)
+    observer.OnHighlighterEnabledChanged(enabled_state);
+}
+
+void HighlighterController::BindRequest(
+    mojom::HighlighterControllerRequest request) {
+  binding_.Bind(std::move(request));
+}
+
+void HighlighterController::SetClient(
+    mojom::HighlighterControllerClientPtr client) {
+  client_ = std::move(client);
+  client_.set_connection_error_handler(
+      base::BindOnce(&HighlighterController::OnClientConnectionLost,
+                     weak_factory_.GetWeakPtr()));
+}
+
 void HighlighterController::SetEnabled(bool enabled) {
   FastInkPointerController::SetEnabled(enabled);
   if (enabled) {
@@ -99,26 +123,11 @@ void HighlighterController::SetEnabled(bool enabled) {
     if (highlighter_view_ && !highlighter_view_->animating())
       DestroyPointerView();
   }
-  for (auto& observer : observers_)
-    observer.OnHighlighterEnabledChanged(enabled);
 
   if (client_)
     client_->HandleEnabledStateChange(enabled);
 }
 
-void HighlighterController::BindRequest(
-    mojom::HighlighterControllerRequest request) {
-  binding_.Bind(std::move(request));
-}
-
-void HighlighterController::SetClient(
-    mojom::HighlighterControllerClientPtr client) {
-  client_ = std::move(client);
-  client_.set_connection_error_handler(
-      base::Bind(&HighlighterController::OnClientConnectionLost,
-                 weak_factory_.GetWeakPtr()));
-}
-
 void HighlighterController::ExitHighlighterMode() {
   CallExitCallback();
 }

ash/highlighter/highlighter_controller.h

@@ -24,6 +24,17 @@ namespace ash {
 class HighlighterResultView;
 class HighlighterView;
 
+// Highlighter enabled state that is notified to observers.
+enum class HighlighterEnabledState {
+  // Highlighter is enabled by any ways.
+  kEnabled,
+  // 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,
+};
+
 // Controller for the highlighter functionality.
 // Enables/disables highlighter as well as receives points
 // and passes them off to be rendered.
@@ -34,10 +45,8 @@ class ASH_EXPORT HighlighterController
   // Interface for classes that wish to be notified with highlighter status.
   class Observer {
    public:
-    // Called when highlighter enabled status changes.
-    // TODO(warx): add a reason enum to distinguish the case of deselecting the
-    // tool and done with a stylus selection.
-    virtual void OnHighlighterEnabledChanged(bool enabled) {}
+    // Called when highlighter enabled state changes.
+    virtual void OnHighlighterEnabledChanged(HighlighterEnabledState state) {}
 
    protected:
     virtual ~Observer() = default;
@@ -46,6 +55,8 @@ class ASH_EXPORT HighlighterController
   HighlighterController();
   ~HighlighterController() override;
 
+  HighlighterEnabledState enabled_state() { return enabled_state_; }
+
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
@@ -55,8 +66,9 @@ class ASH_EXPORT HighlighterController
   // after the first complete gesture, regardless of the recognition result.
   void SetExitCallback(base::OnceClosure callback, bool require_success);
 
-  // fast_ink::FastInkPointerController:
-  void SetEnabled(bool enabled) override;
+  // Update highlighter enabled |state| and notify observers.
+  // TODO(warx): add UpdateEnabledState test cases.
+  void UpdateEnabledState(HighlighterEnabledState enabled_state);
 
   void BindRequest(mojom::HighlighterControllerRequest request);
 
@@ -68,6 +80,7 @@ class ASH_EXPORT HighlighterController
   friend class HighlighterControllerTestApi;
 
   // fast_ink::FastInkPointerController:
+  void SetEnabled(bool enabled) override;
   views::View* GetPointerView() const override;
   void CreatePointerView(base::TimeDelta presentation_delay,
                          aura::Window* root_window) override;
@@ -93,6 +106,10 @@ class ASH_EXPORT HighlighterController
 
   void FlushMojoForTesting();
 
+  // Caches the highlighter enabled state.
+  HighlighterEnabledState enabled_state_ =
+      HighlighterEnabledState::kDisabledByUser;
+
   // |highlighter_view_| will only hold an instance when the highlighter is
   // enabled and activated (pressed or dragged) and until the fade out
   // animation is done.

ash/system/palette/tools/metalayer_mode.cc

@@ -4,7 +4,6 @@
 
 #include "ash/system/palette/tools/metalayer_mode.h"
 
-#include "ash/highlighter/highlighter_controller.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -39,9 +38,11 @@ MetalayerMode::MetalayerMode(Delegate* delegate)
     : CommonPaletteTool(delegate), weak_factory_(this) {
   Shell::Get()->AddPreTargetHandler(this);
   Shell::Get()->voice_interaction_controller()->AddObserver(this);
+  Shell::Get()->highlighter_controller()->AddObserver(this);
 }
 
 MetalayerMode::~MetalayerMode() {
+  Shell::Get()->highlighter_controller()->RemoveObserver(this);
   Shell::Get()->voice_interaction_controller()->RemoveObserver(this);
   Shell::Get()->RemovePreTargetHandler(this);
 }
@@ -57,19 +58,26 @@ PaletteToolId MetalayerMode::GetToolId() const {
 void MetalayerMode::OnEnable() {
   CommonPaletteTool::OnEnable();
 
-  Shell::Get()->highlighter_controller()->SetExitCallback(
+  HighlighterController* controller = Shell::Get()->highlighter_controller();
+  controller->SetExitCallback(
       base::BindOnce(&MetalayerMode::OnMetalayerSessionComplete,
                      weak_factory_.GetWeakPtr()),
       !activated_via_button_ /* no retries if activated via button */);
-  Shell::Get()->highlighter_controller()->SetEnabled(true);
+  controller->UpdateEnabledState(HighlighterEnabledState::kEnabled);
   delegate()->HidePalette();
 }
 
 void MetalayerMode::OnDisable() {
   CommonPaletteTool::OnDisable();
-
-  Shell::Get()->highlighter_controller()->SetEnabled(false);
   activated_via_button_ = false;
+
+  HighlighterController* controller = Shell::Get()->highlighter_controller();
+  if (controller->enabled_state() != HighlighterEnabledState::kEnabled)
+    return;
+  // Call UpdateEnabledState() only when it hasn't been disabled to ensure this
+  // emits the disabling signal by deselecting on palette tool.
+  Shell::Get()->highlighter_controller()->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledByUser);
 }
 
 const gfx::VectorIcon& MetalayerMode::GetActiveTrayIcon() const {
@@ -163,6 +171,19 @@ void MetalayerMode::OnAssistantFeatureAllowedChanged(
   UpdateState();
 }
 
+void MetalayerMode::OnHighlighterEnabledChanged(HighlighterEnabledState state) {
+  // OnHighlighterEnabledChanged is used by the caller that disables highlighter
+  // enabled state to disable the palette tool only.
+  if (state == HighlighterEnabledState::kEnabled)
+    return;
+
+  // TODO(warx): We disable palette tool of metalayer mode when receiving the
+  // disabled state of highlighter controller. And this class also calls
+  // HighlighterController's UpdateEnabledState method to send the state signal.
+  // We shall think of removing this circular dependency.
+  delegate()->DisableTool(GetToolId());
+}
+
 void MetalayerMode::UpdateState() {
   if (enabled() && !selectable())
     delegate()->DisableTool(GetToolId());
@@ -197,7 +218,8 @@ void MetalayerMode::UpdateView() {
 }
 
 void MetalayerMode::OnMetalayerSessionComplete() {
-  delegate()->DisableTool(GetToolId());
+  Shell::Get()->highlighter_controller()->UpdateEnabledState(
+      HighlighterEnabledState::kDisabledBySessionEnd);
 }
 
 }  // namespace ash

ash/system/palette/tools/metalayer_mode.h

@@ -6,6 +6,7 @@
 #define ASH_SYSTEM_PALETTE_TOOLS_METALAYER_MODE_H_
 
 #include "ash/ash_export.h"
+#include "ash/highlighter/highlighter_controller.h"
 #include "ash/public/interfaces/voice_interaction_controller.mojom.h"
 #include "ash/system/palette/common_palette_tool.h"
 #include "ash/voice_interaction/voice_interaction_observer.h"
@@ -21,7 +22,8 @@ namespace ash {
 // menu, but also by the stylus button click.
 class ASH_EXPORT MetalayerMode : public CommonPaletteTool,
                                  public ui::EventHandler,
-                                 public VoiceInteractionObserver {
+                                 public VoiceInteractionObserver,
+                                 public HighlighterController::Observer {
  public:
   explicit MetalayerMode(Delegate* delegate);
   ~MetalayerMode() override;
@@ -70,6 +72,9 @@ class ASH_EXPORT MetalayerMode : public CommonPaletteTool,
   void OnAssistantFeatureAllowedChanged(
       mojom::AssistantAllowedState state) override;
 
+  // HighlighterController::Observer:
+  void OnHighlighterEnabledChanged(HighlighterEnabledState state) override;
+
   // Update the state of the tool based on the current availability of the tool.
   void UpdateState();