Use small previews for in-app tablet mode.

When in tablet mode and in an app, the shelf config changes and the
default size for holding space previews is too large. Now, shelf config
changes are observed and holding space previews are resized to fit.

Resizing previews surfaced a few issues also fixed in this CL:

1. Layers were not being destroyed after shifting outside the viewport.
2. More than 3 previews were visible to the user at a time.

Issue 2 resulted in the 4th preview being slightly visible, making it
look like paint artifacts of the 3rd preview. Not showing the 4th
preview introduced some jank when shifting it in because it would
appear out of nowhere before shifting into position. To soften this,
added an opacity animation when creating the view.

Bug: 1165326
Change-Id: Iae5d00c98b8cb24025d0275c6af49b1a13d48f36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628589
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843365}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -28,7 +28,8 @@ constexpr gfx::Size kHoldingSpaceScreenCaptureSize(104, 80);
 constexpr int kHoldingSpaceSectionChildSpacing = 16;
 constexpr float kHoldingSpaceSelectedOverlayOpacity = 0.24f;
 constexpr int kHoldingSpaceTrayIconMaxVisiblePreviews = 3;
-constexpr int kHoldingSpaceTrayIconPreviewSize = 32;
+constexpr int kHoldingSpaceTrayIconDefaultPreviewSize = 32;
+constexpr int kHoldingSpaceTrayIconSmallPreviewSize = 28;
 constexpr int kHoldingSpaceTrayIconSize = 20;
 
 // Context menu commands.

ash/public/cpp/holding_space/holding_space_image.cc

@@ -131,10 +131,10 @@ gfx::Size HoldingSpaceImage::GetMaxSizeForType(HoldingSpaceItem::Type type) {
       break;
   }
   // To avoid pixelation, ensure that the holding space image size is at least
-  // as large as the tray icon preview size. The image will be scaled down
-  // elsewhere if needed.
-  size.SetToMax(gfx::Size(kHoldingSpaceTrayIconPreviewSize,
-                          kHoldingSpaceTrayIconPreviewSize));
+  // as large as the default tray icon preview size. The image will be scaled
+  // down elsewhere if needed.
+  size.SetToMax(gfx::Size(kHoldingSpaceTrayIconDefaultPreviewSize,
+                          kHoldingSpaceTrayIconDefaultPreviewSize));
   return size;
 }
 

ash/system/holding_space/holding_space_tray_icon.cc

@@ -35,6 +35,16 @@ namespace {
 constexpr base::TimeDelta kPreviewItemUpdateDelayIncrement =
     base::TimeDelta::FromMilliseconds(50);
 
+// Helpers ---------------------------------------------------------------------
+
+// Returns the size of previews given the current shelf configuration.
+int GetPreviewSize() {
+  ShelfConfig* const shelf_config = ShelfConfig::Get();
+  return shelf_config->in_tablet_mode() && shelf_config->is_in_app()
+             ? kHoldingSpaceTrayIconSmallPreviewSize
+             : kHoldingSpaceTrayIconDefaultPreviewSize;
+}
+
 }  // namespace
 
 // Animation for resizing the previews icon. The animation updates the icon
@@ -110,7 +120,9 @@ class HoldingSpaceTrayIcon::ResizeAnimation
 HoldingSpaceTrayIcon::HoldingSpaceTrayIcon(Shelf* shelf) : shelf_(shelf) {
   SetID(kHoldingSpaceTrayPreviewsIconId);
   InitLayout();
-  shell_observer_.Add(Shell::Get());
+
+  shell_observer_.Observe(Shell::Get());
+  shelf_config_observer_.Observe(ShelfConfig::Get());
 }
 
 HoldingSpaceTrayIcon::~HoldingSpaceTrayIcon() = default;
@@ -135,10 +147,11 @@ gfx::Size HoldingSpaceTrayIcon::CalculatePreferredSize() const {
   const int num_visible_previews =
       std::min(kHoldingSpaceTrayIconMaxVisiblePreviews,
                static_cast<int>(previews_by_id_.size()));
+  const int preview_size = GetPreviewSize();
 
-  int primary_axis_size = kTrayItemSize;
+  int primary_axis_size = preview_size;
   if (num_visible_previews > 1)
-    primary_axis_size += (num_visible_previews - 1) * kTrayItemSize / 2;
+    primary_axis_size += (num_visible_previews - 1) * preview_size / 2;
 
   return shelf_->PrimaryAxisValue(
       /*horizontal=*/gfx::Size(primary_axis_size, kTrayItemSize),
@@ -147,7 +160,9 @@ gfx::Size HoldingSpaceTrayIcon::CalculatePreferredSize() const {
 
 void HoldingSpaceTrayIcon::InitLayout() {
   SetLayoutManager(std::make_unique<views::FillLayout>());
-  SetPreferredSize(gfx::Size(kTrayItemSize, kTrayItemSize));
+
+  const int preview_size = GetPreviewSize();
+  SetPreferredSize(gfx::Size(preview_size, preview_size));
 
   SetPaintToLayer(ui::LAYER_NOT_DRAWN);
   layer()->SetFillsBoundsOpaquely(false);
@@ -259,6 +274,20 @@ void HoldingSpaceTrayIcon::OnShelfAlignmentChanged(
   previews_container_->SetTransform(gfx::Transform());
 }
 
+void HoldingSpaceTrayIcon::OnShelfConfigUpdated() {
+  removed_previews_.clear();
+  for (const auto& preview : previews_by_id_)
+    preview.second->OnShelfConfigChanged();
+
+  if (resize_animation_) {
+    resize_animation_->AdvanceToEnd();
+    resize_animation_.reset();
+  }
+
+  SetPreferredSize(CalculatePreferredSize());
+  previews_container_->SetTransform(gfx::Transform());
+}
+
 void HoldingSpaceTrayIcon::OnOldItemAnimatedOut(
     HoldingSpaceTrayIconPreview* preview,
     const base::RepeatingClosure& callback) {

ash/system/holding_space/holding_space_tray_icon.h

@@ -11,9 +11,10 @@
 #include "ash/ash_export.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
+#include "ash/public/cpp/shelf_config.h"
 #include "ash/shell.h"
 #include "ash/shell_observer.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 #include "ui/views/view.h"
 
@@ -24,7 +25,8 @@ class Shelf;
 
 // The icon used to represent holding space in its tray in the shelf.
 class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
-                                        public ShellObserver {
+                                        public ShellObserver,
+                                        public ShelfConfig::Observer {
  public:
   METADATA_HEADER(HoldingSpaceTrayIcon);
 
@@ -67,6 +69,9 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   void OnShelfAlignmentChanged(aura::Window* root_window,
                                ShelfAlignment old_alignment) override;
 
+  // ShelfConfigObserver:
+  void OnShelfConfigUpdated() override;
+
   void InitLayout();
 
   // Updates the previews in the icon without an animation. Assumes that the
@@ -116,12 +121,15 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   // Helper to run icon resize animation.
   std::unique_ptr<ResizeAnimation> resize_animation_;
 
-  ScopedObserver<Shell,
+  base::ScopedObservation<Shell,
                           ShellObserver,
                           &Shell::AddShellObserver,
                           &Shell::RemoveShellObserver>
       shell_observer_{this};
 
+  base::ScopedObservation<ShelfConfig, ShelfConfig::Observer>
+      shelf_config_observer_{this};
+
   // The factory to which callbacks for stages of the previews list update are
   // bound to. The goal is to easily cancel in-progress updates if the list of
   // items gets updated.

ash/system/holding_space/holding_space_tray_icon_preview.cc

@@ -47,10 +47,22 @@ constexpr base::TimeDelta kShiftAnimationDuration =
 
 // Helpers ---------------------------------------------------------------------
 
-// Returns the preview icon contents size.
-gfx::Size GetPreviewSize() {
-  return gfx::Size(kHoldingSpaceTrayIconPreviewSize,
-                   kHoldingSpaceTrayIconPreviewSize);
+// Returns true if small previews should be used given the current shelf
+// configuration, false otherwise.
+bool ShouldUseSmallPreviews() {
+  ShelfConfig* const shelf_config = ShelfConfig::Get();
+  return shelf_config->in_tablet_mode() && shelf_config->is_in_app();
+}
+
+// Returns the size for previews. If `use_small_previews` is absent it will be
+// determined from the current shelf configuration.
+gfx::Size GetPreviewSize(
+    const base::Optional<bool>& use_small_previews = base::nullopt) {
+  return use_small_previews.value_or(ShouldUseSmallPreviews())
+             ? gfx::Size(kHoldingSpaceTrayIconSmallPreviewSize,
+                         kHoldingSpaceTrayIconSmallPreviewSize)
+             : gfx::Size(kHoldingSpaceTrayIconDefaultPreviewSize,
+                         kHoldingSpaceTrayIconDefaultPreviewSize);
 }
 
 // Returns the shadow details for painting elevation.
@@ -121,7 +133,10 @@ HoldingSpaceTrayIconPreview::HoldingSpaceTrayIconPreview(
     Shelf* shelf,
     views::View* container,
     const HoldingSpaceItem* item)
-    : shelf_(shelf), container_(container), item_(item) {
+    : shelf_(shelf),
+      container_(container),
+      item_(item),
+      use_small_previews_(ShouldUseSmallPreviews()) {
   const gfx::Size size(GetPreviewSize());
   contents_image_ = gfx::ImageSkia(
       std::make_unique<ContentsImageSource>(item->image().GetImageSkia(size)),
@@ -230,8 +245,11 @@ void HoldingSpaceTrayIconPreview::AnimateShift(base::TimeDelta delay) {
   index_ = *pending_index_;
   pending_index_.reset();
 
-  if (!layer_ && NeedsLayer())
+  bool created_layer = false;
+  if (!layer_ && NeedsLayer()) {
     CreateLayer(transform_);
+    created_layer = true;
+  }
 
   // Calculate the target preview transform for the new position in the icon.
   // Avoid adjustments based on relative index change, as the current transform
@@ -245,22 +263,37 @@ void HoldingSpaceTrayIconPreview::AnimateShift(base::TimeDelta delay) {
   if (!layer_)
     return;
 
+  // If the `layer_` has just been created because it is shifting into the
+  // viewport, animate in its opacity.
+  if (created_layer)
+    layer_->SetOpacity(0.f);
+
   ui::ScopedLayerAnimationSettings scoped_settings(layer_->GetAnimator());
+  scoped_settings.AddObserver(this);
   scoped_settings.SetPreemptionStrategy(
       ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
 
-  std::unique_ptr<ui::LayerAnimationSequence> sequence =
-      std::make_unique<ui::LayerAnimationSequence>();
-  sequence->AddElement(ui::LayerAnimationElement::CreatePauseElement(
+  auto opacity_sequence = std::make_unique<ui::LayerAnimationSequence>();
+  if (created_layer) {
+    opacity_sequence->AddElement(ui::LayerAnimationElement::CreatePauseElement(
+        ui::LayerAnimationElement::OPACITY, delay));
+    opacity_sequence->AddElement(
+        ui::LayerAnimationElement::CreateOpacityElement(
+            1.f, kShiftAnimationDuration));
+  }
+
+  auto transform_sequence = std::make_unique<ui::LayerAnimationSequence>();
+  transform_sequence->AddElement(ui::LayerAnimationElement::CreatePauseElement(
       ui::LayerAnimationElement::TRANSFORM, delay));
 
   std::unique_ptr<ui::LayerAnimationElement> shift =
       ui::LayerAnimationElement::CreateTransformElement(
           transform_, kShiftAnimationDuration);
   shift->set_tween_type(gfx::Tween::FAST_OUT_SLOW_IN);
-  sequence->AddElement(std::move(shift));
+  transform_sequence->AddElement(std::move(shift));
 
-  layer_->GetAnimator()->StartAnimation(sequence.release());
+  layer_->GetAnimator()->StartTogether(
+      {opacity_sequence.release(), transform_sequence.release()});
 }
 
 void HoldingSpaceTrayIconPreview::AdjustTransformForContainerSizeChange(
@@ -286,10 +319,10 @@ void HoldingSpaceTrayIconPreview::OnShelfAlignmentChanged(
   if (IsHorizontal(old_shelf_alignment) == IsHorizontal(new_shelf_alignment))
     return;
 
-  // Since shelf orientation has changed, the target `transform_` needs to be
+  // Because shelf orientation has changed, the target `transform_` needs to be
   // updated. First stop the current animation to immediately advance to target
   // end values.
-  const auto& weak_ptr = weak_factory_.GetWeakPtr();
+  const auto weak_ptr = weak_factory_.GetWeakPtr();
   if (layer_ && layer_->GetAnimator()->is_animating())
     layer_->GetAnimator()->StopAnimating();
 
@@ -322,6 +355,43 @@ void HoldingSpaceTrayIconPreview::OnShelfAlignmentChanged(
   }
 }
 
+void HoldingSpaceTrayIconPreview::OnShelfConfigChanged() {
+  // If the change in shelf configuration hasn't affected whether or not small
+  // previews should be used, no action needs to be taken.
+  const bool use_small_previews = ShouldUseSmallPreviews();
+  if (use_small_previews_ == use_small_previews)
+    return;
+
+  use_small_previews_ = use_small_previews;
+
+  // Because the size of previews is changing, the target `transform_` needs to
+  // be updated. First stop the current animation to immediately advance to
+  // target end values.
+  const auto weak_ptr = weak_factory_.GetWeakPtr();
+  if (layer_ && layer_->GetAnimator()->is_animating())
+    layer_->GetAnimator()->StopAnimating();
+
+  // This instance may have been deleted as a result of stopping the current
+  // animation if it was in the process of animating out.
+  if (!weak_ptr)
+    return;
+
+  // Adjust `translation` to account for the change in size.
+  gfx::Vector2dF translation = transform_.To2dTranslation();
+  translation.Scale(1.f / GetPreviewSize(!use_small_previews_).width());
+  translation.Scale(GetPreviewSize().width());
+  transform_.MakeIdentity();
+  transform_.Translate(translation);
+
+  if (layer_) {
+    UpdateLayerBounds();
+    layer_->SetTransform(transform_);
+  }
+
+  // Invalidate `contents_image_` so it is resized.
+  OnHoldingSpaceItemImageChanged();
+}
+
 // TODO(crbug.com/1142572): Support theming.
 void HoldingSpaceTrayIconPreview::OnPaintLayer(
     const ui::PaintContext& context) {
@@ -406,7 +476,7 @@ void HoldingSpaceTrayIconPreview::DestroyLayer() {
 }
 
 bool HoldingSpaceTrayIconPreview::NeedsLayer() const {
-  return index_ && *index_ <= kHoldingSpaceTrayIconMaxVisiblePreviews;
+  return index_ && *index_ < kHoldingSpaceTrayIconMaxVisiblePreviews;
 }
 
 void HoldingSpaceTrayIconPreview::InvalidateLayer() {
@@ -436,9 +506,11 @@ void HoldingSpaceTrayIconPreview::UpdateLayerBounds() {
   // with a positive offset.
   const gfx::Size size = GetPreviewSize();
   gfx::Point origin;
-  if (shelf_->IsHorizontalAlignment() && base::i18n::IsRTL()) {
-    origin = container_->GetLocalBounds().top_right() -
-             gfx::Vector2d(size.width(), 0);
+  if (shelf_->IsHorizontalAlignment()) {
+    gfx::Rect container_bounds = container_->GetLocalBounds();
+    if (base::i18n::IsRTL())
+      origin = container_bounds.top_right() - gfx::Vector2d(size.width(), 0);
+    origin.Offset(0, (container_bounds.height() - size.height()) / 2);
   }
   gfx::Rect bounds(origin, size);
   if (bounds != layer_->bounds())

ash/system/holding_space/holding_space_tray_icon_preview.h

@@ -73,6 +73,9 @@ class ASH_EXPORT HoldingSpaceTrayIconPreview
   void OnShelfAlignmentChanged(ShelfAlignment old_shelf_alignment,
                                ShelfAlignment new_shelf_alignment);
 
+  // Invoked when the `shelf_` configuration has changed.
+  void OnShelfConfigChanged();
+
   // Returns the holding space `item_` visually represented by this preview.
   const HoldingSpaceItem* item() const { return item_; }
 
@@ -136,6 +139,10 @@ class ASH_EXPORT HoldingSpaceTrayIconPreview
   // The holding space item this preview represents.
   const HoldingSpaceItem* item_;
 
+  // Whether or not this preview is currently using small dimensions. This is
+  // done when in tablet mode and an app is in use.
+  bool use_small_previews_ = false;
+
   // A cached representation of the associated holding space `item_`'s image
   // which has been cropped, resized, and clipped to a circle to be painted at
   // `layer_`'s contents bounds.