Add and support notification types for Assistant.

Previously all Assistant notifications were Message Center
notifications. Now, we'll support notifications of type:

- kInAssistant
- kPreferInAssistant
- kSystem

The |kInAssistant| and |kPreferInAssistant| types are currently only
used to support timer notification UX requirements.

Still TODO: Entry/exit animations for in-Assistant notifications.

Bug: b:118654460
Change-Id: I815789d89008ff55318d7b6e8c9492b07849f2ca
Reviewed-on: https://chromium-review.googlesource.com/c/1480549Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#634921}

ash/assistant/assistant_alarm_timer_controller.cc

@@ -45,6 +45,7 @@ chromeos::assistant::mojom::AssistantNotificationPtr CreateTimerNotification(
   using chromeos::assistant::mojom::AssistantNotification;
   using chromeos::assistant::mojom::AssistantNotificationButton;
   using chromeos::assistant::mojom::AssistantNotificationPtr;
+  using chromeos::assistant::mojom::AssistantNotificationType;
 
   const std::string title =
       l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_TITLE);
@@ -55,6 +56,14 @@ chromeos::assistant::mojom::AssistantNotificationPtr CreateTimerNotification(
 
   AssistantNotificationPtr notification = AssistantNotification::New();
 
+  // If in-Assistant notifications are supported, we'll allow alarm/timer
+  // notifications to show in either Assistant UI or the Message Center.
+  // Otherwise, we'll only allow the notification to show in the Message Center.
+  notification->type =
+      chromeos::assistant::features::IsInAssistantNotificationsEnabled()
+          ? AssistantNotificationType::kPreferInAssistant
+          : AssistantNotificationType::kSystem;
+
   notification->title = title;
   notification->message = message;
   notification->action_url = action_url;

ash/assistant/assistant_notification_controller.cc

@@ -58,6 +58,13 @@ message_center::NotifierId GetNotifierId() {
       message_center::NotifierType::SYSTEM_COMPONENT, kNotifierId);
 }
 
+bool IsSystemNotification(
+    const chromeos::assistant::mojom::AssistantNotification* notification) {
+  using chromeos::assistant::mojom::AssistantNotificationType;
+  return notification->type == AssistantNotificationType::kPreferInAssistant ||
+         notification->type == AssistantNotificationType::kSystem;
+}
+
 bool IsValidActionUrl(const GURL& action_url) {
   return action_url.is_valid() && (action_url.SchemeIsHTTPOrHTTPS() ||
                                    assistant::util::IsDeepLinkUrl(action_url));
@@ -73,11 +80,13 @@ AssistantNotificationController::AssistantNotificationController(
       binding_(this),
       notifier_id_(GetNotifierId()) {
   AddModelObserver(this);
+  assistant_controller_->AddObserver(this);
   message_center::MessageCenter::Get()->AddObserver(this);
 }
 
 AssistantNotificationController::~AssistantNotificationController() {
   message_center::MessageCenter::Get()->RemoveObserver(this);
+  assistant_controller_->RemoveObserver(this);
   RemoveModelObserver(this);
 }
 
@@ -101,10 +110,69 @@ void AssistantNotificationController::SetAssistant(
   assistant_ = assistant;
 }
 
+// AssistantControllerObserver -------------------------------------------------
+
+void AssistantNotificationController::OnAssistantControllerConstructed() {
+  assistant_controller_->ui_controller()->AddModelObserver(this);
+}
+
+void AssistantNotificationController::OnAssistantControllerDestroying() {
+  assistant_controller_->ui_controller()->RemoveModelObserver(this);
+}
+
+// AssistantUiModelObserver ----------------------------------------------------
+
+void AssistantNotificationController::OnUiVisibilityChanged(
+    AssistantVisibility new_visibility,
+    AssistantVisibility old_visibility,
+    base::Optional<AssistantEntryPoint> entry_point,
+    base::Optional<AssistantExitPoint> exit_point) {
+  switch (new_visibility) {
+    case AssistantVisibility::kVisible:
+      // When the Assistant UI becomes visible we convert any notifications of
+      // type |kPreferInAssistant| to type |kInAssistant|. This will cause them
+      // to be removed from the Message Center (if they had previously been
+      // added) and to finish out their lifetimes as in-Assistant notifications.
+      for (const auto* notification : model_.GetNotificationsByType(
+               AssistantNotificationType::kPreferInAssistant)) {
+        auto update = notification->Clone();
+        update->type = AssistantNotificationType::kInAssistant;
+        model_.AddOrUpdateNotification(std::move(update));
+      }
+      break;
+    case AssistantVisibility::kHidden:
+    case AssistantVisibility::kClosed:
+      // When the Assistant UI is no longer visible to the user we remove any
+      // notifications of type |kInAssistant| as this type of notification does
+      // not outlive the Assistant view hierarchy.
+      if (old_visibility == AssistantVisibility::kVisible) {
+        for (const auto* notification : model_.GetNotificationsByType(
+                 AssistantNotificationType::kInAssistant)) {
+          model_.RemoveNotificationById(notification->client_id,
+                                        /*from_server=*/false);
+        }
+      }
+      break;
+  }
+}
+
 // mojom::AssistantNotificationController --------------------------------------
 
 void AssistantNotificationController::AddOrUpdateNotification(
     AssistantNotificationPtr notification) {
+  const AssistantVisibility visibility =
+      assistant_controller_->ui_controller()->model()->visibility();
+
+  // If Assistant UI is visible and |notification| is of type
+  // |kPreferInAssistant|, we convert it to a notification of type
+  // |kInAssistant|. This will cause the notification to be removed from the
+  // Message Center (if it had previously been added) and it will finish out its
+  // lifetime as an in-Assistant notification.
+  if (visibility == AssistantVisibility::kVisible &&
+      notification->type == AssistantNotificationType::kPreferInAssistant) {
+    notification->type = AssistantNotificationType::kInAssistant;
+  }
+
   model_.AddOrUpdateNotification(std::move(notification));
 }
 
@@ -132,6 +200,10 @@ void AssistantNotificationController::OnNotificationAdded(
   if (!Shell::Get()->voice_interaction_controller()->notification_enabled())
     return;
 
+  // We only show system notifications in the Message Center.
+  if (!IsSystemNotification(notification))
+    return;
+
   message_center::MessageCenter::Get()->AddNotification(
       CreateSystemNotification(notifier_id_, notification));
 }
@@ -142,6 +214,15 @@ void AssistantNotificationController::OnNotificationUpdated(
   if (!Shell::Get()->voice_interaction_controller()->notification_enabled())
     return;
 
+  // If the notification that was updated is *not* a system notification, we
+  // need to ensure that it is removed from the Message Center (given that it
+  // may have been a system notification prior to update).
+  if (!IsSystemNotification(notification)) {
+    message_center::MessageCenter::Get()->RemoveNotification(
+        notification->client_id, /*by_user=*/false);
+    return;
+  }
+
   message_center::MessageCenter::Get()->UpdateNotification(
       notification->client_id,
       CreateSystemNotification(notifier_id_, notification));
@@ -200,7 +281,13 @@ void AssistantNotificationController::OnNotificationClicked(
 void AssistantNotificationController::OnNotificationRemoved(
     const std::string& notification_id,
     bool by_user) {
-  model_.RemoveNotificationById(notification_id, /*from_server=*/false);
+  // If the notification that was removed is a system notification, we need to
+  // update our notification model. If it is *not* a system notification, then
+  // the notification was removed from the Message Center due to a change of
+  // |type| so it should be retained in the model.
+  const auto* notification = model_.GetNotificationById(notification_id);
+  if (notification && IsSystemNotification(notification))
+    model_.RemoveNotificationById(notification_id, /*from_server=*/false);
 }
 
 }  // namespace ash

ash/assistant/assistant_notification_controller.h

@@ -8,8 +8,10 @@
 #include <string>
 
 #include "ash/ash_export.h"
+#include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/model/assistant_notification_model.h"
 #include "ash/assistant/model/assistant_notification_model_observer.h"
+#include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/public/interfaces/assistant_controller.mojom.h"
 #include "base/macros.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
@@ -24,6 +26,8 @@ class AssistantController;
 // The class to manage Assistant notifications.
 class ASH_EXPORT AssistantNotificationController
     : public mojom::AssistantNotificationController,
+      public AssistantControllerObserver,
+      public AssistantUiModelObserver,
       public AssistantNotificationModelObserver,
       public message_center::MessageCenterObserver {
  public:
@@ -31,6 +35,8 @@ class ASH_EXPORT AssistantNotificationController
       chromeos::assistant::mojom::AssistantNotification;
   using AssistantNotificationPtr =
       chromeos::assistant::mojom::AssistantNotificationPtr;
+  using AssistantNotificationType =
+      chromeos::assistant::mojom::AssistantNotificationType;
 
   explicit AssistantNotificationController(
       AssistantController* assistant_controller);
@@ -48,6 +54,17 @@ class ASH_EXPORT AssistantNotificationController
   // Provides a pointer to the |assistant| owned by AssistantController.
   void SetAssistant(chromeos::assistant::mojom::Assistant* assistant);
 
+  // AssistantControllerObserver:
+  void OnAssistantControllerConstructed() override;
+  void OnAssistantControllerDestroying() override;
+
+  // AssistantUiModelObserver:
+  void OnUiVisibilityChanged(
+      AssistantVisibility new_visibility,
+      AssistantVisibility old_visibility,
+      base::Optional<AssistantEntryPoint> entry_point,
+      base::Optional<AssistantExitPoint> exit_point) override;
+
   // mojom::AssistantNotificationController:
   void AddOrUpdateNotification(AssistantNotificationPtr notification) override;
   void RemoveNotificationById(const std::string& id, bool from_server) override;

ash/assistant/model/assistant_notification_model.cc

@@ -78,6 +78,17 @@ AssistantNotificationModel::GetNotificationById(const std::string& id) const {
   return it != notifications_.end() ? it->second.get() : nullptr;
 }
 
+std::vector<const chromeos::assistant::mojom::AssistantNotification*>
+AssistantNotificationModel::GetNotificationsByType(
+    AssistantNotificationType type) const {
+  std::vector<const AssistantNotification*> notifications;
+  for (const auto& notification : notifications_) {
+    if (notification.second->type == type)
+      notifications.push_back(notification.second.get());
+  }
+  return notifications;
+}
+
 bool AssistantNotificationModel::HasNotificationForId(
     const std::string& id) const {
   return base::ContainsKey(notifications_, id);

ash/assistant/model/assistant_notification_model.h

@@ -7,6 +7,7 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/component_export.h"
 #include "base/macros.h"
@@ -25,6 +26,8 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantNotificationModel {
       chromeos::assistant::mojom::AssistantNotification;
   using AssistantNotificationPtr =
       chromeos::assistant::mojom::AssistantNotificationPtr;
+  using AssistantNotificationType =
+      chromeos::assistant::mojom::AssistantNotificationType;
 
   AssistantNotificationModel();
   ~AssistantNotificationModel();
@@ -54,6 +57,10 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantNotificationModel {
   // Returns the notification uniquely identified by |id|.
   const AssistantNotification* GetNotificationById(const std::string& id) const;
 
+  // Returns all notifications matching the specified |type|.
+  std::vector<const AssistantNotification*> GetNotificationsByType(
+      AssistantNotificationType type) const;
+
   // Returns true if the model contains a notification uniquely identified by
   // |id|, otherwise false.
   bool HasNotificationForId(const std::string& id) const;

ash/assistant/ui/assistant_notification_overlay.cc

@@ -28,9 +28,11 @@ AssistantNotificationOverlay::AssistantNotificationOverlay(
 
   // The AssistantViewDelegate outlives the Assistant view hierarchy.
   delegate_->AddNotificationModelObserver(this);
+  delegate_->AddUiModelObserver(this);
 }
 
 AssistantNotificationOverlay::~AssistantNotificationOverlay() {
+  delegate_->RemoveUiModelObserver(this);
   delegate_->RemoveNotificationModelObserver(this);
 }
 
@@ -54,10 +56,30 @@ void AssistantNotificationOverlay::ViewHierarchyChanged(
     PreferredSizeChanged();
 }
 
+void AssistantNotificationOverlay::OnUiVisibilityChanged(
+    AssistantVisibility new_visibility,
+    AssistantVisibility old_visibility,
+    base::Optional<AssistantEntryPoint> entry_point,
+    base::Optional<AssistantExitPoint> exit_point) {
+  if (new_visibility != AssistantVisibility::kVisible)
+    return;
+
+  // We need to create views for any notifications that currently exist of type
+  // |kInAssistant| as they should be shown within Assistant UI.
+  using chromeos::assistant::mojom::AssistantNotificationType;
+  for (const auto* notification :
+       delegate_->GetNotificationModel()->GetNotificationsByType(
+           AssistantNotificationType::kInAssistant)) {
+    AddChildView(new AssistantNotificationView(delegate_, notification));
+  }
+}
+
 void AssistantNotificationOverlay::OnNotificationAdded(
     const AssistantNotification* notification) {
-  // TODO(dmblack): Only add views for notifications of the appropriate type.
-  AddChildView(new AssistantNotificationView(delegate_, notification));
+  // We only show notifications of type |kInAssistant| with Assistant UI.
+  using chromeos::assistant::mojom::AssistantNotificationType;
+  if (notification->type == AssistantNotificationType::kInAssistant)
+    AddChildView(new AssistantNotificationView(delegate_, notification));
 }
 
 void AssistantNotificationOverlay::InitLayout() {

ash/assistant/ui/assistant_notification_overlay.h

@@ -6,6 +6,7 @@
 #define ASH_ASSISTANT_UI_ASSISTANT_NOTIFICATION_OVERLAY_H_
 
 #include "ash/assistant/model/assistant_notification_model_observer.h"
+#include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/assistant/ui/assistant_overlay.h"
 #include "base/component_export.h"
 #include "base/macros.h"
@@ -18,6 +19,7 @@ class AssistantViewDelegate;
 // responsible for parenting in-Assistant notifications.
 class COMPONENT_EXPORT(ASSISTANT_UI) AssistantNotificationOverlay
     : public AssistantOverlay,
+      public AssistantUiModelObserver,
       public AssistantNotificationModelObserver {
  public:
   AssistantNotificationOverlay(AssistantViewDelegate* delegate);
@@ -29,6 +31,13 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantNotificationOverlay
   void ViewHierarchyChanged(
       const ViewHierarchyChangedDetails& details) override;
 
+  // AssistantUiModelObserver:
+  void OnUiVisibilityChanged(
+      AssistantVisibility new_visibility,
+      AssistantVisibility old_visibility,
+      base::Optional<AssistantEntryPoint> entry_point,
+      base::Optional<AssistantExitPoint> exit_point) override;
+
   // AssistantNotificationModelObserver:
   void OnNotificationAdded(const AssistantNotification* notification) override;
 

ash/assistant/ui/assistant_notification_view.cc

@@ -79,9 +79,21 @@ void AssistantNotificationView::ButtonPressed(views::Button* sender,
 
 void AssistantNotificationView::OnNotificationUpdated(
     const AssistantNotification* notification) {
+  // We only care about the |notification| being updated if it is the
+  // notification associated with this view.
   if (notification->client_id != notification_id_)
     return;
 
+  using AssistantNotificationType =
+      chromeos::assistant::mojom::AssistantNotificationType;
+
+  // If the notification associated with this view is no longer of type
+  // |kInAssistant|, it should not be shown in Assistant UI.
+  if (notification->type != AssistantNotificationType::kInAssistant) {
+    delete this;
+    return;
+  }
+
   // Title/Message.
   title_->SetText(base::UTF8ToUTF16(notification->title));
   message_->SetText(base::UTF8ToUTF16(notification->message));

chromeos/services/assistant/public/mojom/assistant.mojom

@@ -193,6 +193,32 @@ struct AssistantSuggestion {
   url.mojom.Url action_url;
 };
 
+// Enumeration of notification types.
+enum AssistantNotificationType {
+  // A notification of type |kInAssistant| will only be displayed within
+  // Assistant UI. If Assistant UI is not visible at the time of notification
+  // creation, the notification will be queued up until Assistant UI becomes
+  // visible. When Assistant UI dismisses, notifications of this type are
+  // dismissed.
+  kInAssistant,
+
+  // A notification of type |kPreferInAssistant| may be shown in either
+  // Assistant UI or the Message Center, depending on Assistant visibility
+  // state. If Assistant UI is not visible, notifications of this type will
+  // show immediately in the Message Center unless the user has opted out of
+  // seeing system notifications. Once Assistant UI becomes visible, or if it
+  // is already visible at the time of notification creation, notifications of
+  // this type are converted to notifications of type |kInAssistant| and they
+  // will finish out their lifetimes as in-Assistant notifications.
+  kPreferInAssistant,
+
+  // A notification of type |kSystem| will only be displayed within the Message
+  // Center. It is immediately added to the Message Center regardless of
+  // Assistant UI visibility state, although it may not be added if the user has
+  // opted out of seeing system notifications.
+  kSystem,
+};
+
 // Models a notification button.
 struct AssistantNotificationButton {
   // Display text of the button.
@@ -204,6 +230,9 @@ struct AssistantNotificationButton {
 
 // Models an Assistant notification.
 struct AssistantNotification {
+  // Type of the notification.
+  AssistantNotificationType type = AssistantNotificationType.kSystem;
+
   // Title of the notification.
   string title;