Add pin option for screenshots

Consolidate pin logic into HoldingSpaceItemView since it will now
be common between screenshots and chip views.

Bug: 1131262
Change-Id: I600f5e7bb78726fe4b07afd379dd77857cd6bef2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429533
Commit-Queue: Ahmed Mehfooz <amehfooz@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#811108}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -26,6 +26,7 @@ constexpr int kHoldingSpaceColumnSpacing = 8;
 constexpr int kHoldingSpaceColumnWidth = 160;
 constexpr int kHoldingSpaceCornerRadius = 8;
 constexpr int kHoldingSpaceRowSpacing = 8;
+constexpr gfx::Insets kHoldingSpaceScreenshotPadding(8);
 constexpr int kHoldingSpaceScreenshotSpacing = 8;
 constexpr gfx::Size kHoldingSpaceScreenshotSize(104, 80);
 constexpr gfx::Insets kHoldingSpaceScreenshotsContainerPadding(8, 0);

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -4,28 +4,21 @@
 
 #include "ash/system/holding_space/holding_space_item_chip_view.h"
 
-#include "ash/public/cpp/holding_space/holding_space_client.h"
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
-#include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
-#include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/style/ash_color_provider.h"
 #include "ash/system/holding_space/holding_space_item_view.h"
-#include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_item_style.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/user/rounded_image_view.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
-#include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/animation/flood_fill_ink_drop_ripple.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/background.h"
-#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
-#include "ui/views/vector_icons.h"
 
 namespace ash {
 
@@ -49,8 +42,6 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(
   TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::HOLDING_SPACE_TITLE);
   style.SetupLabel(label_);
 
-  AddPinButton();
-
   const auto* color_provider = AshColorProvider::Get();
   SetBackground(views::CreateRoundedRectBackground(
       color_provider->GetControlsLayerColor(
@@ -60,7 +51,6 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(
   SetInkDropMode(InkDropMode::ON_NO_GESTURE_HANDLER);
   SetInkDropVisibleOpacity(
       color_provider->GetRippleAttributes().inkdrop_opacity);
-  SetNotifyEnterExitOnChild(true);
 
   // Subscribe to be notified of changes to `item_`'s image.
   image_subscription_ =
@@ -68,55 +58,11 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(
           &HoldingSpaceItemChipView::UpdateImage, base::Unretained(this)));
 
   UpdateImage();
-}
-
-HoldingSpaceItemChipView::~HoldingSpaceItemChipView() = default;
 
-void HoldingSpaceItemChipView::OnMouseEvent(ui::MouseEvent* event) {
-  switch (event->type()) {
-    case ui::ET_MOUSE_ENTERED:
-    case ui::ET_MOUSE_EXITED:
-      UpdatePin();
-      break;
-    default:
-      break;
-  }
-  views::InkDropHostView::OnMouseEvent(event);
+  AddPin(this /*parent*/);
 }
 
-void HoldingSpaceItemChipView::ButtonPressed(views::Button* sender,
-                                             const ui::Event& event) {
-  DCHECK_EQ(sender, pin_);
-  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
-      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
-                                            item()->file_path()));
-
-  // Unpinning `item()` may result in the destruction of this view.
-  auto weak_ptr = weak_factory_.GetWeakPtr();
-  if (is_item_pinned)
-    HoldingSpaceController::Get()->client()->UnpinItems({item()});
-  else
-    HoldingSpaceController::Get()->client()->PinItems({item()});
-
-  if (weak_ptr)
-    UpdatePin();
-}
-
-void HoldingSpaceItemChipView::AddPinButton() {
-  pin_ = AddChildView(std::make_unique<views::ToggleImageButton>(this));
-  pin_->SetVisible(false);
-
-  const SkColor icon_color = AshColorProvider::Get()->GetContentLayerColor(
-      AshColorProvider::ContentLayerType::kSystemMenuIconColor);
-
-  const gfx::ImageSkia unpinned_icon =
-      gfx::CreateVectorIcon(views::kUnpinIcon, icon_color);
-  const gfx::ImageSkia pinned_icon =
-      gfx::CreateVectorIcon(views::kPinIcon, icon_color);
-
-  pin_->SetImage(views::Button::STATE_NORMAL, unpinned_icon);
-  pin_->SetToggledImage(views::Button::STATE_NORMAL, &pinned_icon);
-}
+HoldingSpaceItemChipView::~HoldingSpaceItemChipView() = default;
 
 void HoldingSpaceItemChipView::UpdateImage() {
   image_->SetImage(
@@ -124,20 +70,6 @@ void HoldingSpaceItemChipView::UpdateImage() {
       gfx::Size(kHoldingSpaceChipIconSize, kHoldingSpaceChipIconSize));
 }
 
-void HoldingSpaceItemChipView::UpdatePin() {
-  if (!IsMouseHovered()) {
-    pin_->SetVisible(false);
-    return;
-  }
-
-  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
-      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
-                                            item()->file_path()));
-
-  pin_->SetToggled(!is_item_pinned);
-  pin_->SetVisible(true);
-}
-
 BEGIN_METADATA(HoldingSpaceItemChipView, HoldingSpaceItemView)
 END_METADATA
 

ash/system/holding_space/holding_space_item_chip_view.h

@@ -10,12 +10,10 @@
 #include "ash/ash_export.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/system/holding_space/holding_space_item_view.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 
 namespace views {
 class Label;
-class ToggleImageButton;
 }  // namespace views
 
 namespace ash {
@@ -29,8 +27,7 @@ class RoundedImageView;
 
 // A button with an image derived from a file's thumbnail and file's name as the
 // label.
-class ASH_EXPORT HoldingSpaceItemChipView : public HoldingSpaceItemView,
-                                            public views::ButtonListener {
+class ASH_EXPORT HoldingSpaceItemChipView : public HoldingSpaceItemView {
  public:
   METADATA_HEADER(HoldingSpaceItemChipView);
 
@@ -40,24 +37,13 @@ class ASH_EXPORT HoldingSpaceItemChipView : public HoldingSpaceItemView,
   HoldingSpaceItemChipView& operator=(const HoldingSpaceItemChipView&) = delete;
   ~HoldingSpaceItemChipView() override;
 
-  // HoldingSpaceItemView:
-  void OnMouseEvent(ui::MouseEvent* event) override;
-
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
  private:
-  void AddPinButton();
   void UpdateImage();
-  void UpdatePin();
 
   tray::RoundedImageView* image_ = nullptr;
   views::Label* label_ = nullptr;
-  views::ToggleImageButton* pin_ = nullptr;
 
   std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
-
-  base::WeakPtrFactory<HoldingSpaceItemChipView> weak_factory_{this};
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_item_screenshot_view.cc

@@ -10,6 +10,7 @@
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/user/rounded_image_view.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 
@@ -19,6 +20,7 @@ HoldingSpaceItemScreenshotView::HoldingSpaceItemScreenshotView(
     HoldingSpaceItemViewDelegate* delegate,
     const HoldingSpaceItem* item)
     : HoldingSpaceItemView(delegate, item) {
+  SetPreferredSize(kHoldingSpaceScreenshotSize);
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
   image_ = AddChildView(
@@ -30,6 +32,16 @@ HoldingSpaceItemScreenshotView::HoldingSpaceItemScreenshotView(
                           base::Unretained(this)));
 
   UpdateImage();
+
+  views::View* pin_button_container =
+      AddChildView(std::make_unique<views::View>());
+
+  auto* layout =
+      pin_button_container->SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kHorizontal,
+          kHoldingSpaceScreenshotPadding));
+  layout->set_main_axis_alignment(views::BoxLayout::MainAxisAlignment::kEnd);
+  AddPin(pin_button_container);
 }
 
 HoldingSpaceItemScreenshotView::~HoldingSpaceItemScreenshotView() = default;

ash/system/holding_space/holding_space_item_view.cc

@@ -4,17 +4,24 @@
 
 #include "ash/system/holding_space/holding_space_item_view.h"
 
+#include "ash/public/cpp/holding_space/holding_space_client.h"
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
+#include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
+#include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/style/ash_color_provider.h"
 #include "ash/system/holding_space/holding_space_item_view_delegate.h"
+#include "base/bind.h"
 #include "ui/base/class_property.h"
 #include "ui/gfx/color_palette.h"
+#include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/focus_ring.h"
 #include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
+#include "ui/views/vector_icons.h"
 
 namespace ash {
 
@@ -38,6 +45,8 @@ HoldingSpaceItemView::HoldingSpaceItemView(
   set_context_menu_controller(delegate_);
   set_drag_controller(delegate_);
 
+  SetNotifyEnterExitOnChild(true);
+
   SetPaintToLayer();
   layer()->SetFillsBoundsOpaquely(false);
 
@@ -84,6 +93,18 @@ bool HoldingSpaceItemView::OnKeyPressed(const ui::KeyEvent& event) {
   return delegate_->OnHoldingSpaceItemViewKeyPressed(this, event);
 }
 
+void HoldingSpaceItemView::OnMouseEvent(ui::MouseEvent* event) {
+  switch (event->type()) {
+    case ui::ET_MOUSE_ENTERED:
+    case ui::ET_MOUSE_EXITED:
+      UpdatePin();
+      break;
+    default:
+      break;
+  }
+  views::InkDropHostView::OnMouseEvent(event);
+}
+
 bool HoldingSpaceItemView::OnMousePressed(const ui::MouseEvent& event) {
   return delegate_->OnHoldingSpaceItemViewMousePressed(this, event);
 }
@@ -100,6 +121,56 @@ void HoldingSpaceItemView::SetSelected(bool selected) {
   InvalidateLayout();
 }
 
+void HoldingSpaceItemView::AddPin(views::View* parent) {
+  DCHECK(!pin_);
+
+  pin_ = parent->AddChildView(std::make_unique<views::ToggleImageButton>());
+  pin_->SetVisible(false);
+
+  const SkColor icon_color = AshColorProvider::Get()->GetContentLayerColor(
+      AshColorProvider::ContentLayerType::kSystemMenuIconColor);
+
+  const gfx::ImageSkia unpinned_icon =
+      gfx::CreateVectorIcon(views::kUnpinIcon, icon_color);
+  const gfx::ImageSkia pinned_icon =
+      gfx::CreateVectorIcon(views::kPinIcon, icon_color);
+
+  pin_->SetImage(views::Button::STATE_NORMAL, unpinned_icon);
+  pin_->SetToggledImage(views::Button::STATE_NORMAL, &pinned_icon);
+  pin_->set_callback(base::BindRepeating(&HoldingSpaceItemView::OnPinPressed,
+                                         base::Unretained(this)));
+}
+
+void HoldingSpaceItemView::OnPinPressed() {
+  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
+      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
+                                            item()->file_path()));
+
+  // Unpinning `item()` may result in the destruction of this view.
+  auto weak_ptr = weak_factory_.GetWeakPtr();
+  if (is_item_pinned)
+    HoldingSpaceController::Get()->client()->UnpinItems({item()});
+  else
+    HoldingSpaceController::Get()->client()->PinItems({item()});
+
+  if (weak_ptr)
+    UpdatePin();
+}
+
+void HoldingSpaceItemView::UpdatePin() {
+  if (!IsMouseHovered()) {
+    pin_->SetVisible(false);
+    return;
+  }
+
+  const bool is_item_pinned = HoldingSpaceController::Get()->model()->GetItem(
+      HoldingSpaceItem::GetFileBackedItemId(HoldingSpaceItem::Type::kPinnedFile,
+                                            item()->file_path()));
+
+  pin_->SetToggled(!is_item_pinned);
+  pin_->SetVisible(true);
+}
+
 BEGIN_METADATA(HoldingSpaceItemView, views::InkDropHostView)
 END_METADATA
 

ash/system/holding_space/holding_space_item_view.h

@@ -9,6 +9,10 @@
 #include "ui/views/animation/ink_drop_host_view.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 
+namespace views {
+class ToggleImageButton;
+}  // namespace views
+
 namespace ash {
 
 class HoldingSpaceItem;
@@ -32,20 +36,30 @@ class ASH_EXPORT HoldingSpaceItemView : public views::InkDropHostView {
   SkColor GetInkDropBaseColor() const override;
   void OnGestureEvent(ui::GestureEvent* event) override;
   bool OnKeyPressed(const ui::KeyEvent& event) override;
+  void OnMouseEvent(ui::MouseEvent* event) override;
   bool OnMousePressed(const ui::MouseEvent& event) override;
   void OnMouseReleased(const ui::MouseEvent& event) override;
 
-  const HoldingSpaceItem* item() const { return item_; }
-
   void SetSelected(bool selected);
   bool selected() const { return selected_; }
 
+  const HoldingSpaceItem* item() const { return item_; }
+
+ protected:
+  void AddPin(views::View* parent);
+
  private:
-  HoldingSpaceItemViewDelegate* delegate_;
+  void OnPinPressed();
+  void UpdatePin();
+
+  HoldingSpaceItemViewDelegate* const delegate_;
   const HoldingSpaceItem* const item_;
+  views::ToggleImageButton* pin_ = nullptr;
 
   // Whether or not this view is selected.
   bool selected_ = false;
+
+  base::WeakPtrFactory<HoldingSpaceItemView> weak_factory_{this};
 };
 
 }  // namespace ash