Skip to content

Commit

Permalink
multipaste: Cache clipboard item icon separately from image data URL
Browse files Browse the repository at this point in the history 
This CL creates a distinction between a clipboard history item's image
and its icon. Although the virtual keyboard treats a file item's icon as
its image data today, in the future we will have icons for each item
type. This change adds support for that on the clipboard history side.

Bug: b/252366283, b/267690087, b/266947643
Change-Id: I974d83cbca98c39ad056c1320c4d7e9b7cbfa360
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4282868
Commit-Queue: Colin Kincaid <ckincaid@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/main@{#1111869}
  • Loading branch information
Colin Kincaid authored and Chromium LUCI CQ committed Mar 1, 2023
1 parent 1dbc007 commit b675ee7
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 70 deletions.
21 changes: 20 additions & 1 deletion ash/clipboard/clipboard_history_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "ash/system/toast/toast_manager_impl.h"
#include "ash/wm/window_util.h"
#include "base/barrier_closure.h"
#include "base/check.h"
#include "base/check_op.h"
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
Expand All @@ -45,6 +46,7 @@
#include "base/unguessable_token.h"
#include "base/values.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/aura/window.h"
#include "ui/aura/window_tree_host.h"
#include "ui/base/accelerators/accelerator.h"
#include "ui/base/clipboard/clipboard_data.h"
Expand All @@ -58,6 +60,7 @@
#include "ui/base/models/simple_menu_model.h"
#include "ui/base/webui/web_ui_util.h"
#include "ui/chromeos/events/keyboard_capability.h"
#include "ui/color/color_provider_source.h"
#include "ui/events/event.h"
#include "ui/events/event_constants.h"
#include "ui/events/keycodes/keyboard_codes_posix.h"
Expand Down Expand Up @@ -537,7 +540,7 @@ void ClipboardHistoryControllerImpl::GetHistoryValuesWithEncodedPNGs(
item_dict.Set(kTimeCopiedKey, item.time_copied().ToJsTimeIgnoringNull());
if (const auto maybe_image_data_url = item.GetImageDataUrl();
maybe_image_data_url.has_value()) {
item_dict.Set(kImageDataKey, maybe_image_data_url.value());
item_dict.Set(kImageDataKey, std::move(maybe_image_data_url.value()));
}
switch (item.display_format()) {
case ClipboardHistoryItem::DisplayFormat::kText:
Expand All @@ -551,6 +554,22 @@ void ClipboardHistoryControllerImpl::GetHistoryValuesWithEncodedPNGs(
item_dict.Set(kDisplayFormatKey, kHtmlFormat);
break;
case ClipboardHistoryItem::DisplayFormat::kFile:
DCHECK(!item_dict.contains(kImageDataKey));

const auto& icon = item.icon();
DCHECK(icon.has_value());

const auto* active_window = window_util::GetActiveWindow();
const auto* color_provider =
ColorUtil::GetColorProviderSourceForWindow(
active_window ? active_window
: Shell::Get()->GetPrimaryRootWindow())
->GetColorProvider();
DCHECK(color_provider);

item_dict.Set(
kImageDataKey,
webui::GetBitmapDataUrl(*icon->Rasterize(color_provider).bitmap()));
item_dict.Set(kTextDataKey, item.display_text());
item_dict.Set(kDisplayFormatKey, kFileFormat);
break;
Expand Down
35 changes: 13 additions & 22 deletions ash/clipboard/clipboard_history_item.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,16 @@
#include <vector>

#include "ash/clipboard/clipboard_history_util.h"
#include "ash/shell.h"
#include "ash/style/color_util.h"
#include "base/notreached.h"
#include "base/strings/escape.h"
#include "base/strings/string_piece.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/time/time.h"
#include "ui/aura/window.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/models/image_model.h"
#include "ui/base/webui/web_ui_util.h"
#include "ui/color/color_provider_source.h"
#include "ui/gfx/image/image.h"
#include "ui/strings/grit/ui_strings.h"

Expand Down Expand Up @@ -105,6 +102,14 @@ std::u16string DetermineDisplayText(const ClipboardHistoryItem& item) {
}
}

absl::optional<ui::ImageModel> DetermineIcon(const ClipboardHistoryItem& item) {
if (item.display_format() != ClipboardHistoryItem::DisplayFormat::kFile) {
return absl::nullopt;
}

return clipboard_history_util::GetIconForFileClipboardItem(item);
}

} // namespace

ClipboardHistoryItem::ClipboardHistoryItem(ui::ClipboardData data)
Expand All @@ -113,7 +118,8 @@ ClipboardHistoryItem::ClipboardHistoryItem(ui::ClipboardData data)
time_copied_(base::Time::Now()),
main_format_(clipboard_history_util::CalculateMainFormat(data_).value()),
display_format_(CalculateDisplayFormat(*this)),
display_text_(DetermineDisplayText(*this)) {
display_text_(DetermineDisplayText(*this)),
icon_(DetermineIcon(*this)) {
if (display_format_ == DisplayFormat::kHtml) {
// The `ClipboardHistoryResourceManager` will update this preview once an
// image model is rendered.
Expand Down Expand Up @@ -143,34 +149,19 @@ absl::optional<std::string> ClipboardHistoryItem::GetImageDataUrl() const {
absl::optional<std::string> maybe_url;
switch (display_format_) {
case DisplayFormat::kText:
case DisplayFormat::kFile:
break;
case DisplayFormat::kPng:
if (const auto& maybe_png = data_.maybe_png(); maybe_png.has_value()) {
maybe_url = webui::GetPngDataUrl(maybe_png.value().data(),
maybe_png.value().size());
}
break;
case DisplayFormat::kHtml: {
case DisplayFormat::kHtml:
DCHECK(html_preview_.has_value());
maybe_url =
webui::GetBitmapDataUrl(*html_preview_->GetImage().ToSkBitmap());
break;
}
case DisplayFormat::kFile: {
// TODO(b/267690087): Treat icons as their own item field, separate from
// potential image data.
std::string file_name = base::UTF16ToUTF8(display_text_);
ui::ImageModel image_model =
clipboard_history_util::GetIconForFileClipboardItem(this, file_name);
// TODO(b/252366283): Refactor so we don't use the RootWindow from Shell.
const ui::ColorProvider* color_provider =
ColorUtil::GetColorProviderSourceForWindow(
Shell::Get()->GetPrimaryRootWindow())
->GetColorProvider();
maybe_url = webui::GetBitmapDataUrl(
*image_model.Rasterize(color_provider).bitmap());
break;
}
}
return maybe_url;
}
Expand Down
6 changes: 6 additions & 0 deletions ash/clipboard/clipboard_history_item.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class ASH_EXPORT ClipboardHistoryItem {
ui::ClipboardData ReplaceEquivalentData(ui::ClipboardData&& new_data);

// Returns the data URL for this item's PNG or bitmap image, if any.
// TODO(b/266947643): Move data URL logic to `ChromeVirtualKeyboardDelegate`.
absl::optional<std::string> GetImageDataUrl() const;

const base::UnguessableToken& id() const { return id_; }
Expand All @@ -62,6 +63,7 @@ class ASH_EXPORT ClipboardHistoryItem {
return html_preview_;
}
const std::u16string& display_text() const { return display_text_; }
const absl::optional<ui::ImageModel>& icon() const { return icon_; }

private:
// Unique identifier.
Expand All @@ -87,6 +89,10 @@ class ASH_EXPORT ClipboardHistoryItem {

// The text that should be displayed on this item's menu entry.
const std::u16string display_text_;

// Cached image model for the item's icon. Currently, there will be no value
// for non-file items.
const absl::optional<ui::ImageModel> icon_;
};

} // namespace ash
Expand Down
107 changes: 74 additions & 33 deletions ash/clipboard/clipboard_history_item_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,42 +18,16 @@

namespace ash {

using ClipboardHistoryItemTest = AshTestBase;
namespace {

TEST_F(ClipboardHistoryItemTest, GetImageDataUrl) {
using DisplayFormat = ClipboardHistoryItem::DisplayFormat;
struct FormatPair {
ui::ClipboardInternalFormat clipboard_format;
ClipboardHistoryItem::DisplayFormat display_format;
};

constexpr const auto* kDataUrlStart = "data:image/png;base64,";
} // namespace

for (size_t i = 0; i <= static_cast<size_t>(DisplayFormat::kMaxValue); ++i) {
ClipboardHistoryItemBuilder builder;
const auto display_format = static_cast<DisplayFormat>(i);
switch (display_format) {
case DisplayFormat::kText:
builder.SetFormat(ui::ClipboardInternalFormat::kText);
break;
case DisplayFormat::kPng:
builder.SetFormat(ui::ClipboardInternalFormat::kPng);
break;
case DisplayFormat::kHtml:
builder.SetFormat(ui::ClipboardInternalFormat::kHtml);
break;
case DisplayFormat::kFile:
builder.SetFormat(ui::ClipboardInternalFormat::kFilenames);
break;
}
const auto item = builder.Build();
EXPECT_EQ(item.display_format(), display_format);

const auto maybe_image_data_url = item.GetImageDataUrl();
if (display_format == DisplayFormat::kText) {
EXPECT_FALSE(maybe_image_data_url);
} else {
ASSERT_TRUE(maybe_image_data_url);
EXPECT_TRUE(base::StartsWith(*maybe_image_data_url, kDataUrlStart));
}
}
}
using ClipboardHistoryItemTest = AshTestBase;

TEST_F(ClipboardHistoryItemTest, DisplayText) {
base::test::ScopedRestoreICUDefaultLocale locale("en_US");
Expand Down Expand Up @@ -125,4 +99,71 @@ TEST_F(ClipboardHistoryItemTest, DisplayText) {
EXPECT_EQ(builder.Build().display_text(), u"File.txt, Other File.txt");
}

class ClipboardHistoryItemDisplayTest
: public ClipboardHistoryItemTest,
public testing::WithParamInterface<FormatPair> {
public:
ClipboardHistoryItemDisplayTest() : item_(BuildClipboardHistoryItem()) {}
~ClipboardHistoryItemDisplayTest() override = default;

ui::ClipboardInternalFormat GetClipboardFormat() const {
return GetParam().clipboard_format;
}
ClipboardHistoryItem::DisplayFormat GetDisplayFormat() const {
return GetParam().display_format;
}

const ClipboardHistoryItem& item() const { return item_; }

private:
ClipboardHistoryItem BuildClipboardHistoryItem() const {
ClipboardHistoryItemBuilder builder;
builder.SetFormat(GetClipboardFormat());
ClipboardHistoryItem item = builder.Build();
EXPECT_EQ(item.display_format(), GetDisplayFormat());
return item;
}

const ClipboardHistoryItem item_;
};

INSTANTIATE_TEST_SUITE_P(
All,
ClipboardHistoryItemDisplayTest,
::testing::Values(FormatPair{ui::ClipboardInternalFormat::kText,
ClipboardHistoryItem::DisplayFormat::kText},
FormatPair{ui::ClipboardInternalFormat::kPng,
ClipboardHistoryItem::DisplayFormat::kPng},
FormatPair{ui::ClipboardInternalFormat::kHtml,
ClipboardHistoryItem::DisplayFormat::kHtml},
FormatPair{ui::ClipboardInternalFormat::kFilenames,
ClipboardHistoryItem::DisplayFormat::kFile}));

TEST_P(ClipboardHistoryItemDisplayTest, Icon) {
const auto& maybe_icon = item().icon();
if (GetDisplayFormat() == ClipboardHistoryItem::DisplayFormat::kFile) {
ASSERT_TRUE(maybe_icon.has_value());
EXPECT_TRUE(maybe_icon.value().IsVectorIcon());
} else {
EXPECT_FALSE(maybe_icon.has_value());
}
}

TEST_P(ClipboardHistoryItemDisplayTest, GetImageDataUrl) {
constexpr const auto* kDataUrlStart = "data:image/png;base64,";

const auto maybe_image_data_url = item().GetImageDataUrl();
switch (GetDisplayFormat()) {
case ClipboardHistoryItem::DisplayFormat::kText:
case ClipboardHistoryItem::DisplayFormat::kFile:
EXPECT_FALSE(maybe_image_data_url);
break;
case ClipboardHistoryItem::DisplayFormat::kPng:
case ClipboardHistoryItem::DisplayFormat::kHtml:
ASSERT_TRUE(maybe_image_data_url);
EXPECT_TRUE(base::StartsWith(*maybe_image_data_url, kDataUrlStart));
break;
}
}

} // namespace ash
16 changes: 7 additions & 9 deletions ash/clipboard/clipboard_history_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,9 @@ bool IsEnabledInCurrentMode() {
}
}

ui::ImageModel GetIconForFileClipboardItem(const ClipboardHistoryItem* item,
const std::string& file_name) {
DCHECK_EQ(item->display_format(), ClipboardHistoryItem::DisplayFormat::kFile);
const int copied_files_count = GetCountOfCopiedFiles(item->data());

ui::ImageModel GetIconForFileClipboardItem(const ClipboardHistoryItem& item) {
DCHECK_EQ(item.display_format(), ClipboardHistoryItem::DisplayFormat::kFile);
const int copied_files_count = GetCountOfCopiedFiles(item.data());
if (copied_files_count == 0)
return ui::ImageModel();

Expand All @@ -222,10 +220,10 @@ ui::ImageModel GetIconForFileClipboardItem(const ClipboardHistoryItem* item,
&kEightFilesIcon, &kNineFilesIcon, &kMoreThanNineFilesIcon};
int icon_index = std::min(copied_files_count - 2, (int)icons.size() - 1);

const auto* vector_icon =
copied_files_count == 1
? &chromeos::GetIconForPath(base::FilePath(file_name))
: icons[icon_index];
const auto* vector_icon = copied_files_count == 1
? &chromeos::GetIconForPath(base::FilePath(
base::UTF16ToUTF8(item.display_text())))
: icons[icon_index];
return ui::ImageModel::FromVectorIcon(*vector_icon, ui::kColorSysSecondary);
}

Expand Down
3 changes: 1 addition & 2 deletions ash/clipboard/clipboard_history_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,7 @@ ASH_EXPORT bool IsEnabledInCurrentMode();

// Returns an image icon for the file clipboard item.
ASH_EXPORT ui::ImageModel GetIconForFileClipboardItem(
const ClipboardHistoryItem* item,
const std::string& file_name);
const ClipboardHistoryItem& item);

// Returns a placeholder image to display for HTML items while their previews
// render.
Expand Down
7 changes: 4 additions & 3 deletions ash/clipboard/views/clipboard_history_file_item_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

#include <array>

#include "ash/clipboard/clipboard_history_util.h"
#include "ash/clipboard/clipboard_history_item.h"
#include "base/check.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/view_class_properties.h"
Expand Down Expand Up @@ -35,12 +36,12 @@ ClipboardHistoryFileItemView::CreateContentsView() {

// `file_icon` should be `contents_view`'s first child.
if (const auto* item = GetClipboardHistoryItem()) {
DCHECK(item->icon().has_value());
views::ImageView* file_icon = contents_view->AddChildViewAt(
std::make_unique<views::ImageView>(), /*index=*/0);
file_icon->SetImageSize(kIconSize);
file_icon->SetProperty(views::kMarginsKey, kIconMargin);
file_icon->SetImage(clipboard_history_util::GetIconForFileClipboardItem(
item, base::UTF16ToUTF8(text())));
file_icon->SetImage(item->icon().value());
}

return contents_view;
Expand Down

0 comments on commit b675ee7

Please sign in to comment.